[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 2/4] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors
On 21/04/17 10:46, Jan Beulich wrote: >>>> On 21.04.17 at 11:11, <andrew.cooper3@xxxxxxxxxx> wrote: >> On 21/04/2017 09:54, Andrew Cooper wrote: >>> On 21/04/2017 08:27, Jan Beulich wrote: >>>>>>> On 20.04.17 at 19:59, <jennifer.herbert@xxxxxxxxxx> wrote: >>>>> From: Jennifer Herbert <Jennifer.Herbert@xxxxxxxxxx> >>>> Is this correct, considering that iirc the patch was new in v5 and ... >>>> >>>>> This also allows the usual cases to be simplified, by omitting an >>>>> unnecessary >>>>> buf parameters, and because the macros can appropriately size the object. >>>>> >>>>> This makes copying to or from a buf that isn't big enough an error. >>>>> If the buffer isnt big enough, trying to carry on regardless >>>>> can only cause trouble later on. >>>>> >>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> >>>>> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@xxxxxxxxxx> >>>> ... this sequence of S-o-b-s? >>>> >>>>> --- a/xen/arch/x86/hvm/dm.c >>>>> +++ b/xen/arch/x86/hvm/dm.c >>>>> @@ -32,36 +32,47 @@ struct dmop_args { >>>>> struct xen_dm_op_buf buf[2]; >>>>> }; >>>>> >>>>> -static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[], >>>>> - unsigned int nr_bufs, void *dst, >>>>> - unsigned int idx, size_t dst_size) >>>>> +static bool _raw_copy_from_guest_buf(void *dst, >>>>> + const struct dmop_args *args, >>>>> + unsigned int buf_idx, >>>>> + size_t dst_bytes) >>>>> { >>>>> - size_t size; >>>>> + size_t buf_bytes; >>>>> >>>>> - if ( idx >= nr_bufs ) >>>>> + if ( buf_idx >= args->nr_bufs ) >>>>> return false; >>>>> >>>>> - memset(dst, 0, dst_size); >>>>> + buf_bytes = args->buf[buf_idx].size; >>>>> >>>>> - size = min_t(size_t, dst_size, bufs[idx].size); >>>>> + if ( dst_bytes > buf_bytes ) >>>>> + return false; >>>> While this behavioral change is now being mentioned in the >>>> description, I'm not sure I buy the argument of basically being >>>> guaranteed to cause trouble down the road. Did you consider the >>>> forward compatibility aspect here, allowing us to extend interface >>>> structures by adding fields to their ends without breaking old >>>> callers? Paul, what are your thoughts here? >>> DMOP is a stable ABI. There is no legal extending of any objects. >>> >>> The previous semantics are guaranteed to break the ABI with future >>> subops, which is why I removed it. In the case that the guest >>> originally passed an overly long buffer, and someone tried be "clever" >>> here passing the same object here expecting _raw_copy_from_guest_buf() >>> to DTRT, the function will end up copying too much data from the guest, >>> and you will end up with something the guest wasn't intending to be part >>> of the structure replacing the zero extension. >>> >>> New subops which take a longer object should use a brand new object. >>> >>> $MAGIC compatibility logic like you want has no business living in the >>> copy helper. Had I spotted the intention during the original dmop >>> series, I would have rejected it during review. >> Actually, the existing behaviour is already broken. If a guest passes >> an overly short buf[0], the dmop logic won't get a failure, and instead >> get a truncated structure which has been zero extended. >> >> This is very definitely the wrong thing to do, because such a truncated >> structure might actually contain some legitimate operations. > So what, if that's what the caller meant to happen? If the caller doesn't provide all the information the hypervisor half of the interface expects, it should get a hard error, not a truncated attempt to fulful the request. The existing behaviour is simply broken. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |