linux-kernel - Re: [PATCH 1/2] x86/boot: Fix memremap of setup

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, 22 Feb 2022 16:01:32 -0500
From:   Ross Philipson <ross.philipson@...cle.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org,
        daniel.kiper@...cle.com, dpsmith@...rtussolutions.com,
        tglx@...utronix.de, mingo@...hat.com, hpa@...or.com,
        luto@...capital.net, kanth.ghatraju@...cle.com,
        trenchboot-devel@...glegroups.com
Subject: Re: [PATCH 1/2] x86/boot: Fix memremap of setup_indirect structures

On 2/15/22 13:37, Borislav Petkov wrote:
> On Tue, Feb 15, 2022 at 06:34:43AM -0500, Ross Philipson wrote:
>> It can if you run out of slots in the fixed map.
> 
> Right. Or if any of the checks in __early_ioremap() fail. But those
> would at least warn.
> 
>> The only reason I did not check it for NULL was because it was not
>> checked elsewhere for NULL.
> 
> Elsewhere in the tree or elsewhere in this file or in the setup_indirect
> adding code?

In the ioremap.c module, the check for NULL is only missing in the
functions we updated but the lack of a check was already there before
these changes went in.

In the setup.c and e820.c modules, the check for NULL is missing in the
functions we updated but the lack of a check was already there before
these changes went in in those functions. The lack of early_memremap()
NULL checks can also be found in other functions in those modules.

> 
>> I guess there are two questions:
>>
>> 1. Should I also fix it elsewhere in the code I am touching?
> 
> Yes pls.
> 
>> 2. What should I do on an allocation failure? In a routine like this it
>> seems to be a critical early boot failure.
> 
> How so?
> 
> I'd expect in the case of e820__reserve_setup_data(), for example, to
> not call e820__range_update* and not have those indirect ranges present
> in the e820 map. What the user intended might not work but it'll at
> least boot instead of floating dead in the water.
> 
> And similar approach in the other places you're touching.

Yes I can see how to handle the failures. I will fix the code to do the
appropriate thing given what each of the functions is doing.

Fixing it in other functions and possibly elsewhere in the arch/x86 code
seems to be out of scope for this patch set. We could send separate
patches and hunt down other places this check is missing.

> 
> You could even issue a warning or so so that users at least know what's
> going on. I'd say...

Yea I can pr_warn when the issue occurs.

> 
>> I guess the original intention might have been to let it just blow up
>> since there is no recovery but that is just conjecture...
> 
> The original intention?

It was just idle speculation, just ignore this.

Thanks
Ross

> 
> Thx.
>