| Message ID | 20181022181035.20104-1-ppandit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] arm: check bit index before usage | expand |
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; >
+-- 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
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
+-- 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
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
+-- 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
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;