Re: [PATCH v3 15/15] argo: validate hypercall arg structures via compat machinery

[Christopher Clark <christopher.w.clark@gmail.com>]

On Mon, Jan 14, 2019 at 4:57 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 07.01.19 at 08:42, <christopher.w.clark@gmail.com> wrote:
> > Argo doesn't use compat hypercall or argument translation but can use some
> > of the infrastructure for validating the hypercall argument structures to
> > ensure that the struct sizes, offsets and compositions don't vary between 32
> > and 64bit, so add that here in a new dedicated source file for this purpose.
> >
> > Some of the argo hypercall argument structures contain elements that are
> > hypercall argument structure types themselves, and the standard compat
> > structure validation does not handle this, since the types differ in compat
> > vs. non-compat versions; so for some of the tests the exact-type-match check
> > is replaced with a weaker, but still sufficient, sizeof check.
>
> "Still sufficient" on what basis?

I may have overstepped with that assertion, but I meant sufficient for
the purposes of checking that the fields within the generated compat
structures were the same size and offset, so that copys of data to and
from guests that the code performs behave correctly.

> Note that to date we didn't have to  make exceptions like this (iirc),
> so I'm not happy to see some appear.

Yes, that's completely understandable.

> > Then there are additional hypercall argument structures that contain
> > elements that do not have a fixed size (last element, variable length array
> > fields), so we have to then disable that size check too for validating those
> > structures; the coverage of offset of elements is still retained.
>
> There are prior cases of such as well; I'm not sure though if any
> were actually in need of checking through these macros. Still I'd
> like to better understand what it is that doesn't work in that case.
> Quite possibly there's something that can be fixed in the scripts
> (or elsewhere).

Some details of the problem:

Without the macro overrides in place (ie. using the existing
definitions) the build fails on CHECK_argo_send_addr  because this
struct is defined with types that are themselves translated by the
compat processing:

typedef struct xen_argo_send_addr
{
    xen_argo_addr_t src;
    xen_argo_addr_t dst;
} xen_argo_send_addr_t;

compat/argo.c: In function '__checkFstruct_argo_send_addr__src':
xen/include/xen/compat.h:170:18: error: comparison of distinct pointer
types lacks a cast [-Werror]
     return &x->f == &c->f; \
                  ^
xen/include/xen/compat.h:176:5: note: in expansion of macro
'CHECK_FIELD_COMMON_'
     CHECK_FIELD_COMMON_(k, CHECK_NAME_(k, n ## __ ## f, F), n, f)
     ^~~~~~~~~~~~~~~~~~~
xen/include/compat/xlat.h:1238:5: note: in expansion of macro 'CHECK_FIELD_'
     CHECK_FIELD_(struct, argo_send_addr, src); \
     ^~~~~~~~~~~~
compat/argo.c:43:1: note: in expansion of macro 'CHECK_argo_send_addr'
 CHECK_argo_send_addr;
 ^~~~~~~~~~~~~~~~~~~~

because xen_argo_addr_t is detected as a different type than
compat_argo_addr_t -- when in practice is the same size and has the
same fields at the same offsets.

These also fail for the same reason: they also contain types that are
compat-converted:
CHECK_argo_ring_data_ent;
CHECK_argo_iov;
CHECK_argo_ring_data;

So the first override substitutes a "sizeof" check for the exact type
match, but that doesn't work for CHECK_argo_ring_data, because of the
variable-sized array field, so that CHECK has a separate override just
for it -- and again, it's only encountering this because the array is
of a compat-translated type.

>
> > --- a/xen/common/Makefile
> > +++ b/xen/common/Makefile
> > @@ -70,7 +70,7 @@ obj-y += xmalloc_tlsf.o
> >  obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma unlzo unlz4 earlycpio,$(n).init.o)
> >
> >
> > -obj-$(CONFIG_COMPAT) += $(addprefix compat/,domain.o kernel.o memory.o multicall.o xlat.o)
> > +obj-$(CONFIG_COMPAT) += $(addprefix compat/,argo.o domain.o kernel.o memory.o multicall.o xlat.o)
>
> While a matter of taste to a certain degree, I'm not convinced
> introducing a separate file for this is really necessary, especially
> if some of the overrides to the CHECK_* macros would go away.

ack. I wouldn't have moved them out if the overrides weren't in use;
but I will merge it into the implementation file if that is preferred.

Christopher

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel