Re: [Xen-devel] [PATCH v2] x86/viridian: Add Partition Reference Time enlightenment

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.10.14 at 10:53, <chegger@xxxxxxxxx> wrote:
> On 10.10.14 13:55, Egger, Christoph wrote:
>> On 29.09.14 12:28, Paul Durrant wrote:
>> Reviewed-by: Christoph Egger <chegger@xxxxxxxxx>
> 
> I found one issue in update_reference_tsc(). See below.

As pointed out before.

>>> +    /*
>>> +     * The guest will calculate reference time according to the following
>>> +     * formula:
>>> +     *
>>> +     * ReferenceTime = ((RDTSC() * TscScale) >> 64) + TscOffset
>>> +     *
>>> +     * Windows uses a 100ns tick, so we need a scale which is cpu
>>> +     * ticks per 100ns shifted left by 64.
>>> +     */
>>> +    p->TscScale = ((10000ul << 32) / d->arch.tsc_khz) << 32;
>>> +
>>> +    do {
>>> +        p->TscSequence++;
>>> +    } while ( p->TscSequence == 0xFFFFFFFF ||
>>> +              p->TscSequence == 0 ); /* Avoid both 'invalid' values */
> 
> This is reading guest memory so a malicious guest can spin writing 0 and
> cause a DoS.
> 
> Better do here:
> 
>         p->TscSequence++;
>         if ( p->TscSequence == 0xFFFFFFFF || p->TscSequence == 0 )
>             p->TscSequence = 1;

        switch ( ++p->TscSequence )
        {
        case 0: case 0xffffffff:
                p->TscSequence = 1;
        }

And then again I would think it's wrong to store an "invalid" value
here even temporarily. I.e. we'd probably be better off computing
old value plus one into a local variable, skip the "invalid" values if
necessary, and only then store the new sequence number.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel