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

The high[32:28] bits of 'direction' and 'state' registers of
SA-1100/SA-1110 device are reserved. Setting them may lead to
OOB 's->handler[]' array access issue. Mask off [32:28] bits to
avoid it.

Reported-by: Moguofang <moguofang@huawei.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/arm/strongarm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Update v2: mask off high[32:28] bits
  -> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg05746.html

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

On 26 October 2018 at 08:30, P J P <ppandit@redhat.com> wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> The high[32:28] bits of 'direction' and 'state' registers of
> SA-1100/SA-1110 device are reserved. Setting them may lead to
> OOB 's->handler[]' array access issue. Mask off [32:28] bits to
> avoid it.

There is no bit 32 in a 32-bit value; you mean 31.

>
> Reported-by: Moguofang <moguofang@huawei.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/arm/strongarm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> Update v2: mask off high[32:28] bits
>   -> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg05746.html
>
> 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;

The commit message says it's masking [31:28], but the
code is masking [31:22]. The SA1110 spec suggests the
commit message is correct and the code is not.

thanks
-- PMM

+-- On Mon, 29 Oct 2018, Peter Maydell wrote --+
| >      switch (offset) {
| >      case GPDR:        /* GPIO Pin-Direction registers */
| > -        s->dir = value;
| > +        s->dir = value & 0x3fffff;
| 
| The commit message says it's masking [31:28], but the
| code is masking [31:22]. The SA1110 spec suggests the
| commit message is correct and the code is not.

Ouch, sorry! Sent revised patch v3.

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

On 30/10/18 12:49, P J P wrote:
> +-- On Mon, 29 Oct 2018, Peter Maydell wrote --+
> | >      switch (offset) {
> | >      case GPDR:        /* GPIO Pin-Direction registers */
> | > -        s->dir = value;
> | > +        s->dir = value & 0x3fffff;
> |
> | The commit message says it's masking [31:28], but the
> | code is masking [31:22]. The SA1110 spec suggests the
> | commit message is correct and the code is not.
> 
> Ouch, sorry! Sent revised patch v3.

That's where the extract32() is more convenient and less bug prone:

                s->dir = extract32(value, 0, 28); /* mask off [31:28] */
> 
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
>