| Message ID | 20240723131029.1159908-3-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/misc/bcm2835_property: Fix set-palette, OTP-access properties | expand |
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.
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
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.
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));
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 --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));
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(-)