| Message ID | 20220731230245.1216949-1-minyard@acm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ipmi:smbus: Add a check around a memcpy | expand |
On Sun, Jul 31, 2022 at 06:02:46PM -0500, minyard@acm.org wrote: > From: Corey Minyard <cminyard@mvista.com> > > In one case: > > memcpy(sid->inmsg + sid->inlen, buf, len); > > if len == 0 then sid->inmsg + sig->inlen can point to one past the inmsg > array if the array is full. We have to allow len == 0 due to some > vagueness in the spec, but we don't have to call memcpy. > > Found by Coverity. This is not a problem in practice, but the results > are technically (maybe) undefined. So make Coverity happy. > > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Corey Minyard <cminyard@mvista.com> Acked-by: Michael S. Tsirkin <mst@redhat.com> > --- > hw/ipmi/smbus_ipmi.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > I think this should do it. > > diff --git a/hw/ipmi/smbus_ipmi.c b/hw/ipmi/smbus_ipmi.c > index 9ef9112dd5..d0991ab7f9 100644 > --- a/hw/ipmi/smbus_ipmi.c > +++ b/hw/ipmi/smbus_ipmi.c > @@ -281,7 +281,9 @@ static int ipmi_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len) > */ > send = true; > } > - memcpy(sid->inmsg + sid->inlen, buf, len); > + if (len > 0) { > + memcpy(sid->inmsg + sid->inlen, buf, len); > + } > sid->inlen += len; > break; > } > -- > 2.25.1
On Mon, 1 Aug 2022 at 00:03, <minyard@acm.org> wrote: > > From: Corey Minyard <cminyard@mvista.com> > > In one case: > > memcpy(sid->inmsg + sid->inlen, buf, len); > > if len == 0 then sid->inmsg + sig->inlen can point to one past the inmsg > array if the array is full. We have to allow len == 0 due to some > vagueness in the spec, but we don't have to call memcpy. > > Found by Coverity. This is not a problem in practice, but the results > are technically (maybe) undefined. So make Coverity happy. > > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Corey Minyard <cminyard@mvista.com> > --- > hw/ipmi/smbus_ipmi.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > I think this should do it. > > diff --git a/hw/ipmi/smbus_ipmi.c b/hw/ipmi/smbus_ipmi.c > index 9ef9112dd5..d0991ab7f9 100644 > --- a/hw/ipmi/smbus_ipmi.c > +++ b/hw/ipmi/smbus_ipmi.c > @@ -281,7 +281,9 @@ static int ipmi_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len) > */ > send = true; > } > - memcpy(sid->inmsg + sid->inlen, buf, len); > + if (len > 0) { > + memcpy(sid->inmsg + sid->inlen, buf, len); > + } > sid->inlen += len; > break; > } > -- > 2.25.1 Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
diff --git a/hw/ipmi/smbus_ipmi.c b/hw/ipmi/smbus_ipmi.c index 9ef9112dd5..d0991ab7f9 100644 --- a/hw/ipmi/smbus_ipmi.c +++ b/hw/ipmi/smbus_ipmi.c @@ -281,7 +281,9 @@ static int ipmi_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len) */ send = true; } - memcpy(sid->inmsg + sid->inlen, buf, len); + if (len > 0) { + memcpy(sid->inmsg + sid->inlen, buf, len); + } sid->inlen += len; break; }