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

[Ian Campbell <ian.campbell@citrix.com>]

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