linux-kernel - Re: [PATCH] x86/boot: skip realmode init code when running as Xen PV guest

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] [day] [month] [year] [list]

Date:   Wed, 23 Nov 2022 07:29:16 +0100
From:   Juergen Gross <jgross@...e.com>
To:     "H. Peter Anvin" <hpa@...or.com>, Borislav Petkov <bp@...en8.de>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>
Subject: Re: [PATCH] x86/boot: skip realmode init code when running as Xen PV
 guest

On 23.11.22 01:07, H. Peter Anvin wrote:
> On November 21, 2022 10:28:21 PM PST, Juergen Gross <jgross@...e.com> wrote:
>> On 21.11.22 20:08, Borislav Petkov wrote:
>>> On Mon, Nov 21, 2022 at 05:24:33PM +0100, Juergen Gross wrote:
>>>> When running as a Xen PV guest there is no need for setting up the
>>>> realmode trampoline, as realmode isn't supported in this environment.
>>>>
>>>> Trying to setup the trampoline has been proven to be problematic in
>>>> some cases, especially when trying to debug early boot problems with
>>>> Xen requiring to keep the EFI boot-services memory mapped (some
>>>> firmware variants seem to claim basically all memory below 1M for boot
>>>> services).
>>>>
>>>> Skip the trampoline setup code for Xen PV guests.
>>>>
>>>> Fixes: 084ee1c641a0 ("x86, realmode: Relocator for realmode code")
>>>> Signed-off-by: Juergen Gross <jgross@...e.com>
>>>> ---
>>>>    arch/x86/include/asm/realmode.h | 4 ++--
>>>>    arch/x86/realmode/init.c        | 3 +++
>>>>    2 files changed, 5 insertions(+), 2 deletions(-)
>>>    
>>>> diff --git a/arch/x86/include/asm/realmode.h b/arch/x86/include/asm/realmode.h
>>>> index fd6f6e5b755a..5bfce58f1bab 100644
>>>> --- a/arch/x86/include/asm/realmode.h
>>>> +++ b/arch/x86/include/asm/realmode.h
>>>> @@ -78,8 +78,8 @@ extern unsigned char secondary_startup_64_no_verify[];
>>>>      static inline size_t real_mode_size_needed(void)
>>>>    {
>>>> -	if (real_mode_header)
>>>> -		return 0;	/* already allocated. */
>>>> +	if (real_mode_header || cpu_feature_enabled(X86_FEATURE_XENPV))
>>>> +		return 0;	/* already allocated or not needed. */
>>>>      	return ALIGN(real_mode_blob_end - real_mode_blob, PAGE_SIZE);
>>>>    }
>>>> diff --git a/arch/x86/realmode/init.c b/arch/x86/realmode/init.c
>>>> index 41d7669a97ad..1826700b156e 100644
>>>> --- a/arch/x86/realmode/init.c
>>>> +++ b/arch/x86/realmode/init.c
>>>> @@ -202,6 +202,9 @@ static void __init set_real_mode_permissions(void)
>>>>      static int __init init_real_mode(void)
>>>>    {
>>>> +	if (cpu_feature_enabled(X86_FEATURE_XENPV))a
>>>
>>> This reminds me of the notorious if (xen) sprinkling from years ago.
>>> Please don't do that.
>>>
>>
>> Okay, what about plan B:
>>
>> - rework realmode/rm to:
>>   + replace header.S with main.c making it possible to initialize
>>     struct real_mode_header using the struct definition
>>   + optional: merge stack.S into main.c
>> - include realmode/rm addresses needed outside of it in struct
>>   real_mode_header
>> - setup a dummy struct real_mode_header in Xen PV code removing the
>>   need to skip init_real_mode(), but making it basically a nop
>>
>> Would you be fine with that?
>>
>>
>> Juergen
> 
> I'm wondering if init_real_mode should not simply be part of the platform ops. It's called exactly twice per boot, it is hard to be less performance critical than that.

Actually, it is called only once. :-)

Any preferences regarding the call hierarchy? I could either keep
the early_initcall() and call the platform_op from that initcall, or
I could introduce a small wrapper called by kernel_init_freeable(),
which would be empty on all but the x86 architecture.

Boris, if you agree, I can go that route.


Juergen

Download attachment "OpenPGP_0xB0DE9DD628BF132F.asc" of type "application/pgp-keys" (3099 bytes)

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (496 bytes)