Re: [PATCH 12/15] ipmi: Add an SMBus IPMI interface

[Peter Maydell]

On Thu, 19 Sept 2019 at 22:39, <minyard@acm.org> wrote:
>
> From: Corey Minyard <cminyard@mvista.com>
>
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---

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.

> +        sid->inlen += len;
> +        break;
> +    }
> +
> +    if (send && sid->inlen) {
> +        smbus_ipmi_send_msg(sid);
> +    }
> +
> +    return ret;
> +}

thanks
-- PMM