[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 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. In this case, we want to say
'give me what you have, upto this maximum'. This is the special case,
not the general one, an as such does not belong in the general case
accesser helper.
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].
But what about if we have an offset. I'd think the calling function
would really want to know if its got nonsense.
Another solution would be to have two variants of the helper function. A
'normal' one, and the best effort one.
-jenny
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
