[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 1/2] libxc: Don't write terminating NULL character to command string
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. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |