| 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
| ||
|
Message-ID: <f0e45cd2-5dc8-494f-a48d-369902c07fb6@suse.com>
Date: Wed, 29 Nov 2023 11:08:14 +0100
From: Juergen Gross <jgross@...e.com>
To: Borislav Petkov <bp@...en8.de>
Cc: linux-kernel@...r.kernel.org, x86@...nel.org,
virtualization@...ts.linux-foundation.org,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>,
Dave Hansen <dave.hansen@...ux.intel.com>,
"H. Peter Anvin" <hpa@...or.com>, Ajay Kaher <akaher@...are.com>,
Alexey Makhalov <amakhalov@...are.com>,
VMware PV-Drivers Reviewers <pv-drivers@...are.com>,
Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH v4 4/5] x86/paravirt: switch mixed paravirt/alternative
calls to alternative_2
On 21.11.23 19:45, Borislav Petkov wrote:
> On Mon, Oct 30, 2023 at 03:25:07PM +0100, Juergen Gross wrote:
>> Instead of stacking alternative and paravirt patching, use the new
>> ALT_FLAG_CALL flag to switch those mixed calls to pure alternative
>> handling.
>>
>> This eliminates the need to be careful regarding the sequence of
>> alternative and paravirt patching.
>>
>> For call depth tracking callthunks_setup() needs to be adapted to patch
>> calls at alternative patching sites instead of paravirt calls.
>
> Why is this important so that it is called out explicitly in the commit
> message? Is callthunks_setup() special somehow?
IMHO it is a non-obvious change, so I spelled it out explicitly. I can drop
that paragraph if you want.
>
>>
>> Signed-off-by: Juergen Gross <jgross@...e.com>
>> Acked-by: Peter Zijlstra (Intel) <peterz@...radead.org>
>> ---
>> arch/x86/include/asm/alternative.h | 5 +++--
>> arch/x86/include/asm/paravirt.h | 9 ++++++---
>> arch/x86/include/asm/paravirt_types.h | 26 +++++++++-----------------
>> arch/x86/kernel/callthunks.c | 17 ++++++++---------
>> arch/x86/kernel/module.c | 20 +++++---------------
>> 5 files changed, 31 insertions(+), 46 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/alternative.h b/arch/x86/include/asm/alternative.h
>> index 2a74a94bd569..07b17bc615a0 100644
>> --- a/arch/x86/include/asm/alternative.h
>> +++ b/arch/x86/include/asm/alternative.h
>> @@ -89,6 +89,8 @@ struct alt_instr {
>> u8 replacementlen; /* length of new instruction */
>> } __packed;
>>
>> +extern struct alt_instr __alt_instructions[], __alt_instructions_end[];
>> +
>
> arch/x86/include/asm/alternative.h:92:extern struct alt_instr __alt_instructions[], __alt_instructions_end[];
> arch/x86/kernel/alternative.c:163:extern struct alt_instr __alt_instructions[], __alt_instructions_end[];
>
> Zap the declaration from the .c file pls.
Okay.
>
>> /*
>> * Debug flag that can be tested to see whether alternative
>> * instructions were patched in already:
>> @@ -104,11 +106,10 @@ extern void apply_fineibt(s32 *start_retpoline, s32 *end_retpoine,
>> s32 *start_cfi, s32 *end_cfi);
>>
>> struct module;
>> -struct paravirt_patch_site;
>>
>> struct callthunk_sites {
>> s32 *call_start, *call_end;
>> - struct paravirt_patch_site *pv_start, *pv_end;
>> + struct alt_instr *alt_start, *alt_end;
>> };
>>
>> #ifdef CONFIG_CALL_THUNKS
>> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
>> index 3749311d51c3..9c6c5cfa9fe2 100644
>> --- a/arch/x86/include/asm/paravirt.h
>> +++ b/arch/x86/include/asm/paravirt.h
>> @@ -740,20 +740,23 @@ void native_pv_lock_init(void) __init;
>>
>> #ifdef CONFIG_X86_64
>> #ifdef CONFIG_PARAVIRT_XXL
>> +#ifdef CONFIG_DEBUG_ENTRY
>>
>> #define PARA_PATCH(off) ((off) / 8)
>> #define PARA_SITE(ptype, ops) _PVSITE(ptype, ops, .quad, 8)
>> #define PARA_INDIRECT(addr) *addr(%rip)
>>
>> -#ifdef CONFIG_DEBUG_ENTRY
>> .macro PARA_IRQ_save_fl
>> PARA_SITE(PARA_PATCH(PV_IRQ_save_fl),
>> ANNOTATE_RETPOLINE_SAFE;
>> call PARA_INDIRECT(pv_ops+PV_IRQ_save_fl);)
>> + ANNOTATE_RETPOLINE_SAFE;
>> + call PARA_INDIRECT(pv_ops+PV_IRQ_save_fl);
>> .endm
>>
>> -#define SAVE_FLAGS ALTERNATIVE "PARA_IRQ_save_fl;", "pushf; pop %rax;", \
>> - ALT_NOT_XEN
>> +#define SAVE_FLAGS ALTERNATIVE_2 "PARA_IRQ_save_fl;", \
>> + ALT_CALL_INSTR, ALT_CALL_ALWAYS, \
>> + "pushf; pop %rax;", ALT_NOT_XEN
>
> How is that supposed to work?
>
> At build time for a PARAVIRT_XXL build it'll have that PARA_IRQ_save_fl
> macro in there which issues a .parainstructions section and an indirect
> call to
>
> call *pv_ops+240(%rip);
>
> then it'll always patch in "call BUG_func" due to X86_FEATURE_ALWAYS.
>
> I guess this is your way of saying "this should always be patched, one
> way or the other, depending on X86_FEATURE_XENPV, and this is a way to
> catch unpatched locations...
>
> Then on a pv build which doesn't set X86_FEATURE_XENPV during boot,
> it'll replace the "call BUG_func" thing with the pushf;pop.
>
> And if it does set X86_FEATURE_XENPV, it'll patch in the direct call to
> .... /me greps tree ... pv_native_save_fl I guess.
>
> If anything, how those ALT_CALL_ALWAYS things are supposed to work,
> should be documented there, over the macro definition and what the
> intent is.
I can do that, but OTOH the existing comments are quite clear:
* If CPU has feature2, newinstr2 is used.
* Otherwise, if CPU has feature1, newinstr1 is used.
* Otherwise, oldinstr is used.
>
> Because otherwise we'll have to swap in the whole machinery back into
> our L1s each time we need to touch it.
>
> And btw, this whole patching stuff becomes insanely non-trivial slowly.
Not worse than today. It just replaces the paravirt patching with an
alternative patching.
>
> :-\
>
>> diff --git a/arch/x86/kernel/callthunks.c b/arch/x86/kernel/callthunks.c
>> index faa9f2299848..200ea8087ddb 100644
>> --- a/arch/x86/kernel/callthunks.c
>> +++ b/arch/x86/kernel/callthunks.c
>> @@ -238,14 +238,13 @@ patch_call_sites(s32 *start, s32 *end, const struct core_text *ct)
>> }
>>
>> static __init_or_module void
>> -patch_paravirt_call_sites(struct paravirt_patch_site *start,
>> - struct paravirt_patch_site *end,
>> - const struct core_text *ct)
>> +patch_alt_call_sites(struct alt_instr *start, struct alt_instr *end,
>> + const struct core_text *ct)
>> {
>> - struct paravirt_patch_site *p;
>> + struct alt_instr *a;
>>
>> - for (p = start; p < end; p++)
>> - patch_call(p->instr, ct);
>> + for (a = start; a < end; a++)
>> + patch_call((u8 *)&a->instr_offset + a->instr_offset, ct);
>
> tip:x86/paravirt has:
>
> 5c22c4726e4a ("x86/paravirt: Use relative reference for the original instruction offset")
>
> Perhaps redo yours ontop of tip/master:
Yes, of course.
Juergen
Download attachment "OpenPGP_0xB0DE9DD628BF132F.asc" of type "application/pgp-keys" (3684 bytes)
Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (496 bytes)
Powered by blists - more mailing lists