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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 21 April 2017 13:05
> To: Jennifer Herbert <jennifer.herbert@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Paul Durrant <Paul.Durrant@xxxxxxxxxx>;
> Xen-devel <xen-devel@xxxxxxxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH 2/4] hvm/dmop: Implement copy_{to,
> from}_guest_buf() in terms of raw accessors
> 
> >>> On 21.04.17 at 13:03, <Jennifer.Herbert@xxxxxxxxxx> wrote:
> 
> >
> > On 21/04/17 11:38, Jan Beulich wrote:
> >>>>> 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?
> >
> > I think in the general case, its wrong to assume that its ok to truncate
> > a buffer.
> > In specific cases, such as buf[0], we don't really know how big the buf
> > needs to be until we start parsing it.
> 
> How that? Just like with domctl and sysctl the caller is supposed to
> be handing a full struct xen_dm_op, so I'd rather view this as the
> specific case where zero-extension is of no use.
> 
> > Certainly when we get to updating the accesser helper to deal with
> > offsets, its get more confused.  If we give it an offset greater then
> > the size of the buffer, do we just memset?  That would seem wrong.  With
> > an offset of 0, we want it to do the memset thing, to deal with buf[0].
> 
> Why? It's the natural extension of the zero-extending model.
> 
> > But what about if we have an offset.  I'd think the calling function
> > would really want to know if its got nonsense.
> 
> Zero-extended input is not nonsense to me.
> 
> > Another solution would be to have two variants of the helper function. A
> > 'normal' one, and the best effort one.
> 
> I don't think we want that. Andrew being wholeheartedly opposed
> to this zero-extension approach, he wouldn't agree to that either
> afaict. Considering that I don't expect him to change his mind, I
> guess I'll give up opposition to the change done, provided it is
> being done as a separate patch. Of course I'd be interested to
> know Paul's position here, as back then he appeared to agree
> with using this model (or maybe it also was just giving in)...
> 

I've lost track of all the various arguments but my position is that zero 
extending makes no sense; the caller must always supply sufficient data. The 
length check in the helper function should not be an identity check though 
since not all the data from a particular buffer may be required in one go. Thus 
the patch, with the macro changes, should be ok.

  Paul

> Jan

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