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