[PATCH v2 0/2] HVMlite start_info initialization fixes

[PATCH v2 0/2] HVMlite start_info initialization fixes

[Boris Ostrovsky]

In addition to moving certain things to a later point I also noticed a small
bug with comamnd line initialization which is fixed in patch 1.

I ended up leaving code that deals with special pages and indenity page
tables in alloc_magic_pages_hvm().

Boris Ostrovsky (2):
  libxc: Don't write terminating NULL character to command string
  libxc: Defer initialization of start_page for HVM guests

 tools/libxc/xc_dom_x86.c |  123 +++++++++++++++++++++++++++-------------------
 1 files changed, 73 insertions(+), 50 deletions(-)

[PATCH v2 1/2] libxc: Don't write terminating NULL character to command string

[Boris Ostrovsky]

When copying boot command string for HVMlite guests we explicitly write
'\0' at MAX_GUEST_CMDLINE offset. Unless the string is close to
MAX_GUEST_CMDLINE in length this write will end up in the wrong place,
beyond the end of the mapped range.

Instead we should test string's length early and error out if it is too
long.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 tools/libxc/xc_dom_x86.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index 3960875..b696149 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -647,6 +647,11 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
         if ( dom->cmdline )
         {
             cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8);
+            if ( cmdline_size > MAX_GUEST_CMDLINE )
+            {
+                DOMPRINTF("Boot command line is too long");
+                goto error_out;
+            }
             start_info_size += cmdline_size;
 
         }
@@ -676,8 +681,7 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
 
         if ( dom->cmdline )
         {
-            strncpy(cmdline, dom->cmdline, MAX_GUEST_CMDLINE);
-            cmdline[MAX_GUEST_CMDLINE - 1] = '\0';
+            strcpy(cmdline, dom->cmdline);
             start_info->cmdline_paddr = (seg.pfn << PAGE_SHIFT) +
                                 ((uintptr_t)cmdline - (uintptr_t)start_info);
         }
-- 
1.7.1

Re: [PATCH v2 1/2] libxc: Don't write terminating NULL character to command string

[Andrew Cooper]

On 05/01/2016 22:26, Boris Ostrovsky wrote:
> When copying boot command string for HVMlite guests we explicitly write
> '\0' at MAX_GUEST_CMDLINE offset. Unless the string is close to
> MAX_GUEST_CMDLINE in length this write will end up in the wrong place,
> beyond the end of the mapped range.
>
> Instead we should test string's length early and error out if it is too
> long.
>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

MAX_GUEST_CMDLINE is an arbitrary and incorrect restriction.  It is
sadly baked into the PV ABI, but I specifically want to avoid lumbering
DMLite with the failings of PV.

By the looks of it, the only bug is the use of MAX_GUEST_CMDLINE.  The
xc_map_foreign_range() call already accounts for sufficient space to
store the string when mapping guest memory.

I think you only need the 2nd hunk of this patch.

~Andrew

> ---
>  tools/libxc/xc_dom_x86.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index 3960875..b696149 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -647,6 +647,11 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>          if ( dom->cmdline )
>          {
>              cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8);
> +            if ( cmdline_size > MAX_GUEST_CMDLINE )
> +            {
> +                DOMPRINTF("Boot command line is too long");
> +                goto error_out;
> +            }
>              start_info_size += cmdline_size;
>  
>          }
> @@ -676,8 +681,7 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>  
>          if ( dom->cmdline )
>          {
> -            strncpy(cmdline, dom->cmdline, MAX_GUEST_CMDLINE);
> -            cmdline[MAX_GUEST_CMDLINE - 1] = '\0';
> +            strcpy(cmdline, dom->cmdline);
>              start_info->cmdline_paddr = (seg.pfn << PAGE_SHIFT) +
>                                  ((uintptr_t)cmdline - (uintptr_t)start_info);
>          }

Re: [PATCH v2 1/2] libxc: Don't write terminating NULL character to command string

[Boris Ostrovsky]

On 01/05/2016 05:42 PM, Andrew Cooper wrote:
> On 05/01/2016 22:26, Boris Ostrovsky wrote:
>> When copying boot command string for HVMlite guests we explicitly write
>> '\0' at MAX_GUEST_CMDLINE offset. Unless the string is close to
>> MAX_GUEST_CMDLINE in length this write will end up in the wrong place,
>> beyond the end of the mapped range.
>>
>> Instead we should test string's length early and error out if it is too
>> long.
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> MAX_GUEST_CMDLINE is an arbitrary and incorrect restriction.  It is
> sadly baked into the PV ABI, but I specifically want to avoid lumbering
> DMLite with the failings of PV.
>
> By the looks of it, the only bug is the use of MAX_GUEST_CMDLINE.  The
> xc_map_foreign_range() call already accounts for sufficient space to
> store the string when mapping guest memory.

Yes, I was also thinking about dropping it but ended up keeping it 
mostly because it didn't feel right to blindly use strcpy().

-boris


>
> I think you only need the 2nd hunk of this patch.
>
> ~Andrew
>
>> ---
>>   tools/libxc/xc_dom_x86.c |    8 ++++++--
>>   1 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
>> index 3960875..b696149 100644
>> --- a/tools/libxc/xc_dom_x86.c
>> +++ b/tools/libxc/xc_dom_x86.c
>> @@ -647,6 +647,11 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>>           if ( dom->cmdline )
>>           {
>>               cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8);
>> +            if ( cmdline_size > MAX_GUEST_CMDLINE )
>> +            {
>> +                DOMPRINTF("Boot command line is too long");
>> +                goto error_out;
>> +            }
>>               start_info_size += cmdline_size;
>>   
>>           }
>> @@ -676,8 +681,7 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>>   
>>           if ( dom->cmdline )
>>           {
>> -            strncpy(cmdline, dom->cmdline, MAX_GUEST_CMDLINE);
>> -            cmdline[MAX_GUEST_CMDLINE - 1] = '\0';
>> +            strcpy(cmdline, dom->cmdline);
>>               start_info->cmdline_paddr = (seg.pfn << PAGE_SHIFT) +
>>                                   ((uintptr_t)cmdline - (uintptr_t)start_info);
>>           }

Re: [PATCH v2 1/2] libxc: Don't write terminating NULL character to command string

[Andrew Cooper]

On 05/01/2016 22:59, Boris Ostrovsky wrote:
> On 01/05/2016 05:42 PM, Andrew Cooper wrote:
>> On 05/01/2016 22:26, Boris Ostrovsky wrote:
>>> When copying boot command string for HVMlite guests we explicitly write
>>> '\0' at MAX_GUEST_CMDLINE offset. Unless the string is close to
>>> MAX_GUEST_CMDLINE in length this write will end up in the wrong place,
>>> beyond the end of the mapped range.
>>>
>>> Instead we should test string's length early and error out if it is too
>>> long.
>>>
>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> MAX_GUEST_CMDLINE is an arbitrary and incorrect restriction.  It is
>> sadly baked into the PV ABI, but I specifically want to avoid lumbering
>> DMLite with the failings of PV.
>>
>> By the looks of it, the only bug is the use of MAX_GUEST_CMDLINE.  The
>> xc_map_foreign_range() call already accounts for sufficient space to
>> store the string when mapping guest memory.
>
> Yes, I was also thinking about dropping it but ended up keeping it
> mostly because it didn't feel right to blindly use strcpy().

Possibly add a comment explaining that the length has already been
checked, and that sufficient space has been allocated, if that helps? 
One way or another, the use of strcpy() here is correct.

~Andrew

Re: [PATCH v2 1/2] libxc: Don't write terminating NULL character to command string

[Ian Campbell]

On Tue, 2016-01-05 at 23:01 +0000, Andrew Cooper wrote:
> On 05/01/2016 22:59, Boris Ostrovsky wrote:
> > On 01/05/2016 05:42 PM, Andrew Cooper wrote:
> > > On 05/01/2016 22:26, Boris Ostrovsky wrote:
> > > > When copying boot command string for HVMlite guests we explicitly
> > > > write
> > > > '\0' at MAX_GUEST_CMDLINE offset. Unless the string is close to
> > > > MAX_GUEST_CMDLINE in length this write will end up in the wrong
> > > > place,
> > > > beyond the end of the mapped range.
> > > > 
> > > > Instead we should test string's length early and error out if it is
> > > > too
> > > > long.
> > > > 
> > > > Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > > MAX_GUEST_CMDLINE is an arbitrary and incorrect restriction.  It is
> > > sadly baked into the PV ABI, but I specifically want to avoid
> > > lumbering
> > > DMLite with the failings of PV.
> > > 
> > > By the looks of it, the only bug is the use of
> > > MAX_GUEST_CMDLINE.  The
> > > xc_map_foreign_range() call already accounts for sufficient space to
> > > store the string when mapping guest memory.
> > 
> > Yes, I was also thinking about dropping it but ended up keeping it
> > mostly because it didn't feel right to blindly use strcpy().
> 
> Possibly add a comment explaining that the length has already been
> checked, and that sufficient space has been allocated, if that helps? 
> One way or another, the use of strcpy() here is correct.

The code has cmdline_size in hand still, so using strncpy with that might
make people feel better and also serve as commentary regarding the sizing.

This code wants a check that the whole start_info + cmdline + modlist
doesn't run off the end of whatever guest mapping has been made.

In fact it is only mapping sizeof(*start_info) and relying on that being
rounded up to a page, which seems very wrong (e.g. guest admin controlled
cmdline, this would be a security issue if dmlite weren't still
experimental), it should do the foreign mapping after figuring out where
modlist ends and make sure it maps enough.

Ian.

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

Re: [PATCH v2 1/2] libxc: Don't write terminating NULL character to command string

[Andrew Cooper]

On 06/01/16 16:44, Ian Campbell wrote:
> On Tue, 2016-01-05 at 23:01 +0000, Andrew Cooper wrote:
>> On 05/01/2016 22:59, Boris Ostrovsky wrote:
>>> On 01/05/2016 05:42 PM, Andrew Cooper wrote:
>>>> On 05/01/2016 22:26, Boris Ostrovsky wrote:
>>>>> When copying boot command string for HVMlite guests we explicitly
>>>>> write
>>>>> '\0' at MAX_GUEST_CMDLINE offset. Unless the string is close to
>>>>> MAX_GUEST_CMDLINE in length this write will end up in the wrong
>>>>> place,
>>>>> beyond the end of the mapped range.
>>>>>
>>>>> Instead we should test string's length early and error out if it is
>>>>> too
>>>>> long.
>>>>>
>>>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>> MAX_GUEST_CMDLINE is an arbitrary and incorrect restriction.  It is
>>>> sadly baked into the PV ABI, but I specifically want to avoid
>>>> lumbering
>>>> DMLite with the failings of PV.
>>>>
>>>> By the looks of it, the only bug is the use of
>>>> MAX_GUEST_CMDLINE.  The
>>>> xc_map_foreign_range() call already accounts for sufficient space to
>>>> store the string when mapping guest memory.
>>> Yes, I was also thinking about dropping it but ended up keeping it
>>> mostly because it didn't feel right to blindly use strcpy().
>> Possibly add a comment explaining that the length has already been
>> checked, and that sufficient space has been allocated, if that helps? 
>> One way or another, the use of strcpy() here is correct.
> The code has cmdline_size in hand still, so using strncpy with that might
> make people feel better and also serve as commentary regarding the sizing.

Can do.

>
> This code wants a check that the whole start_info + cmdline + modlist
> doesn't run off the end of whatever guest mapping has been made.
>
> In fact it is only mapping sizeof(*start_info) and relying on that being
> rounded up to a page, which seems very wrong (e.g. guest admin controlled
> cmdline, this would be a security issue if dmlite weren't still
> experimental), it should do the foreign mapping after figuring out where
> modlist ends and make sure it maps enough.

The mapping is start_info_size which includes cmdline_size and a single
modlist entry.

There is no risk (that I can see) of the command line wandering over the
mapping boundary.

~Andrew

Re: [PATCH v2 1/2] libxc: Don't write terminating NULL character to command string

[Ian Campbell]

On Wed, 2016-01-06 at 16:51 +0000, Andrew Cooper wrote:
> On 06/01/16 16:44, Ian Campbell wrote:
> > On Tue, 2016-01-05 at 23:01 +0000, Andrew Cooper wrote:
> > > On 05/01/2016 22:59, Boris Ostrovsky wrote:
> > > > On 01/05/2016 05:42 PM, Andrew Cooper wrote:
> > > > > On 05/01/2016 22:26, Boris Ostrovsky wrote:
> > > > > > When copying boot command string for HVMlite guests we
> > > > > > explicitly
> > > > > > write
> > > > > > '\0' at MAX_GUEST_CMDLINE offset. Unless the string is close to
> > > > > > MAX_GUEST_CMDLINE in length this write will end up in the wrong
> > > > > > place,
> > > > > > beyond the end of the mapped range.
> > > > > > 
> > > > > > Instead we should test string's length early and error out if
> > > > > > it is
> > > > > > too
> > > > > > long.
> > > > > > 
> > > > > > Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > > > > MAX_GUEST_CMDLINE is an arbitrary and incorrect restriction.  It
> > > > > is
> > > > > sadly baked into the PV ABI, but I specifically want to avoid
> > > > > lumbering
> > > > > DMLite with the failings of PV.
> > > > > 
> > > > > By the looks of it, the only bug is the use of
> > > > > MAX_GUEST_CMDLINE.  The
> > > > > xc_map_foreign_range() call already accounts for sufficient space
> > > > > to
> > > > > store the string when mapping guest memory.
> > > > Yes, I was also thinking about dropping it but ended up keeping it
> > > > mostly because it didn't feel right to blindly use strcpy().
> > > Possibly add a comment explaining that the length has already been
> > > checked, and that sufficient space has been allocated, if that helps?
> > > One way or another, the use of strcpy() here is correct.
> > The code has cmdline_size in hand still, so using strncpy with that
> > might
> > make people feel better and also serve as commentary regarding the
> > sizing.
> 
> Can do.
> 
> > 
> > This code wants a check that the whole start_info + cmdline + modlist
> > doesn't run off the end of whatever guest mapping has been made.
> > 
> > In fact it is only mapping sizeof(*start_info) and relying on that
> > being
> > rounded up to a page, which seems very wrong (e.g. guest admin
> > controlled
> > cmdline, this would be a security issue if dmlite weren't still
> > experimental), it should do the foreign mapping after figuring out
> > where
> > modlist ends and make sure it maps enough.
> 
> The mapping is start_info_size which includes cmdline_size and a single
> modlist entry.
> 
> There is no risk (that I can see) of the command line wandering over the
> mapping boundary.

I confused the bit which calculates the actual pointers with the bit which
calculated the total size, sorry for the noise.

Ian


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

[PATCH v2 2/2] libxc: Defer initialization of start_page for HVM guests

[Boris Ostrovsky]

With commit 8c45adec18e0 ("libxc: create unmapped initrd in domain
builder if supported") location of ramdisk may not be available to
HVMlite guests by the time alloc_magic_pages_hvm() is invoked if the
guest supports unmapped initrd.

So let's move ramdisk info initialization (along with a few other
operations that are not directly related to allocating magic/special
pages) from alloc_magic_pages_hvm() to bootlate_hvm().

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 tools/libxc/xc_dom_x86.c |  117 ++++++++++++++++++++++++++-------------------
 1 files changed, 68 insertions(+), 49 deletions(-)

diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index b696149..e8c947f 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -586,23 +586,12 @@ static void build_hvm_info(void *hvm_info_page, struct xc_dom_image *dom)
 static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
 {
     unsigned long i;
-    void *hvm_info_page;
     uint32_t *ident_pt, domid = dom->guest_domid;
     int rc;
     xen_pfn_t special_array[X86_HVM_NR_SPECIAL_PAGES];
     xen_pfn_t ioreq_server_array[NR_IOREQ_SERVER_PAGES];
     xc_interface *xch = dom->xch;
 
-    if ( dom->device_model )
-    {
-        if ( (hvm_info_page = xc_map_foreign_range(
-                  xch, domid, PAGE_SIZE, PROT_READ | PROT_WRITE,
-                  HVM_INFO_PFN)) == NULL )
-            goto error_out;
-        build_hvm_info(hvm_info_page, dom);
-        munmap(hvm_info_page, PAGE_SIZE);
-    }
-
     /* Allocate and clear special pages. */
     for ( i = 0; i < X86_HVM_NR_SPECIAL_PAGES; i++ )
         special_array[i] = special_pfn(i);
@@ -637,12 +626,9 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
     if ( !dom->device_model )
     {
         struct xc_dom_seg seg;
-        struct hvm_start_info *start_info;
-        char *cmdline;
         struct hvm_modlist_entry *modlist;
-        void *start_page;
         size_t cmdline_size = 0;
-        size_t start_info_size = sizeof(*start_info);
+        size_t start_info_size = sizeof(struct hvm_start_info);
 
         if ( dom->cmdline )
         {
@@ -666,39 +652,6 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
             goto out;
         }
 
-        start_page = xc_map_foreign_range(xch, domid, start_info_size,
-                                          PROT_READ | PROT_WRITE,
-                                          seg.pfn);
-        if ( start_page == NULL )
-        {
-            DOMPRINTF("Unable to map HVM start info page");
-            goto error_out;
-        }
-
-        start_info = start_page;
-        cmdline = start_page + sizeof(*start_info);
-        modlist = start_page + sizeof(*start_info) + cmdline_size;
-
-        if ( dom->cmdline )
-        {
-            strcpy(cmdline, dom->cmdline);
-            start_info->cmdline_paddr = (seg.pfn << PAGE_SHIFT) +
-                                ((uintptr_t)cmdline - (uintptr_t)start_info);
-        }
-
-        if ( dom->ramdisk_blob )
-        {
-            modlist[0].paddr = dom->ramdisk_seg.vstart - dom->parms.virt_base;
-            modlist[0].size = dom->ramdisk_seg.vend - dom->ramdisk_seg.vstart;
-            start_info->modlist_paddr = (seg.pfn << PAGE_SHIFT) +
-                                ((uintptr_t)modlist - (uintptr_t)start_info);
-            start_info->nr_modules = 1;
-        }
-
-        start_info->magic = HVM_START_MAGIC_VALUE;
-
-        munmap(start_page, start_info_size);
-
         dom->start_info_pfn = seg.pfn;
     }
     else
@@ -1788,7 +1741,73 @@ static int alloc_pgtables_hvm(struct xc_dom_image *dom)
 
 static int bootlate_hvm(struct xc_dom_image *dom)
 {
-    DOMPRINTF("%s: doing nothing", __func__);
+    uint32_t domid = dom->guest_domid;
+    xc_interface *xch = dom->xch;
+
+    if ( !dom->device_model )
+    {
+        struct hvm_start_info *start_info;
+        size_t start_info_size = sizeof(*start_info);
+        void *start_page;
+        struct hvm_modlist_entry *modlist;
+        size_t cmdline_size = 0;
+
+        if ( dom->cmdline )
+        {
+            cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8);
+            start_info_size += cmdline_size;
+        }
+        if ( dom->ramdisk_blob )
+            start_info_size += sizeof(*modlist); /* Limited to one module. */
+
+        start_page = xc_map_foreign_range(xch, domid, start_info_size,
+                                          PROT_READ | PROT_WRITE,
+                                          dom->start_info_pfn);
+        if ( start_page == NULL )
+        {
+            DOMPRINTF("Unable to map HVM start info page");
+            return -1;
+        }
+
+        start_info = start_page;
+
+        if ( dom->cmdline )
+        {
+            char *cmdline = start_page + sizeof(*start_info);
+
+            /* We've already tested cmdline's size in alloc_magic_pages_hvm() */
+            strcpy(cmdline, dom->cmdline);
+            start_info->cmdline_paddr = (dom->start_info_pfn << PAGE_SHIFT) +
+                                ((uintptr_t)cmdline - (uintptr_t)start_info);
+        }
+
+        if ( dom->ramdisk_blob )
+        {
+            modlist = start_page + sizeof(*start_info) + cmdline_size;
+
+            modlist[0].paddr = dom->ramdisk_seg.vstart - dom->parms.virt_base;
+            modlist[0].size = dom->ramdisk_seg.vend - dom->ramdisk_seg.vstart;
+            start_info->modlist_paddr = (dom->start_info_pfn << PAGE_SHIFT) +
+                                ((uintptr_t)modlist - (uintptr_t)start_info);
+            start_info->nr_modules = 1;
+        }
+
+        start_info->magic = HVM_START_MAGIC_VALUE;
+
+        munmap(start_page, start_info_size);
+    }
+    else
+    {
+        void *hvm_info_page;
+
+        if ( (hvm_info_page = xc_map_foreign_range(
+                  xch, domid, PAGE_SIZE, PROT_READ | PROT_WRITE,
+                  HVM_INFO_PFN)) == NULL )
+            return -1;
+        build_hvm_info(hvm_info_page, dom);
+        munmap(hvm_info_page, PAGE_SIZE);
+    }
+
     return 0;
 }
 
-- 
1.7.1

Re: [PATCH v2 2/2] libxc: Defer initialization of start_page for HVM guests

[Wei Liu]

On Tue, Jan 05, 2016 at 05:26:10PM -0500, Boris Ostrovsky wrote:
> With commit 8c45adec18e0 ("libxc: create unmapped initrd in domain
> builder if supported") location of ramdisk may not be available to
> HVMlite guests by the time alloc_magic_pages_hvm() is invoked if the
> guest supports unmapped initrd.
> 
> So let's move ramdisk info initialization (along with a few other
> operations that are not directly related to allocating magic/special
> pages) from alloc_magic_pages_hvm() to bootlate_hvm().
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>