[PATCH 0/3] m68k: Fix a couple of Coverity nits

[PATCH 0/3] m68k: Fix a couple of Coverity nits

[Peter Maydell]

This patchset fixes a couple of very minor Coverity issues: one
"shift off top of integer", and one "overflow in multiplication". 
Neither will happen in normal use. I would not bother backporting
either fix to stable.

I threw in also a patch adding a comment to mcf5208.c giving the URLs
of the datasheets, since I had to go find them to check the fix in
patch 1.

thanks
-- PMM

Peter Maydell (3):
  hw/m68k/mcf5208: Avoid shifting off end of integer
  hw/m68k/mcf5208: Add URLs for datasheets
  hw/nubus/nubus-device: Range check 'slot' property

 hw/m68k/mcf5208.c       | 10 +++++++++-
 hw/nubus/nubus-device.c |  7 +++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

-- 
2.34.1

[PATCH 1/3] hw/m68k/mcf5208: Avoid shifting off end of integer

[Peter Maydell]

In m5208_sys_read(), we have a loop of n from 0 to 31, and we
calculate (2u << n).  For the n == 31 iteration this will shift off
the top of the unsigned 32 bit integer.

This is harmless, because we're going to stop the loop with n == 31
anyway, but we can avoid the error by using 64-bit arithmetic here.

(The SDCS0 register is documented at
https://www.nxp.com/docs/en/reference-manual/MCF5208RM.pdf
section 18.4.5; we want the lower 5 bits to indicate the
RAM size, where 31 == 4GB, 30 == 2GB, and so on down.
As it happens, the layout of the mcf5208evb board memory map
means it doesn't make sense to have more than 1GB of RAM
in any case.)

Resolves: Coverity CID 1547727
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/m68k/mcf5208.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c
index ec14096aa43..0ad347dfa81 100644
--- a/hw/m68k/mcf5208.c
+++ b/hw/m68k/mcf5208.c
@@ -158,7 +158,7 @@ static uint64_t m5208_sys_read(void *opaque, hwaddr addr,
         {
             int n;
             for (n = 0; n < 32; n++) {
-                if (current_machine->ram_size < (2u << n)) {
+                if (current_machine->ram_size < (2ULL << n)) {
                     break;
                 }
             }
-- 
2.34.1

Re: [PATCH 1/3] hw/m68k/mcf5208: Avoid shifting off end of integer

[Thomas Huth]

Am Fri, 30 Aug 2024 18:34:50 +0100
schrieb Peter Maydell <peter.maydell@linaro.org>:

> In m5208_sys_read(), we have a loop of n from 0 to 31, and we
> calculate (2u << n).  For the n == 31 iteration this will shift off
> the top of the unsigned 32 bit integer.
> 
> This is harmless, because we're going to stop the loop with n == 31
> anyway, but we can avoid the error by using 64-bit arithmetic here.
> 
> (The SDCS0 register is documented at
> https://www.nxp.com/docs/en/reference-manual/MCF5208RM.pdf
> section 18.4.5; we want the lower 5 bits to indicate the
> RAM size, where 31 == 4GB, 30 == 2GB, and so on down.
> As it happens, the layout of the mcf5208evb board memory map
> means it doesn't make sense to have more than 1GB of RAM
> in any case.)
> 
> Resolves: Coverity CID 1547727
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/m68k/mcf5208.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c
> index ec14096aa43..0ad347dfa81 100644
> --- a/hw/m68k/mcf5208.c
> +++ b/hw/m68k/mcf5208.c
> @@ -158,7 +158,7 @@ static uint64_t m5208_sys_read(void *opaque, hwaddr addr,
>          {
>              int n;
>              for (n = 0; n < 32; n++) {
> -                if (current_machine->ram_size < (2u << n)) {
> +                if (current_machine->ram_size < (2ULL << n)) {
>                      break;
>                  }
>              }

Reviewed-by: Thomas Huth <huth@tuxfamily.org>

Re: [PATCH 1/3] hw/m68k/mcf5208: Avoid shifting off end of integer

[Richard Henderson]

On 8/31/24 03:34, Peter Maydell wrote:
> In m5208_sys_read(), we have a loop of n from 0 to 31, and we
> calculate (2u << n).  For the n == 31 iteration this will shift off
> the top of the unsigned 32 bit integer.
> 
> This is harmless, because we're going to stop the loop with n == 31
> anyway, but we can avoid the error by using 64-bit arithmetic here.
> 
> (The SDCS0 register is documented at
> https://www.nxp.com/docs/en/reference-manual/MCF5208RM.pdf
> section 18.4.5; we want the lower 5 bits to indicate the
> RAM size, where 31 == 4GB, 30 == 2GB, and so on down.
> As it happens, the layout of the mcf5208evb board memory map
> means it doesn't make sense to have more than 1GB of RAM
> in any case.)
> 
> Resolves: Coverity CID 1547727
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/m68k/mcf5208.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c
> index ec14096aa43..0ad347dfa81 100644
> --- a/hw/m68k/mcf5208.c
> +++ b/hw/m68k/mcf5208.c
> @@ -158,7 +158,7 @@ static uint64_t m5208_sys_read(void *opaque, hwaddr addr,
>           {
>               int n;
>               for (n = 0; n < 32; n++) {
> -                if (current_machine->ram_size < (2u << n)) {
> +                if (current_machine->ram_size < (2ULL << n)) {
>                       break;
>                   }
>               }

   31 - clz32(ram_size)

?

r~

Re: [PATCH 1/3] hw/m68k/mcf5208: Avoid shifting off end of integer

[Richard Henderson]

On 9/2/24 07:34, Richard Henderson wrote:
>> @@ -158,7 +158,7 @@ static uint64_t m5208_sys_read(void *opaque, hwaddr addr,
>>           {
>>               int n;
>>               for (n = 0; n < 32; n++) {
>> -                if (current_machine->ram_size < (2u << n)) {
>> +                if (current_machine->ram_size < (2ULL << n)) {
>>                       break;
>>                   }
>>               }
> 
>    31 - clz32(ram_size)

We might could do with lg2_pow2ceil() in host-utils.h, if that's clearer.


r~

[PATCH 2/3] hw/m68k/mcf5208: Add URLs for datasheets

[Peter Maydell]

The datasheets for the SoC and board we model here are still
available from the NXP website; add their URLs and titles for
future reference.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/m68k/mcf5208.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c
index 0ad347dfa81..b6677ad6bc3 100644
--- a/hw/m68k/mcf5208.c
+++ b/hw/m68k/mcf5208.c
@@ -4,6 +4,14 @@
  * Copyright (c) 2007 CodeSourcery.
  *
  * This code is licensed under the GPL
+ *
+ * This file models both the MCF5208 SoC, and the
+ * MCF5208EVB evaluation board. For details see
+ *
+ * "MCF5208 Reference Manual"
+ * https://www.nxp.com/docs/en/reference-manual/MCF5208RM.pdf
+ * "M5208EVB-RevB 32-bit Microcontroller User Manual"
+ * https://www.nxp.com/docs/en/reference-manual/M5208EVBUM.pdf
  */
 
 #include "qemu/osdep.h"
-- 
2.34.1

Re: [PATCH 2/3] hw/m68k/mcf5208: Add URLs for datasheets

[Thomas Huth]

Am Fri, 30 Aug 2024 18:34:51 +0100
schrieb Peter Maydell <peter.maydell@linaro.org>:

> The datasheets for the SoC and board we model here are still
> available from the NXP website; add their URLs and titles for
> future reference.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/m68k/mcf5208.c | 8 ++++++++
>  1 file changed, 8 insertions(+)

Reviewed-by: Thomas Huth <huth@tuxfamily.org>

[PATCH 3/3] hw/nubus/nubus-device: Range check 'slot' property

[Peter Maydell]

The TYPE_NUBUS_DEVICE class lets the user specify the nubus slot
using an int32 "slot" QOM property.  Its realize method doesn't do
any range checking on this value, which Coverity notices by way of
the possibility that 'nd->slot * NUBUS_SUPER_SLOT_SIZE' might
overflow the 32-bit arithmetic it is using.

Constrain the slot value to be less than NUBUS_SLOT_NB (16).

Resolves: Coverity CID 1464070
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/nubus/nubus-device.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
index be4cb246966..26fbcf29a2b 100644
--- a/hw/nubus/nubus-device.c
+++ b/hw/nubus/nubus-device.c
@@ -35,6 +35,13 @@ static void nubus_device_realize(DeviceState *dev, Error **errp)
     uint8_t *rom_ptr;
     int ret;
 
+    if (nd->slot < 0 || nd->slot >= NUBUS_SLOT_NB) {
+        error_setg(errp,
+                   "'slot' value %d out of range (must be between 0 and %d)",
+                   nd->slot, NUBUS_SLOT_NB - 1);
+        return;
+    }
+
     /* Super */
     slot_offset = nd->slot * NUBUS_SUPER_SLOT_SIZE;
 
-- 
2.34.1

Re: [PATCH 3/3] hw/nubus/nubus-device: Range check 'slot' property

[Thomas Huth]

Am Fri, 30 Aug 2024 18:34:52 +0100
schrieb Peter Maydell <peter.maydell@linaro.org>:

> The TYPE_NUBUS_DEVICE class lets the user specify the nubus slot
> using an int32 "slot" QOM property.  Its realize method doesn't do
> any range checking on this value, which Coverity notices by way of
> the possibility that 'nd->slot * NUBUS_SUPER_SLOT_SIZE' might
> overflow the 32-bit arithmetic it is using.
> 
> Constrain the slot value to be less than NUBUS_SLOT_NB (16).
> 
> Resolves: Coverity CID 1464070
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/nubus/nubus-device.c | 7 +++++++
>  1 file changed, 7 insertions(+)

Reviewed-by: Thomas Huth <huth@tuxfamily.org>

Re: [PATCH 3/3] hw/nubus/nubus-device: Range check 'slot' property

[Mark Cave-Ayland]

On 30/08/2024 18:34, Peter Maydell wrote:

> The TYPE_NUBUS_DEVICE class lets the user specify the nubus slot
> using an int32 "slot" QOM property.  Its realize method doesn't do
> any range checking on this value, which Coverity notices by way of
> the possibility that 'nd->slot * NUBUS_SUPER_SLOT_SIZE' might
> overflow the 32-bit arithmetic it is using.
> 
> Constrain the slot value to be less than NUBUS_SLOT_NB (16).
> 
> Resolves: Coverity CID 1464070
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/nubus/nubus-device.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
> index be4cb246966..26fbcf29a2b 100644
> --- a/hw/nubus/nubus-device.c
> +++ b/hw/nubus/nubus-device.c
> @@ -35,6 +35,13 @@ static void nubus_device_realize(DeviceState *dev, Error **errp)
>       uint8_t *rom_ptr;
>       int ret;
>   
> +    if (nd->slot < 0 || nd->slot >= NUBUS_SLOT_NB) {
> +        error_setg(errp,
> +                   "'slot' value %d out of range (must be between 0 and %d)",
> +                   nd->slot, NUBUS_SLOT_NB - 1);
> +        return;
> +    }
> +
>       /* Super */
>       slot_offset = nd->slot * NUBUS_SUPER_SLOT_SIZE;

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.