Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 7 Oct 2019 18:25:51 +0200
From:   Borislav Petkov <bp@alien8.de>
To:     Ralf Ramsauer <ralf.ramsauer@oth-regensburg.de>
Cc:     Jan Kiszka <jan.kiszka@siemens.com>, x86@kernel.org,
        jailhouse-dev@googlegroups.com, linux-kernel@vger.kernel.org,
        Ingo Molnar <mingo@redhat.com>,
        "H . Peter Anvin" <hpa@zytor.com>
Subject: Re: [PATCH v5 1/2] x86/jailhouse: improve setup data version
 comparison
Message-ID: <20191007162551.GC24289@zn.tnic>
References: <20191007123819.161432-1-ralf.ramsauer@oth-regensburg.de>
 <20191007123819.161432-2-ralf.ramsauer@oth-regensburg.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20191007123819.161432-2-ralf.ramsauer@oth-regensburg.de>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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