Mailing List Archive

# HG changeset patch
# User kfraser@localhost.localdomain
# Date 1188311516 -3600
# Node ID b1c3b9df7d9a7edf0f70f8869fa05b30e77ddbe0
# Parent 58d131f1fb35977ff2d8682f553391c8a866d52c
[xen, xencomm] fix various xencomm invalid racy access.
- Xencomm should check struct xencomm_desc alignment.
- Xencomm should check whether struct xencomm_desc itself (8 bytes)
doesn't cross page boundary. Otherwise a hostile guest kernel can
pass such a pointer that may across page boundary. Then xencomm
accesses an unrelated page.
- Xencomm shouldn't access struct xencomm_desc::nr_addrs multiple
times. Copy it to local area and use the copy.
Otherwise a hostile guest can modify at the same time.
- Xencomm should check whether struct xencomm_desc::address[] array
crosses page boundary. Otherwise xencomm may access unrelated pages.
- Xencomm should get_page()/put_page() after address conversion from
paddr to maddr because xen supports SMP and balloon driver.
Otherwise another vcpu may free the page at the same time.
Such a domain behaviour doesn't make sense, however nothing prevents
it.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
xen/common/xencomm.c | 328 ++++++++++++++++++++++++++++++++++++---------------
1 files changed, 234 insertions(+), 94 deletions(-)

diff -r 58d131f1fb35 -r b1c3b9df7d9a xen/common/xencomm.c
--- a/xen/common/xencomm.c Fri Aug 24 16:32:56 2007 +0100
+++ b/xen/common/xencomm.c Tue Aug 28 15:31:56 2007 +0100
@@ -34,9 +34,154 @@
#endif

static void*
-xencomm_maddr_to_vaddr(unsigned long maddr)
-{
- return maddr ? maddr_to_virt(maddr) : NULL;
+xencomm_vaddr(unsigned long paddr, struct page_info *page)
+{
+ return (void*)((paddr & ~PAGE_MASK) | (unsigned long)page_to_virt(page));
+}
+
+/* get_page() to prevent from another vcpu freeing the page */
+static int
+xencomm_get_page(unsigned long paddr, struct page_info **page)
+{
+ unsigned long maddr = paddr_to_maddr(paddr);
+ if ( maddr == 0 )
+ return -EFAULT;
+
+ *page = maddr_to_page(maddr);
+ if ( get_page(*page, current->domain) == 0 )
+ {
+ if ( page_get_owner(*page) != current->domain )
+ {
+ /*
+ * This page might be a page granted by another domain, or
+ * this page is freed with decrease reservation hypercall at
+ * the same time.
+ */
+ gdprintk(XENLOG_WARNING,
+ "bad page is passed. paddr 0x%lx maddr 0x%lx\n",
+ paddr, maddr);
+ return -EFAULT;
+ }
+
+ /* Try again. */
+ cpu_relax();
+ return -EAGAIN;
+ }
+
+ return 0;
+}
+
+/* check if struct desc doesn't cross page boundry */
+static int
+xencomm_desc_cross_page_boundary(unsigned long paddr)
+{
+ unsigned long offset = paddr & ~PAGE_MASK;
+ if ( offset > PAGE_SIZE - sizeof(struct xencomm_desc) )
+ return 1;
+ return 0;
+}
+
+struct xencomm_ctxt {
+ struct xencomm_desc *desc;
+
+ uint32_t nr_addrs;
+ struct page_info *page;
+};
+
+static uint32_t
+xencomm_ctxt_nr_addrs(const struct xencomm_ctxt *ctxt)
+{
+ return ctxt->nr_addrs;
+}
+
+static unsigned long*
+xencomm_ctxt_address(struct xencomm_ctxt *ctxt, int i)
+{
+ return &ctxt->desc->address[i];
+}
+
+/* check if struct xencomm_desc and address array cross page boundary */
+static int
+xencomm_ctxt_cross_page_boundary(struct xencomm_ctxt *ctxt)
+{
+ unsigned long saddr = (unsigned long)ctxt->desc;
+ unsigned long eaddr =
+ (unsigned long)&ctxt->desc->address[ctxt->nr_addrs] - 1;
+ if ( (saddr >> PAGE_SHIFT) != (eaddr >> PAGE_SHIFT) )
+ return 1;
+ return 0;
+}
+
+static int
+xencomm_ctxt_init(const void* handle, struct xencomm_ctxt *ctxt)
+{
+ struct page_info *page;
+ struct xencomm_desc *desc;
+ int ret;
+
+ /* avoid unaligned access */
+ if ( (unsigned long)handle % __alignof__(*desc) != 0 )
+ return -EINVAL;
+ if ( xencomm_desc_cross_page_boundary((unsigned long)handle) )
+ return -EINVAL;
+
+ /* first we need to access the descriptor */
+ ret = xencomm_get_page((unsigned long)handle, &page);
+ if ( ret )
+ return ret;
+
+ desc = xencomm_vaddr((unsigned long)handle, page);
+ if ( desc->magic != XENCOMM_MAGIC )
+ {
+ printk("%s: error: %p magic was 0x%x\n", __func__, desc, desc->magic);
+ put_page(page);
+ return -EINVAL;
+ }
+
+ ctxt->desc = desc;
+ ctxt->nr_addrs = desc->nr_addrs; /* copy before use.
+ * It is possible for a guest domain to
+ * modify concurrently.
+ */
+ ctxt->page = page;
+ if ( xencomm_ctxt_cross_page_boundary(ctxt) )
+ {
+ put_page(page);
+ return -EINVAL;
+ }
+ return 0;
+}
+
+static void
+xencomm_ctxt_done(struct xencomm_ctxt *ctxt)
+{
+ put_page(ctxt->page);
+}
+
+static int
+xencomm_copy_chunk_from(
+ unsigned long to, unsigned long paddr, unsigned int len)
+{
+ struct page_info *page;
+
+ while (1)
+ {
+ int res;
+ res = xencomm_get_page(paddr, &page);
+ if ( res != 0 )
+ {
+ if ( res == -EAGAIN )
+ continue; /* Try again. */
+ return res;
+ }
+ xc_dprintk("%lx[%d] -> %lx\n",
+ (unsigned long)xencomm_vaddr(paddr, page), len, to);
+
+ memcpy((void *)to, xencomm_vaddr(paddr, page), len);
+ put_page(page);
+ return 0;
+ }
+ /* NOTREACHED */
}

static unsigned long
@@ -48,14 +193,12 @@ xencomm_inline_from_guest(
while ( n > 0 )
{
unsigned int chunksz, bytes;
- unsigned long src_maddr;

chunksz = PAGE_SIZE - (src_paddr % PAGE_SIZE);
bytes = min(chunksz, n);

- src_maddr = paddr_to_maddr(src_paddr);
- xc_dprintk("%lx[%d] -> %lx\n", src_maddr, bytes, (unsigned long)to);
- memcpy(to, maddr_to_virt(src_maddr), bytes);
+ if ( xencomm_copy_chunk_from((unsigned long)to, src_paddr, bytes) )
+ return n;
src_paddr += bytes;
to += bytes;
n -= bytes;
@@ -81,7 +224,7 @@ xencomm_copy_from_guest(
xencomm_copy_from_guest(
void *to, const void *from, unsigned int n, unsigned int skip)
{
- struct xencomm_desc *desc;
+ struct xencomm_ctxt ctxt;
unsigned int from_pos = 0;
unsigned int to_pos = 0;
unsigned int i = 0;
@@ -89,26 +232,14 @@ xencomm_copy_from_guest(
if ( xencomm_is_inline(from) )
return xencomm_inline_from_guest(to, from, n, skip);

- /* First we need to access the descriptor. */
- desc = (struct xencomm_desc *)
- xencomm_maddr_to_vaddr(paddr_to_maddr((unsigned long)from));
- if ( desc == NULL )
+ if ( xencomm_ctxt_init(from, &ctxt) )
return n;

- if ( desc->magic != XENCOMM_MAGIC )
- {
- printk("%s: error: %p magic was 0x%x\n",
- __func__, desc, desc->magic);
- return n;
- }
-
- /* Iterate through the descriptor, copying up to a page at a time. */
- while ( (to_pos < n) && (i < desc->nr_addrs) )
- {
- unsigned long src_paddr = desc->address[i];
- unsigned int pgoffset;
- unsigned int chunksz;
- unsigned int chunk_skip;
+ /* Iterate through the descriptor, copying up to a page at a time */
+ while ( (to_pos < n) && (i < xencomm_ctxt_nr_addrs(&ctxt)) )
+ {
+ unsigned long src_paddr = *xencomm_ctxt_address(&ctxt, i);
+ unsigned int pgoffset, chunksz, chunk_skip;

if ( src_paddr == XENCOMM_INVALID )
{
@@ -124,18 +255,13 @@ xencomm_copy_from_guest(
chunksz -= chunk_skip;
skip -= chunk_skip;

- if ( (skip == 0) && (chunksz > 0) )
- {
- unsigned long src_maddr;
- unsigned long dest = (unsigned long)to + to_pos;
+ if ( skip == 0 && chunksz > 0 )
+ {
unsigned int bytes = min(chunksz, n - to_pos);

- src_maddr = paddr_to_maddr(src_paddr + chunk_skip);
- if ( src_maddr == 0 )
- return n - to_pos;
-
- xc_dprintk("%lx[%d] -> %lx\n", src_maddr, bytes, dest);
- memcpy((void *)dest, maddr_to_virt(src_maddr), bytes);
+ if ( xencomm_copy_chunk_from((unsigned long)to + to_pos,
+ src_paddr + chunk_skip, bytes) )
+ goto out;
from_pos += bytes;
to_pos += bytes;
}
@@ -143,7 +269,35 @@ xencomm_copy_from_guest(
i++;
}

+out:
+ xencomm_ctxt_done(&ctxt);
return n - to_pos;
+}
+
+static int
+xencomm_copy_chunk_to(
+ unsigned long paddr, unsigned long from, unsigned int len)
+{
+ struct page_info *page;
+
+ while (1)
+ {
+ int res;
+ res = xencomm_get_page(paddr, &page);
+ if ( res != 0 )
+ {
+ if ( res == -EAGAIN )
+ continue; /* Try again. */
+ return res;
+ }
+ xc_dprintk("%lx[%d] -> %lx\n", from, len,
+ (unsigned long)xencomm_vaddr(paddr, page));
+
+ memcpy(xencomm_vaddr(paddr, page), (void *)from, len);
+ put_page(page);
+ return 0;
+ }
+ /* NOTREACHED */
}

static unsigned long
@@ -155,14 +309,12 @@ xencomm_inline_to_guest(
while ( n > 0 )
{
unsigned int chunksz, bytes;
- unsigned long dest_maddr;

chunksz = PAGE_SIZE - (dest_paddr % PAGE_SIZE);
bytes = min(chunksz, n);

- dest_maddr = paddr_to_maddr(dest_paddr);
- xc_dprintk("%lx[%d] -> %lx\n", (unsigned long)from, bytes, dest_maddr);
- memcpy(maddr_to_virt(dest_maddr), (void *)from, bytes);
+ if ( xencomm_copy_chunk_to(dest_paddr, (unsigned long)from, bytes) )
+ return n;
dest_paddr += bytes;
from += bytes;
n -= bytes;
@@ -188,7 +340,7 @@ xencomm_copy_to_guest(
xencomm_copy_to_guest(
void *to, const void *from, unsigned int n, unsigned int skip)
{
- struct xencomm_desc *desc;
+ struct xencomm_ctxt ctxt;
unsigned int from_pos = 0;
unsigned int to_pos = 0;
unsigned int i = 0;
@@ -196,22 +348,13 @@ xencomm_copy_to_guest(
if ( xencomm_is_inline(to) )
return xencomm_inline_to_guest(to, from, n, skip);

- /* First we need to access the descriptor. */
- desc = (struct xencomm_desc *)
- xencomm_maddr_to_vaddr(paddr_to_maddr((unsigned long)to));
- if ( desc == NULL )
+ if ( xencomm_ctxt_init(to, &ctxt) )
return n;

- if ( desc->magic != XENCOMM_MAGIC )
- {
- printk("%s error: %p magic was 0x%x\n", __func__, desc, desc->magic);
- return n;
- }
-
- /* Iterate through the descriptor, copying up to a page at a time. */
- while ( (from_pos < n) && (i < desc->nr_addrs) )
- {
- unsigned long dest_paddr = desc->address[i];
+ /* Iterate through the descriptor, copying up to a page at a time */
+ while ( (from_pos < n) && (i < xencomm_ctxt_nr_addrs(&ctxt)) )
+ {
+ unsigned long dest_paddr = *xencomm_ctxt_address(&ctxt, i);
unsigned int pgoffset, chunksz, chunk_skip;

if ( dest_paddr == XENCOMM_INVALID )
@@ -228,18 +371,13 @@ xencomm_copy_to_guest(
chunksz -= chunk_skip;
skip -= chunk_skip;

- if ( (skip == 0) && (chunksz > 0) )
- {
- unsigned long dest_maddr;
- unsigned long source = (unsigned long)from + from_pos;
+ if ( skip == 0 && chunksz > 0 )
+ {
unsigned int bytes = min(chunksz, n - from_pos);

- dest_maddr = paddr_to_maddr(dest_paddr + chunk_skip);
- if ( dest_maddr == 0 )
- return n - from_pos;
-
- xc_dprintk("%lx[%d] -> %lx\n", source, bytes, dest_maddr);
- memcpy(maddr_to_virt(dest_maddr), (void *)source, bytes);
+ if ( xencomm_copy_chunk_to(dest_paddr + chunk_skip,
+ (unsigned long)from + from_pos, bytes) )
+ goto out;
from_pos += bytes;
to_pos += bytes;
}
@@ -247,6 +385,8 @@ xencomm_copy_to_guest(
i++;
}

+out:
+ xencomm_ctxt_done(&ctxt);
return n - from_pos;
}

@@ -260,29 +400,23 @@ static int xencomm_inline_add_offset(voi
* exhausted pages to XENCOMM_INVALID. */
int xencomm_add_offset(void **handle, unsigned int bytes)
{
- struct xencomm_desc *desc;
+ struct xencomm_ctxt ctxt;
int i = 0;

if ( xencomm_is_inline(*handle) )
return xencomm_inline_add_offset(handle, bytes);

- /* First we need to access the descriptor. */
- desc = (struct xencomm_desc *)
- xencomm_maddr_to_vaddr(paddr_to_maddr((unsigned long)*handle));
- if ( desc == NULL )
+ if ( xencomm_ctxt_init(handle, &ctxt) )
return -1;

- if ( desc->magic != XENCOMM_MAGIC )
- {
- printk("%s error: %p magic was 0x%x\n", __func__, desc, desc->magic);
- return -1;
- }
-
- /* Iterate through the descriptor incrementing addresses. */
- while ( (bytes > 0) && (i < desc->nr_addrs) )
- {
- unsigned long dest_paddr = desc->address[i];
- unsigned int pgoffset, chunksz, chunk_skip;
+ /* Iterate through the descriptor incrementing addresses */
+ while ( (bytes > 0) && (i < xencomm_ctxt_nr_addrs(&ctxt)) )
+ {
+ unsigned long *address = xencomm_ctxt_address(&ctxt, i);
+ unsigned long dest_paddr = *address;
+ unsigned int pgoffset;
+ unsigned int chunksz;
+ unsigned int chunk_skip;

if ( dest_paddr == XENCOMM_INVALID )
{
@@ -295,33 +429,39 @@ int xencomm_add_offset(void **handle, un

chunk_skip = min(chunksz, bytes);
if ( chunk_skip == chunksz )
- desc->address[i] = XENCOMM_INVALID; /* exchausted this page */
+ *address = XENCOMM_INVALID; /* exhausted this page */
else
- desc->address[i] += chunk_skip;
+ *address += chunk_skip;
bytes -= chunk_skip;

i++;
}
-
+ xencomm_ctxt_done(&ctxt);
return 0;
}

int xencomm_handle_is_null(void *handle)
{
- struct xencomm_desc *desc;
+ struct xencomm_ctxt ctxt;
int i;
+ int res = 1;

if ( xencomm_is_inline(handle) )
return xencomm_inline_addr(handle) == 0;

- desc = (struct xencomm_desc *)
- xencomm_maddr_to_vaddr(paddr_to_maddr((unsigned long)handle));
- if ( desc == NULL )
+ if ( xencomm_ctxt_init(handle, &ctxt) )
return 1;

- for ( i = 0; i < desc->nr_addrs; i++ )
- if ( desc->address[i] != XENCOMM_INVALID )
- return 0;
-
- return 1;
-}
+ for ( i = 0; i < xencomm_ctxt_nr_addrs(&ctxt); i++ )
+ {
+ if ( *xencomm_ctxt_address(&ctxt, i) != XENCOMM_INVALID )
+ {
+ res = 0;
+ goto out;
+ }
+ }
+
+out:
+ xencomm_ctxt_done(&ctxt);
+ return res;
+}

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