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: Mon, 14 Jan 2019 05:57:49 -0700	[thread overview]
Message-ID: <5C3C874D020000780020D3D4@prv1-mh.provo.novell.com> (raw)
In-Reply-To: <1546846968-7372-16-git-send-email-christopher.w.clark@gmail.com>

>>> 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? Note that to date we didn't have to
make exceptions like this (iirc), so I'm not happy to see some appear.

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

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

Jan

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