| Message ID | 1476275861-27613-1-git-send-email-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Any ideas about this fix? Regards, -Gonglei > -----Original Message----- > From: Qemu-devel > [mailto:qemu-devel-bounces+arei.gonglei=huawei.com@nongnu.org] On > Behalf Of P J P > Sent: Wednesday, October 12, 2016 8:38 PM > To: Qemu Developers > Cc: Paolo Bonzini; Huawei PSIRT; Prasad J Pandit; Michael S. Tsirkin > Subject: [Qemu-devel] [PATCH] dma: rc4030: limit interval timer reload value > > From: Prasad J Pandit <pjp@fedoraproject.org> > > The JAZZ RC4030 chipset emulator has a periodic timer and > associated interval reload register. The reload value is used > as divider when computing timer's next tick value. If reload > value is large, it could lead to divide by zero error. Limit > the interval reload value to avoid it. > > Reported-by: Huawei PSIRT <psirt@huawei.com> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > --- > hw/dma/rc4030.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/dma/rc4030.c b/hw/dma/rc4030.c > index 2f2576f..c1b4997 100644 > --- a/hw/dma/rc4030.c > +++ b/hw/dma/rc4030.c > @@ -460,7 +460,7 @@ static void rc4030_write(void *opaque, hwaddr addr, > uint64_t data, > break; > /* Interval timer reload */ > case 0x0228: > - s->itr = val; > + s->itr = val & 0x01FF; > qemu_irq_lower(s->timer_irq); > set_next_tick(s); > break; > -- > 2.5.5 >
On 10/11/2016 06:56, Gonglei (Arei) wrote: > Any ideas about this fix? It seems sensible, but perhaps the field is even smaller. Let's CC Hervé and Aurelien as I don't have a datasheet for this device. Also, s->itr is used here: tm_hz = 1000 / (s->itr + 1); timer_mod(s->periodic_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + NANOSECONDS_PER_SECOND / tm_hz); and this is the same as timer_mod(s->periodic_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + NANOSECONDS_PER_SECOND / 1000 * (s->itr + 1)); so perhaps it's better to do it like that. Paolo >> -----Original Message----- >> From: Qemu-devel >> [mailto:qemu-devel-bounces+arei.gonglei=huawei.com@nongnu.org] On >> Behalf Of P J P >> Sent: Wednesday, October 12, 2016 8:38 PM >> To: Qemu Developers >> Cc: Paolo Bonzini; Huawei PSIRT; Prasad J Pandit; Michael S. Tsirkin >> Subject: [Qemu-devel] [PATCH] dma: rc4030: limit interval timer reload value >> >> From: Prasad J Pandit <pjp@fedoraproject.org> >> >> The JAZZ RC4030 chipset emulator has a periodic timer and >> associated interval reload register. The reload value is used >> as divider when computing timer's next tick value. If reload >> value is large, it could lead to divide by zero error. Limit >> the interval reload value to avoid it. >> >> Reported-by: Huawei PSIRT <psirt@huawei.com> >> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> >> --- >> hw/dma/rc4030.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/hw/dma/rc4030.c b/hw/dma/rc4030.c >> index 2f2576f..c1b4997 100644 >> --- a/hw/dma/rc4030.c >> +++ b/hw/dma/rc4030.c >> @@ -460,7 +460,7 @@ static void rc4030_write(void *opaque, hwaddr addr, >> uint64_t data, >> break; >> /* Interval timer reload */ >> case 0x0228: >> - s->itr = val; >> + s->itr = val & 0x01FF; >> qemu_irq_lower(s->timer_irq); >> set_next_tick(s); >> break; >> -- >> 2.5.5 >> > > >
Hi, Le 10/11/2016 à 15:50, Paolo Bonzini a écrit : > > > On 10/11/2016 06:56, Gonglei (Arei) wrote: >> Any ideas about this fix? > > It seems sensible, but perhaps the field is even smaller. Let's CC > Hervé and Aurelien as I don't have a datasheet for this device. Sorry for the delay... I don't have any datasheet for this device either, so I tested with real programs. Those initialize itr field to either 0 or to 9, so your mask doesn't change anything. Tested-by: Hervé Poussineau <hpoussin@reactos.org> > > Also, s->itr is used here: > > tm_hz = 1000 / (s->itr + 1); > > timer_mod(s->periodic_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + > NANOSECONDS_PER_SECOND / tm_hz); > > and this is the same as > > timer_mod(s->periodic_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + > NANOSECONDS_PER_SECOND / 1000 * (s->itr + 1)); > > so perhaps it's better to do it like that. > > Paolo > >>> -----Original Message----- >>> From: Qemu-devel >>> [mailto:qemu-devel-bounces+arei.gonglei=huawei.com@nongnu.org] On >>> Behalf Of P J P >>> Sent: Wednesday, October 12, 2016 8:38 PM >>> To: Qemu Developers >>> Cc: Paolo Bonzini; Huawei PSIRT; Prasad J Pandit; Michael S. Tsirkin >>> Subject: [Qemu-devel] [PATCH] dma: rc4030: limit interval timer reload value >>> >>> From: Prasad J Pandit <pjp@fedoraproject.org> >>> >>> The JAZZ RC4030 chipset emulator has a periodic timer and >>> associated interval reload register. The reload value is used >>> as divider when computing timer's next tick value. If reload >>> value is large, it could lead to divide by zero error. Limit >>> the interval reload value to avoid it. >>> >>> Reported-by: Huawei PSIRT <psirt@huawei.com> >>> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> >>> --- >>> hw/dma/rc4030.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/hw/dma/rc4030.c b/hw/dma/rc4030.c >>> index 2f2576f..c1b4997 100644 >>> --- a/hw/dma/rc4030.c >>> +++ b/hw/dma/rc4030.c >>> @@ -460,7 +460,7 @@ static void rc4030_write(void *opaque, hwaddr addr, >>> uint64_t data, >>> break; >>> /* Interval timer reload */ >>> case 0x0228: >>> - s->itr = val; >>> + s->itr = val & 0x01FF; >>> qemu_irq_lower(s->timer_irq); >>> set_next_tick(s); >>> break; >>> -- >>> 2.5.5 >>> >> >> >> >
> Subject: Re: [Qemu-devel] [PATCH] dma: rc4030: limit interval timer reload > value > > Hi, > > Le 10/11/2016 à 15:50, Paolo Bonzini a écrit : > > > > > > On 10/11/2016 06:56, Gonglei (Arei) wrote: > >> Any ideas about this fix? > > > > It seems sensible, but perhaps the field is even smaller. Let's CC > > Hervé and Aurelien as I don't have a datasheet for this device. > > Sorry for the delay... > > I don't have any datasheet for this device either, so I tested with real > programs. > Those initialize itr field to either 0 or to 9, so your mask doesn't change > anything. > > Tested-by: Hervé Poussineau <hpoussin@reactos.org> > Thanks for your feedback. Paolo, maybe you can post a formal patch :) Regards, -Gonglei > > > > Also, s->itr is used here: > > > > tm_hz = 1000 / (s->itr + 1); > > > > timer_mod(s->periodic_timer, > qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + > > NANOSECONDS_PER_SECOND / tm_hz); > > > > and this is the same as > > > > timer_mod(s->periodic_timer, > qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + > > NANOSECONDS_PER_SECOND / 1000 * (s->itr + 1)); > > > > so perhaps it's better to do it like that. > > > > Paolo > > > >>> -----Original Message----- > >>> From: Qemu-devel > >>> [mailto:qemu-devel-bounces+arei.gonglei=huawei.com@nongnu.org] On > >>> Behalf Of P J P > >>> Sent: Wednesday, October 12, 2016 8:38 PM > >>> To: Qemu Developers > >>> Cc: Paolo Bonzini; Huawei PSIRT; Prasad J Pandit; Michael S. Tsirkin > >>> Subject: [Qemu-devel] [PATCH] dma: rc4030: limit interval timer reload > value > >>> > >>> From: Prasad J Pandit <pjp@fedoraproject.org> > >>> > >>> The JAZZ RC4030 chipset emulator has a periodic timer and > >>> associated interval reload register. The reload value is used > >>> as divider when computing timer's next tick value. If reload > >>> value is large, it could lead to divide by zero error. Limit > >>> the interval reload value to avoid it. > >>> > >>> Reported-by: Huawei PSIRT <psirt@huawei.com> > >>> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > >>> --- > >>> hw/dma/rc4030.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/hw/dma/rc4030.c b/hw/dma/rc4030.c > >>> index 2f2576f..c1b4997 100644 > >>> --- a/hw/dma/rc4030.c > >>> +++ b/hw/dma/rc4030.c > >>> @@ -460,7 +460,7 @@ static void rc4030_write(void *opaque, hwaddr > addr, > >>> uint64_t data, > >>> break; > >>> /* Interval timer reload */ > >>> case 0x0228: > >>> - s->itr = val; > >>> + s->itr = val & 0x01FF; > >>> qemu_irq_lower(s->timer_irq); > >>> set_next_tick(s); > >>> break; > >>> -- > >>> 2.5.5 > >>> > >> > >> > >> > > >
+-- On Wed, 16 Nov 2016, Hervé Poussineau wrote --+ | I don't have any datasheet for this device either, so I tested with real | programs. Those initialize itr field to either 0 or to 9, so your mask | doesn't change anything. | | Tested-by: Hervé Poussineau <hpoussin@reactos.org> Thank you so much. To confirm, do we need to update the mask to maybe 0x000F? | > > > case 0x0228: | > > > - s->itr = val; | > > > + s->itr = val & 0x01FF; | > > > qemu_irq_lower(s->timer_irq); Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Le 16/11/2016 à 07:29, P J P a écrit : > +-- On Wed, 16 Nov 2016, Hervé Poussineau wrote --+ > | I don't have any datasheet for this device either, so I tested with real > | programs. Those initialize itr field to either 0 or to 9, so your mask > | doesn't change anything. > | > | Tested-by: Hervé Poussineau <hpoussin@reactos.org> > > Thank you so much. To confirm, do we need to update the mask to maybe > 0x000F? No, I think that 0x1ff is fine. 0xf mask means timers between 10ms to 1000ms 0x1ff mask means timers between 2ms to 1000ms, which seem also acceptable to me. Hervé > > | > > > case 0x0228: > | > > > - s->itr = val; > | > > > + s->itr = val & 0x01FF; > | > > > qemu_irq_lower(s->timer_irq); > > Thank you. > -- > Prasad J Pandit / Red Hat Product Security Team > 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F >
On 16/11/2016 07:29, P J P wrote: > +-- On Wed, 16 Nov 2016, Hervé Poussineau wrote --+ > | I don't have any datasheet for this device either, so I tested with real > | programs. Those initialize itr field to either 0 or to 9, so your mask > | doesn't change anything. > | > | Tested-by: Hervé Poussineau <hpoussin@reactos.org> > > Thank you so much. To confirm, do we need to update the mask to maybe > 0x000F? It's not needed if you change it as suggested elsewhere in the thread. timer_mod(s->periodic_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + NANOSECONDS_PER_SECOND / 1000 * (s->itr + 1)); Thanks, Paolo > | > > > case 0x0228: > | > > > - s->itr = val; > | > > > + s->itr = val & 0x01FF; > | > > > qemu_irq_lower(s->timer_irq);
On 11/16/2016 12:50 AM, Hervé Poussineau wrote: > Hi, > > Le 10/11/2016 à 15:50, Paolo Bonzini a écrit : >> >> >> On 10/11/2016 06:56, Gonglei (Arei) wrote: >>> Any ideas about this fix? >> >> It seems sensible, but perhaps the field is even smaller. Let's CC >> Hervé and Aurelien as I don't have a datasheet for this device. > > Sorry for the delay... > > I don't have any datasheet for this device either, so I tested with real > programs. > Those initialize itr field to either 0 or to 9, so your mask doesn't change > anything. > > Tested-by: Hervé Poussineau <hpoussin@reactos.org> > I'm coming to this thread from the Fedora bug for this CVE, https://bugzilla.redhat.com/show_bug.cgi?id=1384876 I don't see this patch in qemu.git yet, can someone pick it up for a pull request? Thanks, Cole
On 14 March 2017 at 19:17, Cole Robinson <crobinso@redhat.com> wrote: > I'm coming to this thread from the Fedora bug for this CVE, > https://bugzilla.redhat.com/show_bug.cgi?id=1384876 FWIW this isn't a CVE issue from the point of view of upstream QEMU, because it only affects the MIPS Jazz board, which (if I'm reading the source correctly) you can't use with KVM. Still, we should fix the bug... > I don't see this patch in qemu.git yet, can someone pick it up > for a pull request? Ccing the MIPS maintainer may help in achieving this :-) thanks -- PMM
diff --git a/hw/dma/rc4030.c b/hw/dma/rc4030.c index 2f2576f..c1b4997 100644 --- a/hw/dma/rc4030.c +++ b/hw/dma/rc4030.c @@ -460,7 +460,7 @@ static void rc4030_write(void *opaque, hwaddr addr, uint64_t data, break; /* Interval timer reload */ case 0x0228: - s->itr = val; + s->itr = val & 0x01FF; qemu_irq_lower(s->timer_irq); set_next_tick(s); break;