linux-kernel - Re: [PATCH v5 18/32] x86, mpparse: Use memremap to map the mpf and mpc data

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]

Message-ID: <20170516083658.fq2h4ysmrbgn23cs@pd.tnic>
Date:   Tue, 16 May 2017 10:36:58 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Tom Lendacky <thomas.lendacky@....com>
Cc:     linux-arch@...r.kernel.org, linux-efi@...r.kernel.org,
        kvm@...r.kernel.org, linux-doc@...r.kernel.org, x86@...nel.org,
        kexec@...ts.infradead.org, linux-kernel@...r.kernel.org,
        kasan-dev@...glegroups.com, linux-mm@...ck.org,
        iommu@...ts.linux-foundation.org, Rik van Riel <riel@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        Toshimitsu Kani <toshi.kani@....com>,
        Arnd Bergmann <arnd@...db.de>,
        Jonathan Corbet <corbet@....net>,
        Matt Fleming <matt@...eblueprint.co.uk>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Joerg Roedel <joro@...tes.org>,
        Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Larry Woodman <lwoodman@...hat.com>,
        Brijesh Singh <brijesh.singh@....com>,
        Ingo Molnar <mingo@...hat.com>,
        Andy Lutomirski <luto@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        Andrey Ryabinin <aryabinin@...tuozzo.com>,
        Alexander Potapenko <glider@...gle.com>,
        Dave Young <dyoung@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Dmitry Vyukov <dvyukov@...gle.com>
Subject: Re: [PATCH v5 18/32] x86, mpparse: Use memremap to map the mpf and
 mpc data

On Tue, Apr 18, 2017 at 04:19:30PM -0500, Tom Lendacky wrote:
> The SMP MP-table is built by UEFI and placed in memory in a decrypted
> state. These tables are accessed using a mix of early_memremap(),
> early_memunmap(), phys_to_virt() and virt_to_phys(). Change all accesses
> to use early_memremap()/early_memunmap(). This allows for proper setting
> of the encryption mask so that the data can be successfully accessed when
> SME is active.
> 
> Signed-off-by: Tom Lendacky <thomas.lendacky@....com>
> ---
>  arch/x86/kernel/mpparse.c |  102 +++++++++++++++++++++++++++++++--------------
>  1 file changed, 71 insertions(+), 31 deletions(-)
> 
> diff --git a/arch/x86/kernel/mpparse.c b/arch/x86/kernel/mpparse.c
> index fd37f39..afbda41d 100644
> --- a/arch/x86/kernel/mpparse.c
> +++ b/arch/x86/kernel/mpparse.c
> @@ -429,7 +429,21 @@ static inline void __init construct_default_ISA_mptable(int mpc_default_type)
>  	}
>  }
>  
> -static struct mpf_intel *mpf_found;
> +static unsigned long mpf_base;
> +
> +static void __init unmap_mpf(struct mpf_intel *mpf)
> +{
> +	early_memunmap(mpf, sizeof(*mpf));
> +}
> +
> +static struct mpf_intel * __init map_mpf(unsigned long paddr)
> +{
> +	struct mpf_intel *mpf;
> +
> +	mpf = early_memremap(paddr, sizeof(*mpf));
> +
> +	return mpf;

	return early_memremap(paddr, sizeof(*mpf));

...

> @@ -842,25 +873,26 @@ static int __init update_mp_table(void)
>  	if (!enable_update_mptable)
>  		return 0;
>  
> -	mpf = mpf_found;
> -	if (!mpf)
> +	if (!mpf_base)
>  		return 0;
>  
> +	mpf = map_mpf(mpf_base);
> +
>  	/*
>  	 * Now see if we need to go further.
>  	 */
>  	if (mpf->feature1 != 0)

You're kidding, right? map_mpf() *can* return NULL.

Also, simplify that test:

	if (mpf->feature1)
		...


> -		return 0;
> +		goto do_unmap_mpf;
>  
>  	if (!mpf->physptr)
> -		return 0;
> +		goto do_unmap_mpf;
>  
> -	mpc = phys_to_virt(mpf->physptr);
> +	mpc = map_mpc(mpf->physptr);

Again: error checking !!!

You have other calls to early_memremap()/map_mpf() in this patch. Please
add error checking everywhere.

>  
>  	if (!smp_check_mpc(mpc, oem, str))
> -		return 0;
> +		goto do_unmap_mpc;
>  
> -	pr_info("mpf: %llx\n", (u64)virt_to_phys(mpf));
> +	pr_info("mpf: %llx\n", (u64)mpf_base);
>  	pr_info("physptr: %x\n", mpf->physptr);
>  
>  	if (mpc_new_phys && mpc->length > mpc_new_length) {
> @@ -878,21 +910,23 @@ static int __init update_mp_table(void)
>  		new = mpf_checksum((unsigned char *)mpc, mpc->length);
>  		if (old == new) {
>  			pr_info("mpc is readonly, please try alloc_mptable instead\n");
> -			return 0;
> +			goto do_unmap_mpc;
>  		}
>  		pr_info("use in-position replacing\n");
>  	} else {
>  		mpf->physptr = mpc_new_phys;
> -		mpc_new = phys_to_virt(mpc_new_phys);
> +		mpc_new = map_mpc(mpc_new_phys);

Ditto.

>  		memcpy(mpc_new, mpc, mpc->length);
> +		unmap_mpc(mpc);
>  		mpc = mpc_new;
>  		/* check if we can modify that */
>  		if (mpc_new_phys - mpf->physptr) {
>  			struct mpf_intel *mpf_new;
>  			/* steal 16 bytes from [0, 1k) */
>  			pr_info("mpf new: %x\n", 0x400 - 16);
> -			mpf_new = phys_to_virt(0x400 - 16);
> +			mpf_new = map_mpf(0x400 - 16);

Ditto.

>  			memcpy(mpf_new, mpf, 16);
> +			unmap_mpf(mpf);
>  			mpf = mpf_new;
>  			mpf->physptr = mpc_new_phys;
>  		}

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.