Re: [Qemu-devel] [PATCH v5 09/24] hpet: fix buffer overrun on invalid state load

[Michael S. Tsirkin]

On Fri, Apr 04, 2014 at 06:04:50PM +0300, Michael S. Tsirkin wrote:
> On Fri, Apr 04, 2014 at 05:47:39PM +0300, Michael S. Tsirkin wrote:
> > On Fri, Apr 04, 2014 at 11:51:52AM +0200, Juan Quintela wrote:
> > > "Michael S. Tsirkin" <address@hidden> wrote:
> > > > CVE-2013-4527 hw/timer/hpet.c buffer overrun
> > > >
> > > > hpet is a VARRAY with a uint8 size but static array of 32
> > > >
> > > > To fix, make sure num_timers is valid using VMSTATE_VALID hook.
> > > >
> > > > Reported-by: Anthony Liguori <address@hidden>
> > > > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > > > Reviewed-by: Dr. David Alan Gilbert <address@hidden>
> > > 
> > > 
> > > Ok, seing what you want to do with VMSTATE_VALIDATE.
> > 
> > It does not seem clean to add a callback that is only
> > be used with a single type.
> > 
> > > Much better solution is add a ->validate() field, and use it in the
> > > equivalent of LESS_EQUAL and rest of tests.
> > > 
> > > Will sent a patch.
> > > 
> > > Later, Juan.
> > 
> > I think it's better to stick to plain C, extending macros like
> > LESS_EQUAL is too tricky, vmstate is already full ugly one-off macros.
> 
> Sent too quick, phrase is cut off short
> 
> ... let's not add any more, let's use a generic macro like
> VMSTATE_VALIDATE.
> 

Hmm and I notice this still sounds rude and unclear.

Pls exuse the tone and the confusion, I wrote this in a hurry, I'm sorry
about that.

It's weekend here - I'll write a clearer response next week.


> > > > ---
> > > >  hw/timer/hpet.c | 13 +++++++++++++
> > > >  1 file changed, 13 insertions(+)
> > > >
> > > > diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> > > > index e15d6bc..2792f89 100644
> > > > --- a/hw/timer/hpet.c
> > > > +++ b/hw/timer/hpet.c
> > > > @@ -239,6 +239,18 @@ static int hpet_pre_load(void *opaque)
> > > >      return 0;
> > > >  }
> > > >  
> > > > +static bool hpet_validate_num_timers(void *opaque, int version_id)
> > > > +{
> > > > +    HPETState *s = opaque;
> > > > +
> > > > +    if (s->num_timers < HPET_MIN_TIMERS) {
> > > > +        return false;
> > > > +    } else if (s->num_timers > HPET_MAX_TIMERS) {
> > > > +        return false;
> > > > +    }
> > > > +    return true;
> > > > +}
> > > > +
> > > >  static int hpet_post_load(void *opaque, int version_id)
> > > >  {
> > > >      HPETState *s = opaque;
> > > > @@ -307,6 +319,7 @@ static const VMStateDescription vmstate_hpet = {
> > > >          VMSTATE_UINT64(isr, HPETState),
> > > >          VMSTATE_UINT64(hpet_counter, HPETState),
> > > >          VMSTATE_UINT8_V(num_timers, HPETState, 2),
> > > > +        VMSTATE_VALIDATE("num_timers in range", 
> > > > hpet_validate_num_timers),
> > > >          VMSTATE_STRUCT_VARRAY_UINT8(timer, HPETState, num_timers, 0,
> > > >                                      vmstate_hpet_timer, HPETTimer),
> > > >          VMSTATE_END_OF_LIST()