arm: check bit index before usage

From: Prasad J Pandit <[email protected]>

While performing gpio write via strongarm_gpio_handler_update routine, the 'bit' index could access beyond s->handler[28] array. Add check to avoid OOB access.

---
 hw/arm/strongarm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Update v1: use ARRAY_SIZE macro
-> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg04826.html

index ec2627374d..9225b1ba6e 100644
@@ -532,7 +532,9 @@ static void strongarm_gpio_handler_update(StrongARMGPIOInfo *s)
 
     for (diff = s->prev_level ^ level; diff; diff ^= 1 << bit) {
         bit = ctz32(diff);
-        qemu_set_irq(s->handler[bit], (level >> bit) & 1);
+        if (bit < ARRAY_SIZE(s->handler)) {
+            qemu_set_irq(s->handler[bit], (level >> bit) & 1);
+        }
     }
 
     s->prev_level = level;

2.17.2

Hi Prasad,

           } else {
               	qemu_log_mask(LOG_GUEST_ERROR, ...

+-- On Tue, 23 Oct 2018, Philippe Mathieu-Daudé wrote --+
| > From: Prasad J Pandit <[email protected]>
| > 
| > Update v1: use ARRAY_SIZE macro
| >    -> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg04826.html
| > 
| > -        qemu_set_irq(s->handler[bit], (level >> bit) & 1);
| > +        if (bit < ARRAY_SIZE(s->handler)) {
| > +            qemu_set_irq(s->handler[bit], (level >> bit) & 1);
| 
|            } else {
|                	qemu_log_mask(LOG_GUEST_ERROR, ...
| 
| With that:
| Reviewed-by: Philippe Mathieu-Daudé <[email protected]>

Thank you Philippe and Li.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Hi; thanks for this patch. Looking at the SA1110 manual, it says that writes to the reserved bits [31:28] are ignored. So I think that rather than doing this check here, we should do what the strongarm_ppc_* code in the same file does -- mask off the high bits for writes to the direction and state registers. Then it will not be possible for high bits to be set here that cause an out-of-range array access.

Side note: this device is used only in the "collie" machine model, which only works via TCG, so this is not a security issue, just a bug (which will only be visible if the guest is buggy.)

thanks
-- PMM

+-- On Thu, 25 Oct 2018, Peter Maydell wrote --+
| Hi; thanks for this patch. Looking at the SA1110 manual,
| it says that writes to the reserved bits [31:28] are
| ignored. So I think that rather than doing this check
| here, we should do what the strongarm_ppc_* code in the
| same file does -- mask off the high bits for writes to
| the direction and state registers. Then it will not
| be possible for high bits to be set here that cause an
| out-of-range array access.

===

index ec2627374d..dd8c4b1f2e 100644
@@ -587,12 +587,12 @@ static void strongarm_gpio_write(void *opaque, hwaddr 

offset,

switch (offset) {
case GPDR: /* GPIO Pin-Direction registers */
- s->dir = value;

+        s->dir = value & 0x3fffff;
         strongarm_gpio_handler_update(s);
         break;
 
     case GPSR:        /* GPIO Pin-Output Set registers */
-        s->olevel |= value;
+        s->olevel |= value & 0x3fffff;
         strongarm_gpio_handler_update(s);
         break;

===

does this seem okay?

| Side note: this device is used only in the "collie"
| machine model, which only works via TCG, so this is
| not a security issue, just a bug (which will only be
| visible if the guest is buggy.)

Cool, thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Yes, that's what I had in mind.

thanks
-- PMM

+-- On Fri, 26 Oct 2018, Peter Maydell wrote --+
| > ===
| > diff --git a/hw/arm/strongarm.c b/hw/arm/strongarm.c
| > index ec2627374d..dd8c4b1f2e 100644
| > --- a/hw/arm/strongarm.c
| > +++ b/hw/arm/strongarm.c
| > @@ -587,12 +587,12 @@ static void strongarm_gpio_write(void *opaque, hwaddr
| > offset,
| >
| >      switch (offset) {
| >      case GPDR:        /* GPIO Pin-Direction registers */
| > -        s->dir = value;
| > +        s->dir = value & 0x3fffff;
| >          strongarm_gpio_handler_update(s);
| >          break;
| >
| >      case GPSR:        /* GPIO Pin-Output Set registers */
| > -        s->olevel |= value;
| > +        s->olevel |= value & 0x3fffff;
| >          strongarm_gpio_handler_update(s);
| >          break;
| > ===
| >
| > does this seem okay?
| 
| Yes, that's what I had in mind.

Cool, patch sent.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F