[PATCH] ipmi:smbus: Add a check around a memcpy

[PATCH] ipmi:smbus: Add a check around a memcpy

[minyard]

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

Re: [PATCH] ipmi:smbus: Add a check around a memcpy

[Michael S. Tsirkin]

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

Re: [PATCH] ipmi:smbus: Add a check around a memcpy

[Peter Maydell]

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