linux-kernel - Re: [PATCH v2] arch/x86: Check validity of EBDA pointer in mpparse.c

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:   Tue, 3 May 2022 19:36:35 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Vit Kabele <vit@...ele.me>
Cc:     platform-driver-x86@...r.kernel.org, r.marek@...embler.cz,
        x86@...nel.org, linux-kernel@...r.kernel.org, rafael@...nel.org,
        mingo@...hat.com
Subject: Re: [PATCH v2] arch/x86: Check validity of EBDA pointer in mpparse.c

On Fri, Apr 08, 2022 at 10:46:46AM +0200, Vit Kabele wrote:
> The pointer to EBDA area is retrieved from a word at 0x40e in BDA.
> In case that the memory there is not initialized and contains garbage,
> it might happen that the kernel touches memory above 640K.

This is where I'm missing the "why do this" at all. Grepping back in
my mbox, I see another thread from you where you say something about
"testing custom virtualization platform".

So I'd like to see why this fix is needed so feel free to elaborate in the
commit message what the situation is and why you're doing this.

> This may cause unwanted reads from VGA memory which may not be decoded,
> or even present when running under virtualization.
> 
> This patch adds sanity check for the EBDA pointer retrieved from the memory

Avoid having "This patch" or "This commit" in the commit message. It is
tautologically useless.

IOW,

s/This patch adds/Add/

> diff --git a/arch/x86/kernel/mpparse.c b/arch/x86/kernel/mpparse.c
> index fed721f90116..9e0b4820f33b 100644
> --- a/arch/x86/kernel/mpparse.c
> +++ b/arch/x86/kernel/mpparse.c
> @@ -633,8 +633,18 @@ void __init default_find_smp_config(void)
>  	 */
>  
>  	address = get_bios_ebda();
> -	if (address)
> -		smp_scan_config(address, 0x400);
> +
> +	/*
> +	 * Check that the EBDA address is sane and the get_bios_ebda() did not
> +	 * return just garbage from memory.
> +	 * The upper bound is considered valid if it points below 1K before
> +	 * end of the lower memory (i.e. 639K). The EBDA can be smaller
> +	 * than 1K in which case the pointer will point above 639K but that
> +	 * case is handled in step 2) above, and we don't need to adjust scan
> +	 * size to not bump into the memory above 640K.
> +	 */
> +	if (address >= BIOS_START_MIN && address < (BIOS_START_MAX - 1024))
> +		smp_scan_config(address, 1024);
>  }

I guess but looking at reserve_bios_regions(), that function is already
doing kinda the same along with being a bit more careful to figure out
bios_start so you could unify the code into a common helper and use it
at both places?

Thx.

-- 
Regards/Gruss,
    Boris.

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