[Qemu-devel] [PATCH v1] arm: check bit index before usage

[Qemu-devel] [PATCH v1] arm: check bit index before usage

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

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.

Reported-by: Moguofang <moguofang@huawei.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 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

diff --git a/hw/arm/strongarm.c b/hw/arm/strongarm.c
index ec2627374d..9225b1ba6e 100644
--- a/hw/arm/strongarm.c
+++ b/hw/arm/strongarm.c
@@ -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

Re: [Qemu-devel] [PATCH v1] arm: check bit index before usage

[Philippe Mathieu-Daudé]

Hi Prasad,

On 22/10/18 20:10, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> 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.
> 
> Reported-by: Moguofang <moguofang@huawei.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>   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
> 
> diff --git a/hw/arm/strongarm.c b/hw/arm/strongarm.c
> index ec2627374d..9225b1ba6e 100644
> --- a/hw/arm/strongarm.c
> +++ b/hw/arm/strongarm.c
> @@ -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);

            } else {
                	qemu_log_mask(LOG_GUEST_ERROR, ...

With that:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +        }
>       }
>   
>       s->prev_level = level;
> 

Re: [Qemu-devel] [PATCH v1] arm: check bit index before usage

[P J P]

+-- On Tue, 23 Oct 2018, Philippe Mathieu-Daudé wrote --+
| > From: Prasad J Pandit <pjp@fedoraproject.org>
| > 
| > 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é <philmd@redhat.com>

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

Re: [Qemu-devel] [PATCH v1] arm: check bit index before usage

[Peter Maydell]

On 22 October 2018 at 19:10, P J P <ppandit@redhat.com> wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> 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.
>
> Reported-by: Moguofang <moguofang@huawei.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  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
>

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

Re: [Qemu-devel] [PATCH v1] arm: check bit index before usage

[P J P]

+-- 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.

===
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?
 
| 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

Re: [Qemu-devel] [PATCH v1] arm: check bit index before usage

[Peter Maydell]

On 25 October 2018 at 21:31, P J P <ppandit@redhat.com> wrote:
> +-- 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.
>
> ===
> 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.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1] arm: check bit index before usage

[P J P]

+-- 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