Re: [Xen-devel] [PATCH 2/4] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.04.17 at 12:11, <Jennifer.Herbert@xxxxxxxxxx> wrote:
> 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?
>>
>> Considering this is a controversial change, I think it is a bad idea
>> to merge this into the otherwise uncontroversial change here.
> 
> How about we move the min(..) and memset out of the helper function, and 
> into dm_op?

Wouldn't that result in different buffers being treated differently,
which I'd rather not see happening?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel