From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v2 1/2] libxc: Don't write terminating NULL character to command string Date: Wed, 6 Jan 2016 16:51:12 +0000 Message-ID: <568D4600.7000200@citrix.com> References: <1452032770-5642-1-git-send-email-boris.ostrovsky@oracle.com> <1452032770-5642-2-git-send-email-boris.ostrovsky@oracle.com> <568C46B8.6020204@citrix.com> <568C4AE8.1050302@oracle.com> <568C4B5E.7080900@citrix.com> <1452098680.21055.120.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1452098680.21055.120.camel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell , Boris Ostrovsky , ian.jackson@eu.citrix.com, stefano.stabellini@eu.citrix.com, wei.liu2@citrix.com Cc: jgross@suse.com, xen-devel@lists.xen.org, roger.pau@citrix.com List-Id: xen-devel@lists.xenproject.org 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 >>>> 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