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

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

On Mon, Jan 21, 2019 at 4:03 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 20.01.19 at 22:18, <christopher.w.clark@gmail.com> wrote:
> > 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:
> >
> > 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.

ack.

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

I have a patch to fix that, that it turns out I will not need but can
post separately if this is still wanted -- copied here for illustration.
(apologies in advance if this gets mail-client mangled here).

diff --git a/xen/tools/get-fields.sh b/xen/tools/get-fields.sh
index 45a0e2e..14c6859 100644
--- a/xen/tools/get-fields.sh
+++ b/xen/tools/get-fields.sh
@@ -438,7 +438,7 @@ build_check ()
 {
        echo
        echo "#define CHECK_$1 \\"
-       local level=1 fields= kind= id= arrlvl=1 token
+       local level=1 fields= kind= id= arrlvl=1 token suppress_dups=
        for token in $2
        do
                case "$token" in
@@ -470,8 +470,12 @@ build_check ()
                [\,\;])
                        if [ $level = 2 -a -n "$(echo $id | $SED
's,^_pad[[:digit:]]*,,')" ]
                        then
-                               check_field $kind $1 $id "$fields"
-                               test "$token" != ";" || fields= id=
+                if [ "${suppress_dups#*|$kind $1|}" = "${suppress_dups}" ]
+                then
+                    check_field $kind $1 $id "$fields"
+                    [ -z "$fields" ] ||
suppress_dups="${suppress_dups:-|}$kind $1|"
+                    test "$token" != ";" || fields= id=
+                fi
                        fi
                        ;;
                esac

On Tue, Jan 22, 2019 at 3:08 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 21.01.19 at 13:03, <JBeulich@suse.com> wrote:
> >>>> On 20.01.19 at 22:18, <christopher.w.clark@gmail.com> wrote:
> >> 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.
>
> Due to
>
> CHECK_mcinfo_common;
> # undef xen_mcinfo_common
> # undef CHECK_mcinfo_common
> # define CHECK_mcinfo_common         struct mcinfo_common
>
> which I think would be easy enough to use in your case as well
> (until we could perhaps get around and address the underlying
> issue, albeit it's not really clear to me how that should be done).

ack, this technique works for the Argo data structures, so I've
applied it, dropped the previous macro overrides, moved the checks
into the common/argo.c file and dropped common/compat/argo.c,
with each check being added at the same time as the structs go in
through the series.

Christopher

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