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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Ian Campbell <ian.campbell@citrix.com>,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>,
	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
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	[thread overview]
Message-ID: <568D4600.7000200@citrix.com> (raw)
In-Reply-To: <1452098680.21055.120.camel@citrix.com>

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