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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 7 Oct 2019 18:25:51 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Ralf Ramsauer <ralf.ramsauer@...-regensburg.de>
Cc:     Jan Kiszka <jan.kiszka@...mens.com>, x86@...nel.org,
        jailhouse-dev@...glegroups.com, linux-kernel@...r.kernel.org,
        Ingo Molnar <mingo@...hat.com>,
        "H . Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH v5 1/2] x86/jailhouse: improve setup data version
 comparison

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@...-regensburg.de>
> Reviewed-by: Jan Kiszka <jan.kiszka@...mens.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