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

[Vit Kabele]

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