[PATCH 0/4] hw/misc/bcm2835_property: Fix set-palette, OTP-access properties

[PATCH 0/4] hw/misc/bcm2835_property: Fix set-palette, OTP-access properties

[Peter Maydell]

Coverity pointed out a problem in the bcm2835_property handling of
the OTP access properties, where a mixup between uint32_t and int
meant that the guest could hand us a very large unsigned value
that we would end up treating as a negative number. In the course
of fixing that I noticed that we also don't handle the set-palette
property correctly. This series:
 * fixes set-palette (enforcing the range restrictions on the
   inputs from the guest and getting the loop termination condition
   correct)
 * uses uint32_t rather than int for the loop variable in the
   OTP access properties to avoid the overflow problem
 * cleans up the code by making various variables have only the
   scope they need rather than being declared once at the top of
   this 400 line function

Only patches 1 and 2 are strictly bugfixes (and cc'd to stable); patches
3 and 4 are small and safe but don't really need to be backported.

thanks
-- PMM

Peter Maydell (4):
  hw/misc/bcm2835_property: Fix handling of FRAMEBUFFER_SET_PALETTE
  hw/misc/bcm2835_property: Avoid overflow in OTP access properties
  hw/misc/bcm2835_property: Restrict scope of start_num, number, otp_row
  hw/misc/bcm2835_property: Reduce scope of variables in mbox push
    function

 hw/misc/bcm2835_property.c | 91 +++++++++++++++++++++-----------------
 1 file changed, 50 insertions(+), 41 deletions(-)

-- 
2.34.1

[PATCH 1/4] hw/misc/bcm2835_property: Fix handling of FRAMEBUFFER_SET_PALETTE

[Peter Maydell]

The documentation of the "Set palette" mailbox property at
https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface#set-palette
says it has the form:

    Length: 24..1032
    Value:
        u32: offset: first palette index to set (0-255)
        u32: length: number of palette entries to set (1-256)
        u32...: RGBA palette values (offset to offset+length-1)

We get this wrong in a couple of ways:
 * we aren't checking the offset and length are in range, so the guest
   can make us spin for a long time by providing a large length
 * the bounds check on our loop is wrong: we should iterate through
   'length' palette entries, not 'length - offset' entries

Fix the loop to implement the bounds checks and get the loop
condition right. In the process, make the variables local to
this switch case, rather than function-global, so it's clearer
what type they are when reading the code.

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/misc/bcm2835_property.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c
index 63de3db6215..e28fdca9846 100644
--- a/hw/misc/bcm2835_property.c
+++ b/hw/misc/bcm2835_property.c
@@ -31,7 +31,6 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
     size_t resplen;
     uint32_t tmp;
     int n;
-    uint32_t offset, length, color;
     uint32_t start_num, number, otp_row;
 
     /*
@@ -274,19 +273,25 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
             resplen = 16;
             break;
         case RPI_FWREQ_FRAMEBUFFER_SET_PALETTE:
-            offset = ldl_le_phys(&s->dma_as, value + 12);
-            length = ldl_le_phys(&s->dma_as, value + 16);
-            n = 0;
-            while (n < length - offset) {
-                color = ldl_le_phys(&s->dma_as, value + 20 + (n << 2));
-                stl_le_phys(&s->dma_as,
-                            s->fbdev->vcram_base + ((offset + n) << 2), color);
-                n++;
+        {
+            uint32_t offset = ldl_le_phys(&s->dma_as, value + 12);
+            uint32_t length = ldl_le_phys(&s->dma_as, value + 16);
+            int resp;
+
+            if (offset > 255 || length < 1 || length > 256) {
+                resp = 1; /* invalid request */
+            } else {
+                for (uint32_t e = 0; e < length; e++) {
+                    uint32_t color = ldl_le_phys(&s->dma_as, value + 20 + (e << 2));
+                    stl_le_phys(&s->dma_as,
+                                s->fbdev->vcram_base + ((offset + e) << 2), color);
+                }
+                resp = 0;
             }
-            stl_le_phys(&s->dma_as, value + 12, 0);
+            stl_le_phys(&s->dma_as, value + 12, resp);
             resplen = 4;
             break;
-
+        }
         case RPI_FWREQ_FRAMEBUFFER_GET_NUM_DISPLAYS:
             stl_le_phys(&s->dma_as, value + 12, 1);
             resplen = 4;
-- 
2.34.1

[PATCH 2/4] hw/misc/bcm2835_property: Avoid overflow in OTP access properties

[Peter Maydell]

Coverity points out that in our handling of the property
RPI_FWREQ_SET_CUSTOMER_OTP we have a potential overflow.  This
happens because we read start_num and number from the guest as
unsigned 32 bit integers, but then the variable 'n' we use as a loop
counter as we iterate from start_num to start_num + number is only an
"int".  That means that if the guest passes us a very large start_num
we will interpret it as negative.  This will result in an assertion
failure inside bcm2835_otp_set_row(), which checks that we didn't
pass it an invalid row number.

A similar issue applies to all the properties for accessing OTP rows
where we are iterating through with a start and length read from the
guest.

Use uint32_t for the loop counter to avoid this problem. Because in
all cases 'n' is only used as a loop counter, we can do this as
part of the for(), restricting its scope to exactly where we need it.

Resolves: Coverity CID 1549401
Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/misc/bcm2835_property.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c
index e28fdca9846..7eb623b4e90 100644
--- a/hw/misc/bcm2835_property.c
+++ b/hw/misc/bcm2835_property.c
@@ -30,7 +30,6 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
     uint32_t tot_len;
     size_t resplen;
     uint32_t tmp;
-    int n;
     uint32_t start_num, number, otp_row;
 
     /*
@@ -337,7 +336,7 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
 
             resplen = 8 + 4 * number;
 
-            for (n = start_num; n < start_num + number &&
+            for (uint32_t n = start_num; n < start_num + number &&
                  n < BCM2835_OTP_CUSTOMER_OTP_LEN; n++) {
                 otp_row = bcm2835_otp_get_row(s->otp,
                                               BCM2835_OTP_CUSTOMER_OTP + n);
@@ -366,7 +365,7 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
                 break;
             }
 
-            for (n = start_num; n < start_num + number &&
+            for (uint32_t n = start_num; n < start_num + number &&
                  n < BCM2835_OTP_CUSTOMER_OTP_LEN; n++) {
                 otp_row = ldl_le_phys(&s->dma_as,
                                       value + 20 + ((n - start_num) << 2));
@@ -383,7 +382,7 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
 
             resplen = 8 + 4 * number;
 
-            for (n = start_num; n < start_num + number &&
+            for (uint32_t n = start_num; n < start_num + number &&
                  n < BCM2835_OTP_PRIVATE_KEY_LEN; n++) {
                 otp_row = bcm2835_otp_get_row(s->otp,
                                               BCM2835_OTP_PRIVATE_KEY + n);
@@ -403,7 +402,7 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
                 break;
             }
 
-            for (n = start_num; n < start_num + number &&
+            for (uint32_t n = start_num; n < start_num + number &&
                  n < BCM2835_OTP_PRIVATE_KEY_LEN; n++) {
                 otp_row = ldl_le_phys(&s->dma_as,
                                       value + 20 + ((n - start_num) << 2));
-- 
2.34.1

[PATCH 3/4] hw/misc/bcm2835_property: Restrict scope of start_num, number, otp_row

[Peter Maydell]

In the long function bcm2835_property_mbox_push(), the variables
start_num, number and otp_row are used only in the four cases which
access OTP data, and their uses don't overlap with each other.

Make these variables have scope restricted to the cases where they're
used, so it's easier to read each individual case without having to
cross-refer up to the variable declaration at the top of the function
and check whether the variable is also used later in the loop.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/misc/bcm2835_property.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c
index 7eb623b4e90..443d42a1824 100644
--- a/hw/misc/bcm2835_property.c
+++ b/hw/misc/bcm2835_property.c
@@ -30,7 +30,6 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
     uint32_t tot_len;
     size_t resplen;
     uint32_t tmp;
-    uint32_t start_num, number, otp_row;
 
     /*
      * Copy the current state of the framebuffer config; we will update
@@ -331,22 +330,25 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
         /* Customer OTP */
 
         case RPI_FWREQ_GET_CUSTOMER_OTP:
-            start_num = ldl_le_phys(&s->dma_as, value + 12);
-            number = ldl_le_phys(&s->dma_as, value + 16);
+        {
+            uint32_t start_num = ldl_le_phys(&s->dma_as, value + 12);
+            uint32_t number = ldl_le_phys(&s->dma_as, value + 16);
 
             resplen = 8 + 4 * number;
 
             for (uint32_t n = start_num; n < start_num + number &&
                  n < BCM2835_OTP_CUSTOMER_OTP_LEN; n++) {
-                otp_row = bcm2835_otp_get_row(s->otp,
+                uint32_t otp_row = bcm2835_otp_get_row(s->otp,
                                               BCM2835_OTP_CUSTOMER_OTP + n);
                 stl_le_phys(&s->dma_as,
                             value + 20 + ((n - start_num) << 2), otp_row);
             }
             break;
+        }
         case RPI_FWREQ_SET_CUSTOMER_OTP:
-            start_num = ldl_le_phys(&s->dma_as, value + 12);
-            number = ldl_le_phys(&s->dma_as, value + 16);
+        {
+            uint32_t start_num = ldl_le_phys(&s->dma_as, value + 12);
+            uint32_t number = ldl_le_phys(&s->dma_as, value + 16);
 
             resplen = 4;
 
@@ -367,32 +369,35 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
 
             for (uint32_t n = start_num; n < start_num + number &&
                  n < BCM2835_OTP_CUSTOMER_OTP_LEN; n++) {
-                otp_row = ldl_le_phys(&s->dma_as,
+                uint32_t otp_row = ldl_le_phys(&s->dma_as,
                                       value + 20 + ((n - start_num) << 2));
                 bcm2835_otp_set_row(s->otp,
                                     BCM2835_OTP_CUSTOMER_OTP + n, otp_row);
             }
             break;
+        }
 
         /* Device-specific private key */
-
         case RPI_FWREQ_GET_PRIVATE_KEY:
-            start_num = ldl_le_phys(&s->dma_as, value + 12);
-            number = ldl_le_phys(&s->dma_as, value + 16);
+        {
+            uint32_t start_num = ldl_le_phys(&s->dma_as, value + 12);
+            uint32_t number = ldl_le_phys(&s->dma_as, value + 16);
 
             resplen = 8 + 4 * number;
 
             for (uint32_t n = start_num; n < start_num + number &&
                  n < BCM2835_OTP_PRIVATE_KEY_LEN; n++) {
-                otp_row = bcm2835_otp_get_row(s->otp,
+                uint32_t otp_row = bcm2835_otp_get_row(s->otp,
                                               BCM2835_OTP_PRIVATE_KEY + n);
                 stl_le_phys(&s->dma_as,
                             value + 20 + ((n - start_num) << 2), otp_row);
             }
             break;
+        }
         case RPI_FWREQ_SET_PRIVATE_KEY:
-            start_num = ldl_le_phys(&s->dma_as, value + 12);
-            number = ldl_le_phys(&s->dma_as, value + 16);
+        {
+            uint32_t start_num = ldl_le_phys(&s->dma_as, value + 12);
+            uint32_t number = ldl_le_phys(&s->dma_as, value + 16);
 
             resplen = 4;
 
@@ -404,12 +409,13 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
 
             for (uint32_t n = start_num; n < start_num + number &&
                  n < BCM2835_OTP_PRIVATE_KEY_LEN; n++) {
-                otp_row = ldl_le_phys(&s->dma_as,
+                uint32_t otp_row = ldl_le_phys(&s->dma_as,
                                       value + 20 + ((n - start_num) << 2));
                 bcm2835_otp_set_row(s->otp,
                                     BCM2835_OTP_PRIVATE_KEY + n, otp_row);
             }
             break;
+        }
         default:
             qemu_log_mask(LOG_UNIMP,
                           "bcm2835_property: unhandled tag 0x%08x\n", tag);
-- 
2.34.1

[PATCH 4/4] hw/misc/bcm2835_property: Reduce scope of variables in mbox push function

[Peter Maydell]

In bcm2835_property_mbox_push(), some variables are defined at function scope
but used only in a smaller scope of the function:
 * tag, bufsize, resplen are used only in the body of the while() loop
 * tmp is used only for RPI_FWREQ_SET_POWER_STATE (and is badly named)

Declare these variables in the scope where they're needed, so the code
is easier to read.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/misc/bcm2835_property.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c
index 443d42a1824..8ca3128f29b 100644
--- a/hw/misc/bcm2835_property.c
+++ b/hw/misc/bcm2835_property.c
@@ -25,11 +25,7 @@
 
 static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
 {
-    uint32_t tag;
-    uint32_t bufsize;
     uint32_t tot_len;
-    size_t resplen;
-    uint32_t tmp;
 
     /*
      * Copy the current state of the framebuffer config; we will update
@@ -48,10 +44,10 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
     /* @(addr + 4) : Buffer response code */
     value = s->addr + 8;
     while (value + 8 <= s->addr + tot_len) {
-        tag = ldl_le_phys(&s->dma_as, value);
-        bufsize = ldl_le_phys(&s->dma_as, value + 4);
+        uint32_t tag = ldl_le_phys(&s->dma_as, value);
+        uint32_t bufsize = ldl_le_phys(&s->dma_as, value + 4);
         /* @(value + 8) : Request/response indicator */
-        resplen = 0;
+        size_t resplen = 0;
         switch (tag) {
         case RPI_FWREQ_PROPERTY_END:
             break;
@@ -95,13 +91,16 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
             resplen = 8;
             break;
         case RPI_FWREQ_SET_POWER_STATE:
-            /* Assume that whatever device they asked for exists,
-             * and we'll just claim we set it to the desired state
+        {
+            /*
+             * Assume that whatever device they asked for exists,
+             * and we'll just claim we set it to the desired state.
              */
-            tmp = ldl_le_phys(&s->dma_as, value + 16);
-            stl_le_phys(&s->dma_as, value + 16, (tmp & 1));
+            uint32_t state = ldl_le_phys(&s->dma_as, value + 16);
+            stl_le_phys(&s->dma_as, value + 16, (state & 1));
             resplen = 8;
             break;
+        }
 
         /* Clocks */
 
-- 
2.34.1

Re: [PATCH 4/4] hw/misc/bcm2835_property: Reduce scope of variables in mbox push function

[Philippe Mathieu-Daudé]

On 23/7/24 15:10, Peter Maydell wrote:
> In bcm2835_property_mbox_push(), some variables are defined at function scope
> but used only in a smaller scope of the function:
>   * tag, bufsize, resplen are used only in the body of the while() loop
>   * tmp is used only for RPI_FWREQ_SET_POWER_STATE (and is badly named)
> 
> Declare these variables in the scope where they're needed, so the code
> is easier to read.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/misc/bcm2835_property.c | 21 ++++++++++-----------
>   1 file changed, 10 insertions(+), 11 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 3/4] hw/misc/bcm2835_property: Restrict scope of start_num, number, otp_row

[Philippe Mathieu-Daudé]

On 23/7/24 15:10, Peter Maydell wrote:
> In the long function bcm2835_property_mbox_push(), the variables
> start_num, number and otp_row are used only in the four cases which
> access OTP data, and their uses don't overlap with each other.
> 
> Make these variables have scope restricted to the cases where they're
> used, so it's easier to read each individual case without having to
> cross-refer up to the variable declaration at the top of the function
> and check whether the variable is also used later in the loop.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/misc/bcm2835_property.c | 34 ++++++++++++++++++++--------------
>   1 file changed, 20 insertions(+), 14 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 2/4] hw/misc/bcm2835_property: Avoid overflow in OTP access properties

[Philippe Mathieu-Daudé]

Hi Peter,

On 23/7/24 15:10, Peter Maydell wrote:
> Coverity points out that in our handling of the property
> RPI_FWREQ_SET_CUSTOMER_OTP we have a potential overflow.  This
> happens because we read start_num and number from the guest as
> unsigned 32 bit integers, but then the variable 'n' we use as a loop
> counter as we iterate from start_num to start_num + number is only an
> "int".  That means that if the guest passes us a very large start_num
> we will interpret it as negative.  This will result in an assertion
> failure inside bcm2835_otp_set_row(), which checks that we didn't
> pass it an invalid row number.
> 
> A similar issue applies to all the properties for accessing OTP rows
> where we are iterating through with a start and length read from the
> guest.
> 
> Use uint32_t for the loop counter to avoid this problem. Because in
> all cases 'n' is only used as a loop counter, we can do this as
> part of the for(), restricting its scope to exactly where we need it.
> 
> Resolves: Coverity CID 1549401
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> ---
>   hw/misc/bcm2835_property.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c
> index e28fdca9846..7eb623b4e90 100644
> --- a/hw/misc/bcm2835_property.c
> +++ b/hw/misc/bcm2835_property.c
> @@ -30,7 +30,6 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
>       uint32_t tot_len;
>       size_t resplen;
>       uint32_t tmp;
> -    int n;
>       uint32_t start_num, number, otp_row;
>   
>       /*
> @@ -337,7 +336,7 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
>   
>               resplen = 8 + 4 * number;
>   
> -            for (n = start_num; n < start_num + number &&
> +            for (uint32_t n = start_num; n < start_num + number &&
>                    n < BCM2835_OTP_CUSTOMER_OTP_LEN; n++) {

I find not making the counter size explicit and use 'unsigned'
simpler, since using 32-bit in particular doesn't bring much here.
Is there a reason I'm missing?

Thanks,

Phil.

Re: [PATCH 2/4] hw/misc/bcm2835_property: Avoid overflow in OTP access properties

[Peter Maydell]

On Wed, 24 Jul 2024 at 08:06, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi Peter,
>
> On 23/7/24 15:10, Peter Maydell wrote:
> > Coverity points out that in our handling of the property
> > RPI_FWREQ_SET_CUSTOMER_OTP we have a potential overflow.  This
> > happens because we read start_num and number from the guest as
> > unsigned 32 bit integers, but then the variable 'n' we use as a loop
> > counter as we iterate from start_num to start_num + number is only an
> > "int".  That means that if the guest passes us a very large start_num
> > we will interpret it as negative.  This will result in an assertion
> > failure inside bcm2835_otp_set_row(), which checks that we didn't
> > pass it an invalid row number.
> >
> > A similar issue applies to all the properties for accessing OTP rows
> > where we are iterating through with a start and length read from the
> > guest.
> >
> > Use uint32_t for the loop counter to avoid this problem. Because in
> > all cases 'n' is only used as a loop counter, we can do this as
> > part of the for(), restricting its scope to exactly where we need it.
> >
> > Resolves: Coverity CID 1549401
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> > ---
> >   hw/misc/bcm2835_property.c | 9 ++++-----
> >   1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c
> > index e28fdca9846..7eb623b4e90 100644
> > --- a/hw/misc/bcm2835_property.c
> > +++ b/hw/misc/bcm2835_property.c
> > @@ -30,7 +30,6 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
> >       uint32_t tot_len;
> >       size_t resplen;
> >       uint32_t tmp;
> > -    int n;
> >       uint32_t start_num, number, otp_row;
> >
> >       /*
> > @@ -337,7 +336,7 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
> >
> >               resplen = 8 + 4 * number;
> >
> > -            for (n = start_num; n < start_num + number &&
> > +            for (uint32_t n = start_num; n < start_num + number &&
> >                    n < BCM2835_OTP_CUSTOMER_OTP_LEN; n++) {
>
> I find not making the counter size explicit and use 'unsigned'
> simpler, since using 32-bit in particular doesn't bring much here.
> Is there a reason I'm missing?

I just wanted to match the types between n and start_num and
number (where the latter two should be uint32_t because we load
them from the guest as 32-bit values). Otherwise we're relying
on "unsigned" being at least 32 bit -- it is, but if we need
it to be 32 bit then why not use the type that is guaranteed
and says specifically that it's 32 bits ?

thanks
-- PMM

Re: [PATCH 2/4] hw/misc/bcm2835_property: Avoid overflow in OTP access properties

[Philippe Mathieu-Daudé]

On 24/7/24 14:31, Peter Maydell wrote:
> On Wed, 24 Jul 2024 at 08:06, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Hi Peter,
>>
>> On 23/7/24 15:10, Peter Maydell wrote:
>>> Coverity points out that in our handling of the property
>>> RPI_FWREQ_SET_CUSTOMER_OTP we have a potential overflow.  This
>>> happens because we read start_num and number from the guest as
>>> unsigned 32 bit integers, but then the variable 'n' we use as a loop
>>> counter as we iterate from start_num to start_num + number is only an
>>> "int".  That means that if the guest passes us a very large start_num
>>> we will interpret it as negative.  This will result in an assertion
>>> failure inside bcm2835_otp_set_row(), which checks that we didn't
>>> pass it an invalid row number.
>>>
>>> A similar issue applies to all the properties for accessing OTP rows
>>> where we are iterating through with a start and length read from the
>>> guest.
>>>
>>> Use uint32_t for the loop counter to avoid this problem. Because in
>>> all cases 'n' is only used as a loop counter, we can do this as
>>> part of the for(), restricting its scope to exactly where we need it.
>>>
>>> Resolves: Coverity CID 1549401
>>> Cc: qemu-stable@nongnu.org
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>
>>> ---
>>>    hw/misc/bcm2835_property.c | 9 ++++-----
>>>    1 file changed, 4 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c
>>> index e28fdca9846..7eb623b4e90 100644
>>> --- a/hw/misc/bcm2835_property.c
>>> +++ b/hw/misc/bcm2835_property.c
>>> @@ -30,7 +30,6 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
>>>        uint32_t tot_len;
>>>        size_t resplen;
>>>        uint32_t tmp;
>>> -    int n;
>>>        uint32_t start_num, number, otp_row;
>>>
>>>        /*
>>> @@ -337,7 +336,7 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
>>>
>>>                resplen = 8 + 4 * number;
>>>
>>> -            for (n = start_num; n < start_num + number &&
>>> +            for (uint32_t n = start_num; n < start_num + number &&
>>>                     n < BCM2835_OTP_CUSTOMER_OTP_LEN; n++) {
>>
>> I find not making the counter size explicit and use 'unsigned'
>> simpler, since using 32-bit in particular doesn't bring much here.
>> Is there a reason I'm missing?
> 
> I just wanted to match the types between n and start_num and
> number (where the latter two should be uint32_t because we load
> them from the guest as 32-bit values). Otherwise we're relying
> on "unsigned" being at least 32 bit -- it is, but if we need
> it to be 32 bit then why not use the type that is guaranteed
> and says specifically that it's 32 bits ?

Yes OK no problem.

Re: [PATCH 1/4] hw/misc/bcm2835_property: Fix handling of FRAMEBUFFER_SET_PALETTE

[Philippe Mathieu-Daudé]

On 23/7/24 15:10, Peter Maydell wrote:
> The documentation of the "Set palette" mailbox property at
> https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface#set-palette
> says it has the form:
> 
>      Length: 24..1032
>      Value:
>          u32: offset: first palette index to set (0-255)
>          u32: length: number of palette entries to set (1-256)
>          u32...: RGBA palette values (offset to offset+length-1)
> 
> We get this wrong in a couple of ways:
>   * we aren't checking the offset and length are in range, so the guest
>     can make us spin for a long time by providing a large length
>   * the bounds check on our loop is wrong: we should iterate through
>     'length' palette entries, not 'length - offset' entries
> 
> Fix the loop to implement the bounds checks and get the loop
> condition right. In the process, make the variables local to
> this switch case, rather than function-global, so it's clearer
> what type they are when reading the code.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/misc/bcm2835_property.c | 27 ++++++++++++++++-----------
>   1 file changed, 16 insertions(+), 11 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 2/4] hw/misc/bcm2835_property: Avoid overflow in OTP access properties

[Michael Tokarev]

23.07.2024 16:10, Peter Maydell wrote:
> Coverity points out that in our handling of the property
> RPI_FWREQ_SET_CUSTOMER_OTP we have a potential overflow.  This
> happens because we read start_num and number from the guest as
> unsigned 32 bit integers, but then the variable 'n' we use as a loop
> counter as we iterate from start_num to start_num + number is only an
> "int".  That means that if the guest passes us a very large start_num
> we will interpret it as negative.  This will result in an assertion
> failure inside bcm2835_otp_set_row(), which checks that we didn't
> pass it an invalid row number.
> 
> A similar issue applies to all the properties for accessing OTP rows
> where we are iterating through with a start and length read from the
> guest.

This is a fun one wrt the -stable series.

The code which is mentioned in the subject and above (OTP access
properties) is introduced in v9.0.0-1812-g5d5f1b60916a " hw/misc:Implement
mailbox properties for customer OTP and device specific private keys",
which is not in any released version of qemu.  However, the next comment
("A similar issue..") tells us the same prob exists in all other
cases in the same function.  So the fix mentioned in subject does not
apply to -stable, while "all others" "side-fix" does :)

I wonder why coverity didn't notice the other cases here.

Thanks,

/mjt

> Use uint32_t for the loop counter to avoid this problem. Because in
> all cases 'n' is only used as a loop counter, we can do this as
> part of the for(), restricting its scope to exactly where we need it.
> 
> Resolves: Coverity CID 1549401
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/misc/bcm2835_property.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c
> index e28fdca9846..7eb623b4e90 100644
> --- a/hw/misc/bcm2835_property.c
> +++ b/hw/misc/bcm2835_property.c
> @@ -30,7 +30,6 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
>       uint32_t tot_len;
>       size_t resplen;
>       uint32_t tmp;
> -    int n;
>       uint32_t start_num, number, otp_row;
>   
>       /*
> @@ -337,7 +336,7 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
>   
>               resplen = 8 + 4 * number;
>   
> -            for (n = start_num; n < start_num + number &&
> +            for (uint32_t n = start_num; n < start_num + number &&
>                    n < BCM2835_OTP_CUSTOMER_OTP_LEN; n++) {
>                   otp_row = bcm2835_otp_get_row(s->otp,
>                                                 BCM2835_OTP_CUSTOMER_OTP + n);
> @@ -366,7 +365,7 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
>                   break;
>               }
>   
> -            for (n = start_num; n < start_num + number &&
> +            for (uint32_t n = start_num; n < start_num + number &&
>                    n < BCM2835_OTP_CUSTOMER_OTP_LEN; n++) {
>                   otp_row = ldl_le_phys(&s->dma_as,
>                                         value + 20 + ((n - start_num) << 2));
> @@ -383,7 +382,7 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
>   
>               resplen = 8 + 4 * number;
>   
> -            for (n = start_num; n < start_num + number &&
> +            for (uint32_t n = start_num; n < start_num + number &&
>                    n < BCM2835_OTP_PRIVATE_KEY_LEN; n++) {
>                   otp_row = bcm2835_otp_get_row(s->otp,
>                                                 BCM2835_OTP_PRIVATE_KEY + n);
> @@ -403,7 +402,7 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
>                   break;
>               }
>   
> -            for (n = start_num; n < start_num + number &&
> +            for (uint32_t n = start_num; n < start_num + number &&
>                    n < BCM2835_OTP_PRIVATE_KEY_LEN; n++) {
>                   otp_row = ldl_le_phys(&s->dma_as,
>                                         value + 20 + ((n - start_num) << 2));

-- 
GPG Key transition (from rsa2048 to rsa4096) since 2024-04-24.
New key: rsa4096/61AD3D98ECDF2C8E  9D8B E14E 3F2A 9DD7 9199  28F1 61AD 3D98 ECDF 2C8E
Old key: rsa2048/457CE0A0804465C5  6EE1 95D1 886E 8FFB 810D  4324 457C E0A0 8044 65C5
Transition statement: http://www.corpit.ru/mjt/gpg-transition-2024.txt

Re: [PATCH 2/4] hw/misc/bcm2835_property: Avoid overflow in OTP access properties

[Michael Tokarev]

02.08.2024 10:02, Michael Tokarev wrote:
> 23.07.2024 16:10, Peter Maydell wrote:
>> Coverity points out that in our handling of the property
>> RPI_FWREQ_SET_CUSTOMER_OTP we have a potential overflow.  This
>> happens because we read start_num and number from the guest as
>> unsigned 32 bit integers, but then the variable 'n' we use as a loop
>> counter as we iterate from start_num to start_num + number is only an
>> "int".  That means that if the guest passes us a very large start_num
>> we will interpret it as negative.  This will result in an assertion
>> failure inside bcm2835_otp_set_row(), which checks that we didn't
>> pass it an invalid row number.
>>
>> A similar issue applies to all the properties for accessing OTP rows
>> where we are iterating through with a start and length read from the
>> guest.
> 
> This is a fun one wrt the -stable series.
> 
> The code which is mentioned in the subject and above (OTP access
> properties) is introduced in v9.0.0-1812-g5d5f1b60916a " hw/misc:Implement
> mailbox properties for customer OTP and device specific private keys",
> which is not in any released version of qemu.  However, the next comment
> ("A similar issue..") tells us the same prob exists in all other
> cases in the same function.  So the fix mentioned in subject does not
> apply to -stable, while "all others" "side-fix" does :)

Okay, there's no "all other" case here, it is really all about OTP access.
This change basically only removes the 'n' variable for stable-9.0, which
isn't used since the previous patch anyway, and the build fails after the
previous patch due to this.

Still fun, but in a different way :)

I'll add removal of this `n' variable to the previous patch for -stable.

Thanks,

/mjt