[Qemu-devel] [PATCH] dma: rc4030: limit interval timer reload value

[Qemu-devel] [PATCH] dma: rc4030: limit interval timer reload value

[P J P]

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

Re: [Qemu-devel] [PATCH] dma: rc4030: limit interval timer reload value

[Gonglei (Arei)]

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
> 

Re: [Qemu-devel] [PATCH] dma: rc4030: limit interval timer reload value

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] dma: rc4030: limit interval timer reload value

[Hervé Poussineau]

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

Re: [Qemu-devel] [PATCH] dma: rc4030: limit interval timer reload value

[Gonglei (Arei)]

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

Re: [Qemu-devel] [PATCH] dma: rc4030: limit interval timer reload value

[P J P]

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

Re: [Qemu-devel] [PATCH] dma: rc4030: limit interval timer reload value

[Hervé Poussineau]

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
>

Re: [Qemu-devel] [PATCH] dma: rc4030: limit interval timer reload value

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] dma: rc4030: limit interval timer reload value

[Cole Robinson]

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

Re: [Qemu-devel] [PATCH] dma: rc4030: limit interval timer reload value

[Peter Maydell]

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