| Message ID | 1452032770-5642-2-git-send-email-boris.ostrovsky@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); > }
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); >> }
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
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.
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
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
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); }
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(-)