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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.01.19 at 22:18, <christopher.w.clark@xxxxxxxxx> wrote:
> On Thu, Jan 17, 2019 at 3:25 AM Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>
>> >>> On 17.01.19 at 08:22, <christopher.w.clark@xxxxxxxxx> 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.

arch-x86/xen-mca.h has

struct mcinfo_global {
    struct mcinfo_common common;
    ...

which results in

#define CHECK_mcinfo_global \
    CHECK_SIZE_(struct, mcinfo_global); \
    CHECK_mcinfo_common; \
    ...

and separately

#define CHECK_mcinfo_common ...

which I would assume ought to similarly work for the Argo
structures.

> 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.

Structures containing handles are intentionally not covered
by the CHECK_* machinery, because handles necessarily
need translation due to their different widths in 32- and
64-bit modes on x86.

> 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.

Hmm, this looks like something that indeed wants fixing.

> 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.

Hmm, I think the mcinfo example above contradicts this, because
struct mcinfo_common is used by multiple other structures.

Jan



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