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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > > > > 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel