* Re: [PATCH 1/2] x86/paravirt: clean up typos and grammaros [not found] ` <20220902213750.1124421-2-morbo@google.com> @ 2022-09-03 4:28 ` Borislav Petkov 0 siblings, 0 replies; 6+ messages in thread From: Borislav Petkov @ 2022-09-03 4:28 UTC (permalink / raw) To: Bill Wendling Cc: Juergen Gross, x86, H. Peter Anvin, VMware PV-Drivers Reviewers, Dave Hansen, Nick Desaulniers, linux-kernel, virtualization, Nathan Chancellor, Ingo Molnar, llvm, Alexey Makhalov, Thomas Gleixner On Fri, Sep 02, 2022 at 09:37:49PM +0000, Bill Wendling wrote: > Drive-by clean up of the comment. > > [ Impact: cleanup] We used to do that type of "impact" tagging years ago but we stopped. Where did you dig this out from? :) -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <20220902213750.1124421-3-morbo@google.com>]
* Re: [PATCH 2/2] x86/paravirt: add extra clobbers with ZERO_CALL_USED_REGS enabled [not found] ` <20220902213750.1124421-3-morbo@google.com> @ 2022-09-03 7:18 ` Kees Cook [not found] ` <CAGG=3QXpK+bFOSYZkdNNFGzNfgJSSADGTRWYRv6z0vfBAgQvWQ@mail.gmail.com> 0 siblings, 1 reply; 6+ messages in thread From: Kees Cook @ 2022-09-03 7:18 UTC (permalink / raw) To: Bill Wendling Cc: Juergen Gross, x86, H. Peter Anvin, VMware PV-Drivers Reviewers, Dave Hansen, Nick Desaulniers, linux-kernel, virtualization, Nathan Chancellor, Ingo Molnar, Borislav Petkov, linux-hardening, llvm, Alexey Makhalov, Thomas Gleixner On Fri, Sep 02, 2022 at 09:37:50PM +0000, Bill Wendling wrote: > [...] > callq *pv_ops+536(%rip) Do you know which pv_ops function is this? I can't figure out where pte_offset_kernel() gets converted into a pv_ops call.... > [...] > --- a/arch/x86/include/asm/paravirt_types.h > +++ b/arch/x86/include/asm/paravirt_types.h > @@ -414,8 +414,17 @@ int paravirt_disable_iospace(void); > "=c" (__ecx) > #define PVOP_CALL_CLOBBERS PVOP_VCALL_CLOBBERS, "=a" (__eax) > > -/* void functions are still allowed [re]ax for scratch */ > +/* > + * void functions are still allowed [re]ax for scratch. > + * > + * The ZERO_CALL_USED REGS feature may end up zeroing out callee-saved > + * registers. Make sure we model this with the appropriate clobbers. > + */ > +#ifdef CONFIG_ZERO_CALL_USED_REGS > +#define PVOP_VCALLEE_CLOBBERS "=a" (__eax), PVOP_VCALL_CLOBBERS > +#else > #define PVOP_VCALLEE_CLOBBERS "=a" (__eax) > +#endif > #define PVOP_CALLEE_CLOBBERS PVOP_VCALLEE_CLOBBERS I don't think this should depend on CONFIG_ZERO_CALL_USED_REGS; it should always be present. I've only been looking at this just now, so many I'm missing something. The callee clobbers are for functions with return values, yes? For example, 32-bit has to manually deal with doing a 64-bit value return, and even got it wrong originally, fixing it in commit 0eb592dbba40 ("x86/paravirt: return full 64-bit result"), with: -#define PVOP_VCALLEE_CLOBBERS "=a" (__eax) +#define PVOP_VCALLEE_CLOBBERS "=a" (__eax), "=d" (__edx) But the naming is confusing, since these aren't actually clobbers, they're input constraints marked as clobbers (the "=" modifier). Regardless, the note in the comments ... ... * However, x86_64 also have to clobber all caller saved registers, which * unfortunately, are quite a bit (r8 - r11) ... ... would indicate that ALL the function argument registers need to be marked as clobbers (i.e. the compiler can't figure this out on its own). I was going to say it seems like they're missing from EXTRA_CLOBBERS, but it's not used with any of the macros using PVOP_VCALLEE_CLOBBERS, and then I saw the weird alternatives patching that encodes the clobbers a second time (CLBR_ANY vs CLBR_RET_REG) via: #define _paravirt_alt(insn_string, type, clobber) \ "771:\n\t" insn_string "\n" "772:\n" \ ".pushsection .parainstructions,\"a\"\n" \ _ASM_ALIGN "\n" \ _ASM_PTR " 771b\n" \ " .byte " type "\n" \ " .byte 772b-771b\n" \ " .short " clobber "\n" \ ".popsection\n" And after reading the alternatives patching code which parses this via the following struct: /* These all sit in the .parainstructions section to tell us what to patch. */ struct paravirt_patch_site { u8 *instr; /* original instructions */ u8 type; /* type of this instruction */ u8 len; /* length of original instruction */ }; ... I see it _doesn't use the clobbers_ at all! *head explode* I found that removal in commit 27876f3882fd ("x86/paravirt: Remove clobbers from struct paravirt_patch_site") So, I guess the CLBR_* can all be entirely removed. But back to my other train of thought... It seems like all the input registers need to be explicitly listed in the PVOP_VCALLEE_CLOBBERS list (as you have), but likely should be done unconditionally and for 32-bit as well. -Kees (Also, please CC linux-hardening@vger.kernel.org.) -- Kees Cook _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <CAGG=3QXpK+bFOSYZkdNNFGzNfgJSSADGTRWYRv6z0vfBAgQvWQ@mail.gmail.com>]
* Re: [PATCH 2/2] x86/paravirt: add extra clobbers with ZERO_CALL_USED_REGS enabled [not found] ` <CAGG=3QXpK+bFOSYZkdNNFGzNfgJSSADGTRWYRv6z0vfBAgQvWQ@mail.gmail.com> @ 2022-09-07 6:00 ` Nick Desaulniers via Virtualization 2022-09-07 8:50 ` Peter Zijlstra 0 siblings, 1 reply; 6+ messages in thread From: Nick Desaulniers via Virtualization @ 2022-09-07 6:00 UTC (permalink / raw) To: Bill Wendling Cc: Juergen Gross, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), H. Peter Anvin, Kees Cook, VMware PV-Drivers Reviewers, Dave Hansen, clang-built-linux, LKML, virtualization, Nathan Chancellor, Ingo Molnar, Borislav Petkov, linux-hardening, Alexey Makhalov, Thomas Gleixner On Sun, Sep 4, 2022 at 11:02 PM Bill Wendling <morbo@google.com> wrote: > > On Sat, Sep 3, 2022 at 12:18 AM Kees Cook <keescook@chromium.org> wrote: > > > > On Fri, Sep 02, 2022 at 09:37:50PM +0000, Bill Wendling wrote: > > > [...] > > > callq *pv_ops+536(%rip) > > > > Do you know which pv_ops function is this? I can't figure out where > > pte_offset_kernel() gets converted into a pv_ops call.... > > > This one is _paravirt_ident_64, I believe. I think that the original > issue Nathan was seeing was with another seemingly innocuous function. _paravirt_ident_64 is marked noinstr, which makes me suspect that it really needs to not be touched at all by the compiler for these...special features. Maybe the definition of noinstr in include/linux/compiler_types.h should be adding __attribute__((zero_call_used_regs(skip)))? Untested: ``` diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h index 4f2a819fd60a..a51ab77e2da8 100644 --- a/include/linux/compiler_types.h +++ b/include/linux/compiler_types.h @@ -226,10 +226,17 @@ struct ftrace_likely_data { #define __no_sanitize_or_inline __always_inline #endif +#ifdef CONFIG_ZERO_CALL_USED_REGS +#define __no_zero_call_used_regs __attribute__((__zero_call_used_reg__(skip))) +#else +#define __no_zero_call_used_regs +#endif + /* Section for code which can't be instrumented at all */ #define noinstr \ noinline notrace __attribute((__section__(".noinstr.text"))) \ - __no_kcsan __no_sanitize_address __no_profile __no_sanitize_coverage + __no_kcsan __no_sanitize_address __no_profile \ + __no_sanitize_coverage __no_zero_call_used_regs #endif /* __KERNEL__ */ ``` Or use __has_attribute in include/linux/compiler_attributes.h. -- Thanks, ~Nick Desaulniers _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] x86/paravirt: add extra clobbers with ZERO_CALL_USED_REGS enabled 2022-09-07 6:00 ` Nick Desaulniers via Virtualization @ 2022-09-07 8:50 ` Peter Zijlstra 2022-09-07 23:10 ` Kees Cook 0 siblings, 1 reply; 6+ messages in thread From: Peter Zijlstra @ 2022-09-07 8:50 UTC (permalink / raw) To: Nick Desaulniers Cc: Juergen Gross, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), H. Peter Anvin, Kees Cook, VMware PV-Drivers Reviewers, Dave Hansen, clang-built-linux, LKML, virtualization, Nathan Chancellor, Ingo Molnar, Borislav Petkov, Bill Wendling, Alexey Makhalov, Thomas Gleixner, linux-hardening On Tue, Sep 06, 2022 at 11:00:07PM -0700, Nick Desaulniers wrote: > On Sun, Sep 4, 2022 at 11:02 PM Bill Wendling <morbo@google.com> wrote: > > > > On Sat, Sep 3, 2022 at 12:18 AM Kees Cook <keescook@chromium.org> wrote: > > > > > > On Fri, Sep 02, 2022 at 09:37:50PM +0000, Bill Wendling wrote: > > > > [...] > > > > callq *pv_ops+536(%rip) > > > > > > Do you know which pv_ops function is this? I can't figure out where > > > pte_offset_kernel() gets converted into a pv_ops call.... > > > > > This one is _paravirt_ident_64, I believe. I think that the original > > issue Nathan was seeing was with another seemingly innocuous function. > > _paravirt_ident_64 is marked noinstr, which makes me suspect that it > really needs to not be touched at all by the compiler for > these...special features. My source tree sayeth: u64 notrace _paravirt_ident_64(u64 x) And that function is only ever called at boot, after alternatives runs it's patched with: mov %_ASM_ARG1, %_ASM_AX Anyway, if you want to take it away from the compiler, something like so should do. diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c index 7ca2d46c08cc..8922e2887779 100644 --- a/arch/x86/kernel/paravirt.c +++ b/arch/x86/kernel/paravirt.c @@ -80,11 +80,16 @@ static unsigned paravirt_patch_call(void *insn_buff, const void *target, } #ifdef CONFIG_PARAVIRT_XXL -/* identity function, which can be inlined */ -u64 notrace _paravirt_ident_64(u64 x) -{ - return x; -} +extern u64 _paravirt_ident_64(u64 x); +asm (".pushsection .entry.text, \"ax\"\n" + ".global _paravirt_ident_64\n" + "_paravirt_ident_64:\n\t" + ASM_ENDBR + "mov %" _ASM_ARG1 ", %" _ASM_AX "\n\t" + ASM_RET + ".size _paravirt_ident_64, . - _paravirt_ident_64\n\t" + ".type _paravirt_ident_64, @function\n\t" + ".popsection"); #endif DEFINE_STATIC_KEY_TRUE(virt_spin_lock_key); _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] x86/paravirt: add extra clobbers with ZERO_CALL_USED_REGS enabled 2022-09-07 8:50 ` Peter Zijlstra @ 2022-09-07 23:10 ` Kees Cook 0 siblings, 0 replies; 6+ messages in thread From: Kees Cook @ 2022-09-07 23:10 UTC (permalink / raw) To: Peter Zijlstra Cc: Juergen Gross, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), H. Peter Anvin, VMware PV-Drivers Reviewers, Dave Hansen, Nick Desaulniers, LKML, virtualization, Nathan Chancellor, Ingo Molnar, Borislav Petkov, Bill Wendling, clang-built-linux, Alexey Makhalov, Thomas Gleixner, linux-hardening On Wed, Sep 07, 2022 at 10:50:03AM +0200, Peter Zijlstra wrote: > On Tue, Sep 06, 2022 at 11:00:07PM -0700, Nick Desaulniers wrote: > > On Sun, Sep 4, 2022 at 11:02 PM Bill Wendling <morbo@google.com> wrote: > > > > > > On Sat, Sep 3, 2022 at 12:18 AM Kees Cook <keescook@chromium.org> wrote: > > > > > > > > On Fri, Sep 02, 2022 at 09:37:50PM +0000, Bill Wendling wrote: > > > > > [...] > > > > > callq *pv_ops+536(%rip) > > > > > > > > Do you know which pv_ops function is this? I can't figure out where > > > > pte_offset_kernel() gets converted into a pv_ops call.... > > > > > > > This one is _paravirt_ident_64, I believe. I think that the original > > > issue Nathan was seeing was with another seemingly innocuous function. > > > > _paravirt_ident_64 is marked noinstr, which makes me suspect that it > > really needs to not be touched at all by the compiler for > > these...special features. > > My source tree sayeth: > > u64 notrace _paravirt_ident_64(u64 x) I don't see noinstr either. But it seems a reasonable thing to do? Bill, does fixing up the noinstr macro and adding it here fix the problem? -- Kees Cook _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <20220914162149.71271-1-morbo@google.com>]
* Re: [PATCH v2 1/1] x86/paravirt: write paravirt ident function in assembly [not found] ` <20220914162149.71271-1-morbo@google.com> @ 2022-09-15 6:59 ` Juergen Gross via Virtualization 0 siblings, 0 replies; 6+ messages in thread From: Juergen Gross via Virtualization @ 2022-09-15 6:59 UTC (permalink / raw) To: Bill Wendling, linux-hardening Cc: x86, H. Peter Anvin, Kees Cook, VMware PV-Drivers Reviewers, Dave Hansen, Nick Desaulniers, linux-kernel, virtualization, Nathan Chancellor, Peter Zijlstra, Ingo Molnar, Borislav Petkov, llvm, Alexey Makhalov, Thomas Gleixner [-- Attachment #1.1.1.1: Type: text/plain, Size: 2837 bytes --] On 14.09.22 18:21, Bill Wendling wrote: > The ZERO_CALL_USED_REGS feature may zero out caller-saved registers > before returning. However, alternate code may call this function without > first saving %[re]di, because the proper clobbers aren't taken into > account. > > This shows up in spurious_kernel_fault() where the "pte_offset_kernel()" > call results in this assembly code: > > .Ltmp151: > #APP > # ALT: oldnstr > .Ltmp152: > .Ltmp153: > .Ltmp154: > .section .discard.retpoline_safe,"",@progbits > .quad .Ltmp154 > .text > > callq *pv_ops+536(%rip) > > .Ltmp155: > .section .parainstructions,"a",@progbits > .p2align 3, 0x0 > .quad .Ltmp153 > .byte 67 > .byte .Ltmp155-.Ltmp153 > .short 1 > .text > .Ltmp156: > # ALT: padding > .zero (-(((.Ltmp157-.Ltmp158)-(.Ltmp156-.Ltmp152))>0))*((.Ltmp157-.Ltmp158)-(.Ltmp156-.Ltmp152)),144 > .Ltmp159: > .section .altinstructions,"a",@progbits > .Ltmp160: > .long .Ltmp152-.Ltmp160 > .Ltmp161: > .long .Ltmp158-.Ltmp161 > .short 33040 > .byte .Ltmp159-.Ltmp152 > .byte .Ltmp157-.Ltmp158 > .text > > .section .altinstr_replacement,"ax",@progbits > # ALT: replacement 1 > .Ltmp158: > movq %rdi, %rax > .Ltmp157: > .text > #NO_APP > .Ltmp162: > testb $-128, %dil > > The %dil register was zeroed out by the call to "*pv_ops+536(%rip)". > > In general, the _paravirt_ident_64() function appears like it shouldn't > have any instrumentation or other modifications applied to it. Thus just > write it in assembly to avoid having to continually modify it whenever a > new feature comes along. > > Link: https://github.com/KSPP/linux/issues/192 > Cc: Kees Cook <keescook@chromium.org> > Cc: Nick Desaulniers <ndesaulniers@google.com> > Cc: Juergen Gross <jgross@suse.com> > Cc: "Srivatsa S. Bhat (VMware)" <srivatsa@csail.mit.edu> > Cc: Alexey Makhalov <amakhalov@vmware.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Borislav Petkov <bp@alien8.de> > Cc: Dave Hansen <dave.hansen@linux.intel.com> > Cc: "H. Peter Anvin" <hpa@zytor.com> > Cc: VMware PV-Drivers Reviewers <pv-drivers@vmware.com> > Cc: x86@kernel.org > Cc: virtualization@lists.linux-foundation.org > Cc: linux-kernel@vger.kernel.org > Cc: llvm@lists.linux.dev > Signed-off-by: Bill Wendling <morbo@google.com> > Suggested-by: Peter Zijlstra <peterz@infradead.org> > Reported-and-tested-by: Nathan Chancellor <nathan@kernel.org> Reviewed-by: Juergen Gross <jgross@suse.com> Juergen [-- Attachment #1.1.1.2: OpenPGP public key --] [-- Type: application/pgp-keys, Size: 3149 bytes --] [-- Attachment #1.2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 495 bytes --] [-- Attachment #2: Type: text/plain, Size: 183 bytes --] _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-09-15 6:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20220902213750.1124421-1-morbo@google.com>
[not found] ` <20220902213750.1124421-2-morbo@google.com>
2022-09-03 4:28 ` [PATCH 1/2] x86/paravirt: clean up typos and grammaros Borislav Petkov
[not found] ` <20220902213750.1124421-3-morbo@google.com>
2022-09-03 7:18 ` [PATCH 2/2] x86/paravirt: add extra clobbers with ZERO_CALL_USED_REGS enabled Kees Cook
[not found] ` <CAGG=3QXpK+bFOSYZkdNNFGzNfgJSSADGTRWYRv6z0vfBAgQvWQ@mail.gmail.com>
2022-09-07 6:00 ` Nick Desaulniers via Virtualization
2022-09-07 8:50 ` Peter Zijlstra
2022-09-07 23:10 ` Kees Cook
[not found] ` <20220914162149.71271-1-morbo@google.com>
2022-09-15 6:59 ` [PATCH v2 1/1] x86/paravirt: write paravirt ident function in assembly Juergen Gross via Virtualization
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox