Xen devel - Sep 2007 - Unsigned bug in rdmsr_hypervisor_regs/wrmsr_hypervisor_regs

The code, below, in rdmsr_hypervisor_regs (in xen/arch/x86/traps.c) looks
wrong.  (The same code is in wrmsr_hypervisor_regs.)

int rdmsr_hypervisor_regs(
    uint32_t idx, uint32_t *eax, uint32_t *edx)
{
    idx -= 0x40000000;
    if ( idx > 0 )
        return 0;
...

The intent, apparently, is that the function should return zero if the
original idx exceeds 0x40000000.
However because idx is unsigned the function will always return zero
unlessthe original idx is precisely 0x40000000.

The effect is that reading or writing any unexpected MSR will cause the
guest to get a GPF.
(The intended effect is, I think, reading any MSR less than 0x40000001
should simply return 0
and writing any such MSR should bug.  Injecting a GPF should only happen
with MSRs greater than 0x40000000.)

Replacing the test with
   if ( (int32_t)idx > 0 )
 seems to cause problems of its own.

If this change is wrong, what is a proper fix?

-- rsp


-- 
--------------------------------------------------------------------
Robert S. Phillips                          Virtual Iron Software
rphillips@virtualiron.com                Tower 1, Floor 2
978-849-1220                                 900 Chelmsford Street
                                                    Lowell, MA 01851


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

The code is correct as it is. The function returns 1 if it supplies the
requested CPUID leaf, else 0. It only currently supplies leaf 0x40000000, so
it is correct to return 0 for all other index values.

 -- Keir

On 27/9/07 15:21, "Robert Phillips" <rsp.vi.xen@gmail.com>
wrote:
> The code, below, in rdmsr_hypervisor_regs (in xen/arch/x86/traps.c) looks
> wrong.  (The same code is in wrmsr_hypervisor_regs.)
> 
> int rdmsr_hypervisor_regs(
>     uint32_t idx, uint32_t *eax, uint32_t *edx)
> {
>     idx -= 0x40000000;
>     if ( idx > 0 )
>         return 0;
> ...
> 
> The intent, apparently, is that the function should return zero if the
> original idx exceeds 0x40000000.
> However because idx is unsigned the function will always return zero unless
> the original idx is precisely 0x40000000.
> 
> The effect is that reading or writing any unexpected MSR will cause the
guest
> to get a GPF.
> (The intended effect is, I think, reading any MSR less than 0x40000001
should
> simply return 0 
> and writing any such MSR should bug.  Injecting a GPF should only happen
with
> MSRs greater than 0x40000000.)
> 
> Replacing the test with
>    if ( (int32_t)idx > 0 )
>  seems to cause problems of its own.
> 
> If this change is wrong, what is a proper fix?
> 
> -- rsp
> 



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel