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:   Mon, 16 May 2022 11:43:48 +0200
From:   Vit Kabele <vit@...ele.me>
To:     Borislav Petkov <bp@...en8.de>
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 Tue, May 03, 2022 at 07:36:35PM +0200, Borislav Petkov wrote:
> > 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?
I also initially thought of extracting the check to a separate method,
but imo this decreases the overall code readability. Any function
calling the get_bios_ebda() must check the returned value anyway, so
there will be always at least one if statement involved. And the valid
upper bound of the EBDA pointer is also different for these two use-cases.
(The mpparse.c usage is interested in EBDA pointer only if it ends 1KiB
before the end of low memory, while the ebda.c accepts even the values in the
last KiB below 640KiB).

I also consider it unlikely that there will be some new code using
the same bounds check, so I'd prefer to leave it inline.

-- 
Best regards,
Vit Kabele