On Tue, Jun 28, 2022 at 05:21:44PM +0100, Peter Maydell wrote:
> On Thu, 19 Sept 2019 at 22:39, <miny...@acm.org> wrote:
> >
> > From: Corey Minyard <cminy...@mvista.com>
> >
> > Signed-off-by: Corey Minyard <cminy...@mvista.com>
> > ---
>
Thank you for the ping. Comments inline...
> Very old patch, but Coverity has decided it doesn't like something
> in this function that's still basically the same in the current codebase
> (CID 1487146):
>
> > +static int ipmi_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len)
> > +{
> > + SMBusIPMIDevice *sid = SMBUS_IPMI(dev);
> > + bool send = false;
> > + uint8_t cmd;
> > + int ret = 0;
> > +
> > + /* length is guaranteed to be >= 1. */
> > + cmd = *buf++;
> > + len--;
> > +
> > + /* Handle read request, which don't have any data in the write part. */
> > + switch (cmd) {
> > + case SSIF_IPMI_RESPONSE:
> > + sid->currblk = 0;
> > + ret = ipmi_load_readbuf(sid);
> > + break;
> > +
> > + case SSIF_IPMI_MULTI_PART_RESPONSE_MIDDLE:
> > + sid->currblk++;
> > + ret = ipmi_load_readbuf(sid);
> > + break;
> > +
> > + case SSIF_IPMI_MULTI_PART_RETRY:
> > + if (len >= 1) {
> > + sid->currblk = buf[0];
> > + ret = ipmi_load_readbuf(sid);
> > + } else {
> > + ret = -1;
> > + }
> > + break;
> > +
> > + default:
> > + break;
> > + }
> > +
> > + /* This should be a message write, make the length is there and
> > correct. */
> > + if (len >= 1) {
> > + if (*buf != len - 1 || *buf > MAX_SSIF_IPMI_MSG_CHUNK) {
> > + return -1; /* Bogus message */
> > + }
> > + buf++;
> > + len--;
> > + }
>
> After all of this preamble, len can be zero...
>
> > +
> > + switch (cmd) {
> > + case SSIF_IPMI_REQUEST:
> > + send = true;
> > + /* FALLTHRU */
> > + case SSIF_IPMI_MULTI_PART_REQUEST_START:
> > + if (len < 2) {
> > + return -1; /* Bogus. */
> > + }
> > + memcpy(sid->inmsg, buf, len);
> > + sid->inlen = len;
> > + break;
> > +
> > + case SSIF_IPMI_MULTI_PART_REQUEST_END:
> > + send = true;
> > + /* FALLTHRU */
> > + case SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE:
> > + if (!sid->inlen) {
> > + return -1; /* Bogus. */
> > + }
> > + if (sid->inlen + len > MAX_SSIF_IPMI_MSG_SIZE) {
> > + sid->inlen = 0; /* Discard the message. */
> > + return -1; /* Bogus. */
> > + }
>
> ...this error checking on the values of the 'middle' request
> means that after one 'middle' request we can end up with
> sid->inlen == MAX_SSIF_IPMI_MSG_SIZE (ie we filled the
> entire sid->inmsg[] array).
>
> But then if we get another 'middle' request with len == 0,
> that will pass this error checking because (sid->inlen + len == MSG_SIZE)
> and fall through into...
>
> > + if (len < 32) {
> > + /*
> > + * Special hack, a multi-part middle that is less than 32 bytes
> > + * marks the end of a message. The specification is fairly
> > + * confusing, so some systems to this, even sending a zero
> > + * length end message to mark the end.
> > + */
> > + send = true;
> > + }
> > + memcpy(sid->inmsg + sid->inlen, buf, len);
>
> ...calling memcpy() with argument 1 being a pointer that points
> one past the end of the array. Even though len will be 0 and
> we won't memcpy() anything, this is (depending on how you choose
> to intepret things the C standard doesn't come right out and state
> explicitly) undefined behaviour, because memcpy() wants to be passed
> valid pointers, even if you ask it to do no work with a zero len.
>
> This isn't going to be a visible bug in practical terms, but it would
> make Coverity happy if we either (a) rejected a request with an empty
> length or else (b) skipped the memcpy(). I don't know enough about
> IPMI to know which is better.
Hmm. In some cases you have to accept a zero-length packet (as
described in the comments), but if you said:
if (len > 0)
memcpy(sid->inmsg + sid->inlen, buf, len);
would that make Coverity happy? I was under the impression that if you
passed zero into len, you could pass anything into the data on a memcpy.
But apparently not; I can make this change.
-corey
>
> > + sid->inlen += len;
> > + break;
> > + }
> > +
> > + if (send && sid->inlen) {
> > + smbus_ipmi_send_msg(sid);
> > + }
> > +
> > + return ret;
> > +}
>
> thanks
> -- PMM
>
