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

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

On Thu, Jan 17, 2019 at 3:25 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 17.01.19 at 08:22, <christopher.w.clark@gmail.com> wrote:
> > 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:
>
> But that's a normal situation.

I thought it would be too but I haven't found a direct equivalent to
what this header needs. I'll outline the results of my examination
below.

>
> > 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.
>
> Did you perhaps not add entries for the inner structures to xlat.lst?

No, unfortunately I did.

Here are my findings after exploring the compat machinery, in relation
to understanding its processing of the the Argo public header file, and
its production of the compat header file and validation macros.

1) Two alternative validation macros are possible as output, depending
on how struct fields are declared within structs in the input header.

Struct fields that are themselves structs can be declared two different
ways:

* by type (I'll call this "type form"): xen_mystruct_t field;

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


* by struct name ("struct form"): struct xen_mystruct field;

eg.
    typedef struct xen_argo_send_addr
    {
        struct xen_argo_addr src;
        struct xen_argo_addr dst;
    } xen_argo_send_addr_t;


In the validation macros that are produced for xen_argo_send_addr, the
"struct form" contains:
    CHECK_mystruct;

and the "type form" contains instead:
    CHECK_FIELD_(struct, mystruct, field);

These two validation macros do different things; the CHECK_FIELD one
is stronger because it tests that the offset, size and type of the
field match, whereas the other checks the structure of the inner struct,
but not its placement within the outer or matching type in compat vs
non-compat.

After reviewing other public headers within Xen and the structs in the
xlat.lst list, it looks like some existing structs are passing
CHECKs by using the "struct" form, even though the checks are weaker.


2. The "type form" checks cannot work for fields that are themselves
compat-translated.

For fields that are struct types that are themselves compat-translated,
the CHECK_FIELD fails because it does a typeof check in the macro,
which cannot pass since the non-compat type will never equal the
compat-type -- the fields really are different types.

So when defining a struct field with a type that will be translated,
you have to declare them using the "struct" form, not the "type" form.

A prior example of this is: struct mcinfo_extended
which declares an array field using the struct form, and not its type.

A problem with what the "struct" form generates is that the test that it
produces doesn't check the offset of the field within the struct it
belongs to. Enabling CHECK_FIELD to work looks preferable as it
provides better assurance of correctness.

One way to make CHECK_FIELD work is to override the CHECK_COMMON macro,
and disable the typeof check when necessary, as has been presented in
the versions of the Argo patch series so far.


3. A challenge with using the "struct" form, following from the result
of point 2, occurs when it's a XEN_GUEST_HANDLE field within the struct.
It's not obvious how to declare that field using the "struct" form
rather than the "type" form.
This affects the argo_iov struct.

4. Macros to perform "struct form" checks cannot be repeated.

When using the "struct" form, it's problem when the struct contains two
fields of the same compat-translated type.

eg. consider the "struct form" version of xen_argo_send_addr, which has
two fields of struct xen_argo_addr:

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

which then generates this in the compat header:

    #define CHECK_argo_send_addr \
        CHECK_SIZE_(struct, argo_send_addr); \
        CHECK_argo_addr; \
        CHECK_argo_addr

and the second macro invocation of CHECK_argo_addr just breaks, with the
build failing due to redefinition of a symbol that is already defined.

A (horrible, unacceptable) workaround that unblocks further discovery
is to redefine xen_argo_send_addr to use an array:

typedef struct xen_argo_send_addr
{
    struct xen_argo_addr addrs[2];
} xen_argo_send_addr_t;

which does pass, since it now generates this:

#define CHECK_argo_send_addr \
    CHECK_SIZE_(struct, argo_send_addr); \
    CHECK_argo_addr

and then CHECK_argo_send_addr passes, *but* the fields are no longer
named as they should be, which is not OK.

The "no repeated checks" problem also occurs when another separate
struct contains a field of a type that has already been checked:
whichever CHECK is performed second will break.

eg.
typedef struct xen_argo_ring_data_ent
{
    struct xen_argo_addr ring;
    uint16_t flags;
    uint16_t pad;
    uint32_t space_required;
    uint32_t max_message_size;
} xen_argo_ring_data_ent_t;

also has a field of type xen_argo_addr, which produces CHECK_argo_addr,
which then fails because that was already tested in
CHECK_argo_send_addr.

Anyway, hopefully this provides context for evaluating the method of
passing the compat tests that has been proposed in the Argo series:
selective override of the CHECK_FIELD_COMMON macro to disable the typeof
checks only for validating those structs that require it to be turned
off.


> >> > --- 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.
>
> Well - let's first see whether the overrides are really needed. If so,
> keeping this in a separate file might indeed be better.

ack.

Christopher

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