Re: [Xen-ia64-devel] [PATCH] Fix vulnerability of copy_to_user in PAL emulation

[Alex Williamson <alex.williamson@xxxxxx>]

On Tue, 2007-12-11 at 16:11 +0900, Kouya Shimura wrote:
> There is a security vulnerability in PAL emulation
> since alt-dtlb miss handler of HVM absolutely 
> inserts a identity-mapped TLB when psr.vm=0.
> 
> HVM guest can access an arbitrary machine physical
> memory with this security hole.
> 
> Actually windows 2008 destroys the content of machine 
> physical address 0x108000. This is a serious problem.
> 
> I tried to support PV domain with the same logic too
> but it doesn't work well since PV domain can't hold
> more than one vtlb.

   I think there's a off by one issue as noted below.  Also a few
cleanups.  Like Tristan, I'd rather see a xencomm for GFW, but that
might be too intrusive for 3.2 at this point.  So this seems like a
reasonable temporary fix.  Thanks,

        Alex


> diff -r 4054cd60895b xen/arch/ia64/vmx/vmx_fault.c
> --- a/xen/arch/ia64/vmx/vmx_fault.c   Mon Dec 10 13:49:22 2007 +0000
> +++ b/xen/arch/ia64/vmx/vmx_fault.c   Mon Dec 10 18:39:23 2007 +0900
> @@ -174,6 +174,7 @@ vmx_ia64_handle_break (unsigned long ifa
>  {
>      struct domain *d = current->domain;
>      struct vcpu *v = current;
> +    IA64FAULT fault;
>  
>      perfc_incr(vmx_ia64_handle_break);
>  #ifdef CRASH_DEBUG
> @@ -196,9 +197,9 @@ vmx_ia64_handle_break (unsigned long ifa
>                  return IA64_NO_FAULT;
>              }
>              else if (iim == DOMN_PAL_REQUEST) {
> -                pal_emul(v);
> -                vcpu_increment_iip(v);
> -                return IA64_NO_FAULT;
> +                if ((fault = pal_emul(v)) == IA64_NO_FAULT)

Please split this up:

fault = pal_emul(v);
if (fault == IA64_NO_FAULT)
    ...

> +                    vcpu_increment_iip(v);
> +                return fault;
>              } else if (iim == DOMN_SAL_REQUEST) {
>                  sal_emul(v);
>                  vcpu_increment_iip(v);
> diff -r 4054cd60895b xen/arch/ia64/xen/fw_emul.c
> --- a/xen/arch/ia64/xen/fw_emul.c     Mon Dec 10 13:49:22 2007 +0000
> +++ b/xen/arch/ia64/xen/fw_emul.c     Tue Dec 11 15:00:36 2007 +0900
...
> +static IA64FAULT
> +palcomm_init(struct palcomm_ctxt *comm, unsigned long buf, long size)
> +{
> +     IA64FAULT fault = IA64_NO_FAULT;
> +     int i;
> +     unsigned long paddr, maddr, poff;
> +     struct page_info* page;
> +
> +     BUG_ON((unsigned)size > MIN_PAGE_SIZE);
> +
> +     /* check for vulnerability. NB - go through in metaphysical mode. */
> +     if (IS_VMM_ADDRESS(buf) || IS_VMM_ADDRESS(buf + size))

Should the end address be (buf + size - 1)?  Seems this could
incorrectly trigger on the next page if the buffer has the right
alignment.

> +             panic_domain(NULL, "copy to bad address:0x%lx\n", buf);
> +
> +     comm->va[0] = comm->va[1] = NULL;
> +
> +     /* XXX: not implemented for PV domain */
> +     if (!VMX_DOMAIN(current))
> +             return fault;
> +
> +     for (i = 0; i < 2; i++) {
> +             if (is_virtual_mode(current)) {
> +                     fault = vmx_vcpu_tpa(current, buf, &paddr);
> +                     if (fault != IA64_NO_FAULT)
> +                             goto fail;
> +             } else
> +                     paddr = buf;
> +             if ((maddr = paddr_to_maddr(paddr)) == 0)

Split up:

maddr = paddr_to_maddr(paddr);
if (maddr == 0)
        ...


> @@ -801,21 +896,22 @@ xen_pal_emulator(unsigned long index, u6
>           case PAL_PERF_MON_INFO:
>               {
>                       unsigned long pm_buffer[16];
> +                     struct palcomm_ctxt comm;
> +                     IA64FAULT fault;
> +
> +                     fault = palcomm_init(&comm, in1, 128);

s/128/sizeof(pm_buffer)/

> +                     if (fault != IA64_NO_FAULT)
> +                             return fault;
> +
>                       status = ia64_pal_perf_mon_info(
>                                       pm_buffer,
>                                       (pal_perf_mon_info_u_t *) &r9);
> -                     if (status != 0) {
> -                             while(1)
> -                             printk("PAL_PERF_MON_INFO fails ret=%ld\n", 
> status);
> -                             break;
> +                     if (status == PAL_STATUS_SUCCESS) {
> +                             if (palcomm_copy_to_guest(&comm, in1,
> +                                                       pm_buffer, 128))

same

> @@ -885,9 +988,19 @@ xen_pal_emulator(unsigned long index, u6
>           case PAL_BRAND_INFO:
>               if (in1 == 0) {
>                       char brand_info[128];
> +                     struct palcomm_ctxt comm;
> +                     IA64FAULT fault;
> +
> +                     fault = palcomm_init(&comm, in2, 128);

s/128/sizeof(brand_info)/

> +                     if (fault != IA64_NO_FAULT)
> +                             return fault;
>                       status = ia64_pal_get_brand_info(brand_info);
> -                     if (status == PAL_STATUS_SUCCESS)
> -                             copy_to_user((void *)in2, brand_info, 128);
> +                     if (status == PAL_STATUS_SUCCESS) {
> +                             if (palcomm_copy_to_guest(&comm, in2,
> +                                                       brand_info, 128))

same


-- 
Alex Williamson                             HP Open Source & Linux Org.


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