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

From: "Jan Beulich" <JBeulich@suse.com>
To: Christopher Clark <christopher.w.clark@gmail.com>
Cc: Tim Deegan <tim@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	ross.philipson@gmail.com, Jason Andryuk <jandryuk@gmail.com>,
	Daniel Smith <dpsmith@apertussolutions.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	Ian Jackson <Ian.Jackson@eu.citrix.com>,
	Rich Persaud <persaur@gmail.com>,
	James McKenzie <james@bromium.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Julien Grall <julien.grall@arm.com>,
	Paul Durrant <paul.durrant@citrix.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	eric chanudet <eric.chanudet@gmail.com>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH v3 15/15] argo: validate hypercall arg structures via compat machinery
Date: Thu, 17 Jan 2019 04:25:39 -0700	[thread overview]
Message-ID: <5C406633020000780020E9BC@prv1-mh.provo.novell.com> (raw)
In-Reply-To: <CACMJ4Gbcf9xXBATQsJtjx4LuiAxUXqjh8J3cevO+3x+6E-sZZQ@mail.gmail.com>

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

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

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

Jan

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