WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-ia64-devel

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

To: Kouya Shimura <kouya@xxxxxxxxxxxxxx>
Subject: Re: [Xen-ia64-devel] [PATCH] Fix vulnerability of copy_to_user in PAL emulation
From: Alex Williamson <alex.williamson@xxxxxx>
Date: Tue, 11 Dec 2007 09:41:09 -0700
Cc: xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
Delivery-date: Tue, 11 Dec 2007 08:41:45 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <7k8x41puy7.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-ia64-devel-request@lists.xensource.com?subject=help>
List-id: Discussion of the ia64 port of Xen <xen-ia64-devel.lists.xensource.com>
List-post: <mailto:xen-ia64-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-ia64-devel>, <mailto:xen-ia64-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-ia64-devel>, <mailto:xen-ia64-devel-request@lists.xensource.com?subject=unsubscribe>
Organization: OSLO R&D
References: <7k8x41puy7.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-ia64-devel-bounces@xxxxxxxxxxxxxxxxxxx
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