Re: [PATCH v5 1/2] x86/jailhouse: improve setup data version comparison

[Borislav Petkov <bp@alien8.de>]

On Mon, Oct 07, 2019 at 02:38:18PM +0200, Ralf Ramsauer wrote:
> We will soon introduce a new setup_data version and extend the

Who is "We"?

There a couple more "we" below. Can you please rewrite that commit message in
passive voice and thus dispense my confusion about who's "we"? :)

> structure. This requires some preparational work for the sanity check of
> the header and the check of the version.
> 
> Use the following strategy:
> 
> 1. Ensure that the header declares at least enough space for the version
>    and the compatible_version as we must hold that fields for any
>    version. Furthermore, the location and semantics of those fields will
>    never change.
> 
> 2. Copy over data -- as much as we can. The length is either limited by
>    the header length, or the length of setup_data.
> 
> 3. Things are now in place -- sanity check if the header length complies
>    the actual version.
> 
> For future versions of the setup_data, only step 3 requires alignment.
> 
> Signed-off-by: Ralf Ramsauer <ralf.ramsauer@oth-regensburg.de>
> Reviewed-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  arch/x86/include/uapi/asm/bootparam.h | 22 +++++++-----
>  arch/x86/kernel/jailhouse.c           | 50 +++++++++++++++++----------
>  2 files changed, 44 insertions(+), 28 deletions(-)

...

> @@ -180,13 +175,26 @@ static void __init jailhouse_init_platform(void)
>  	if (!pa_data)
>  		panic("Jailhouse: No valid setup data found");
>  
> -	if (setup_data.compatible_version > JAILHOUSE_SETUP_REQUIRED_VERSION)
> -		panic("Jailhouse: Unsupported setup data structure");
> +	/* setup data must at least contain the header */
> +	if (header.len < sizeof(setup_data.hdr))
> +		goto unsupported;
>  
> -	pmtmr_ioport = setup_data.pm_timer_address;
> +	pa_data += offsetof(struct setup_data, data);
> +	setup_data_len = min(sizeof(setup_data), (unsigned long)header.len);

Checkpatch makes sense here:

WARNING: min() should probably be min_t(unsigned long, sizeof(setup_data), header.len)
#165: FILE: arch/x86/kernel/jailhouse.c:183:
+       setup_data_len = min(sizeof(setup_data), (unsigned long)header.len);

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette