diff mbox series

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

Message ID 20240723131029.1159908-3-peter.maydell@linaro.org
State Superseded
Headers show
Series hw/misc/bcm2835_property: Fix set-palette, OTP-access properties | expand

Commit Message

Peter Maydell July 23, 2024, 1:10 p.m. UTC
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(-)

Comments

Philippe Mathieu-Daudé July 24, 2024, 7:06 a.m. UTC | #1
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.
Peter Maydell July 24, 2024, 12:31 p.m. UTC | #2
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
Philippe Mathieu-Daudé July 25, 2024, 6:57 a.m. UTC | #3
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.
Michael Tokarev Aug. 2, 2024, 7:02 a.m. UTC | #4
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));
Michael Tokarev Aug. 2, 2024, 7:13 a.m. UTC | #5
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
diff mbox series

Patch

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));