All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] fix clobbers list with ZERO_CALL_USED_REGS feature
@ 2022-09-02 21:37 Bill Wendling
  2022-09-02 21:37 ` [PATCH 1/2] x86/paravirt: clean up typos and grammaros Bill Wendling
                   ` (2 more replies)
  0 siblings, 3 replies; 20+ messages in thread
From: Bill Wendling @ 2022-09-02 21:37 UTC (permalink / raw)
  To: Juergen Gross, Srivatsa S. Bhat (VMware),
	Alexey Makhalov, VMware PV-Drivers Reviewers, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
	virtualization, linux-kernel, Nathan Chancellor,
	Nick Desaulniers, llvm
  Cc: Bill Wendling

The ZERO_CALL_USED_REGS feature may zero out callee-saved registers. This needs
to be properly modeled by things like code alternatives. Otherwise, it's
possible that a callee-saved register would be expected to remain unchanged
past an ASM statement when in reality it isn't.

Bill Wendling (2):
  x86/paravirt: clean up typos and grammaros
  x86/paravirt: add extra clobbers with ZERO_CALL_USED_REGS enabled

 arch/x86/include/asm/paravirt_types.h | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

-- 
2.37.2.789.g6183377224-goog


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 1/2] x86/paravirt: clean up typos and grammaros
  2022-09-02 21:37 [PATCH 0/2] fix clobbers list with ZERO_CALL_USED_REGS feature Bill Wendling
@ 2022-09-02 21:37 ` Bill Wendling
  2022-09-03  4:28     ` Borislav Petkov
  2022-09-02 21:37 ` [PATCH 2/2] x86/paravirt: add extra clobbers with ZERO_CALL_USED_REGS enabled Bill Wendling
       [not found] ` <20220914162149.71271-1-morbo@google.com>
  2 siblings, 1 reply; 20+ messages in thread
From: Bill Wendling @ 2022-09-02 21:37 UTC (permalink / raw)
  To: Juergen Gross, Srivatsa S. Bhat (VMware),
	Alexey Makhalov, VMware PV-Drivers Reviewers, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
	virtualization, linux-kernel, Nathan Chancellor,
	Nick Desaulniers, llvm
  Cc: Bill Wendling

Drive-by clean up of the comment.

[ Impact: cleanup]

Signed-off-by: Bill Wendling <morbo@google.com>
---
 arch/x86/include/asm/paravirt_types.h | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
index 89df6c6617f5..f04157456a49 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -328,7 +328,7 @@ int paravirt_disable_iospace(void);
  * Unfortunately, this is a relatively slow operation for modern CPUs,
  * because it cannot necessarily determine what the destination
  * address is.  In this case, the address is a runtime constant, so at
- * the very least we can patch the call to e a simple direct call, or
+ * the very least we can patch the call to a simple direct call, or,
  * ideally, patch an inline implementation into the callsite.  (Direct
  * calls are essentially free, because the call and return addresses
  * are completely predictable.)
@@ -339,10 +339,10 @@ int paravirt_disable_iospace(void);
  * on the stack.  All caller-save registers (eax,edx,ecx) are expected
  * to be modified (either clobbered or used for return values).
  * X86_64, on the other hand, already specifies a register-based calling
- * conventions, returning at %rax, with parameters going on %rdi, %rsi,
+ * conventions, returning at %rax, with parameters going in %rdi, %rsi,
  * %rdx, and %rcx. Note that for this reason, x86_64 does not need any
  * special handling for dealing with 4 arguments, unlike i386.
- * However, x86_64 also have to clobber all caller saved registers, which
+ * However, x86_64 also has to clobber all caller saved registers, which
  * unfortunately, are quite a bit (r8 - r11)
  *
  * The call instruction itself is marked by placing its start address
@@ -360,22 +360,22 @@ int paravirt_disable_iospace(void);
  * There are 5 sets of PVOP_* macros for dealing with 0-4 arguments.
  * It could be extended to more arguments, but there would be little
  * to be gained from that.  For each number of arguments, there are
- * the two VCALL and CALL variants for void and non-void functions.
+ * two VCALL and CALL variants for void and non-void functions.
  *
  * When there is a return value, the invoker of the macro must specify
  * the return type.  The macro then uses sizeof() on that type to
- * determine whether its a 32 or 64 bit value, and places the return
+ * determine whether it's a 32 or 64 bit value and places the return
  * in the right register(s) (just %eax for 32-bit, and %edx:%eax for
- * 64-bit). For x86_64 machines, it just returns at %rax regardless of
+ * 64-bit). For x86_64 machines, it just returns in %rax regardless of
  * the return value size.
  *
- * 64-bit arguments are passed as a pair of adjacent 32-bit arguments
+ * 64-bit arguments are passed as a pair of adjacent 32-bit arguments;
  * i386 also passes 64-bit arguments as a pair of adjacent 32-bit arguments
  * in low,high order
  *
  * Small structures are passed and returned in registers.  The macro
  * calling convention can't directly deal with this, so the wrapper
- * functions must do this.
+ * functions must do it.
  *
  * These PVOP_* macros are only defined within this header.  This
  * means that all uses must be wrapped in inline functions.  This also
-- 
2.37.2.789.g6183377224-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 2/2] x86/paravirt: add extra clobbers with ZERO_CALL_USED_REGS enabled
  2022-09-02 21:37 [PATCH 0/2] fix clobbers list with ZERO_CALL_USED_REGS feature Bill Wendling
  2022-09-02 21:37 ` [PATCH 1/2] x86/paravirt: clean up typos and grammaros Bill Wendling
@ 2022-09-02 21:37 ` Bill Wendling
  2022-09-03  7:18     ` Kees Cook
       [not found] ` <20220914162149.71271-1-morbo@google.com>
  2 siblings, 1 reply; 20+ messages in thread
From: Bill Wendling @ 2022-09-02 21:37 UTC (permalink / raw)
  To: Juergen Gross, Srivatsa S. Bhat (VMware),
	Alexey Makhalov, VMware PV-Drivers Reviewers, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
	virtualization, linux-kernel, Nathan Chancellor,
	Nick Desaulniers, llvm
  Cc: Bill Wendling

The ZERO_CALL_USED_REGS feature may zero out caller-saved registers
before returning.

In spurious_kernel_fault(), 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 "testb" here is using %dil, but the %rdi register was cleared before
returning from "callq *pv_ops+536(%rip)". Adding the proper constraints
results in the use of a different register:

        movq    %r11, %rdi

        # Similar to above.

        testb   $-128, %r11b

Link: https://github.com/KSPP/linux/issues/192
Signed-off-by: Bill Wendling <morbo@google.com>
Reported-and-tested-by: Nathan Chancellor <nathan@kernel.org>
---
 arch/x86/include/asm/paravirt_types.h | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
index f04157456a49..b1ab5d94881b 100644
--- 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
 
 #define EXTRA_CLOBBERS	 , "r8", "r9", "r10", "r11"
-- 
2.37.2.789.g6183377224-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/2] x86/paravirt: clean up typos and grammaros
  2022-09-02 21:37 ` [PATCH 1/2] x86/paravirt: clean up typos and grammaros Bill Wendling
@ 2022-09-03  4:28     ` Borislav Petkov
  0 siblings, 0 replies; 20+ messages in thread
From: Borislav Petkov @ 2022-09-03  4:28 UTC (permalink / raw)
  To: Bill Wendling
  Cc: Juergen Gross, Srivatsa S. Bhat (VMware),
	Alexey Makhalov, VMware PV-Drivers Reviewers, Thomas Gleixner,
	Ingo Molnar, Dave Hansen, x86, H. Peter Anvin, virtualization,
	linux-kernel, Nathan Chancellor, Nick Desaulniers, llvm

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/2] x86/paravirt: clean up typos and grammaros
@ 2022-09-03  4:28     ` Borislav Petkov
  0 siblings, 0 replies; 20+ 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] 20+ messages in thread

* Re: [PATCH 2/2] x86/paravirt: add extra clobbers with ZERO_CALL_USED_REGS enabled
  2022-09-02 21:37 ` [PATCH 2/2] x86/paravirt: add extra clobbers with ZERO_CALL_USED_REGS enabled Bill Wendling
@ 2022-09-03  7:18     ` Kees Cook
  0 siblings, 0 replies; 20+ messages in thread
From: Kees Cook @ 2022-09-03  7:18 UTC (permalink / raw)
  To: Bill Wendling
  Cc: Juergen Gross, Srivatsa S. Bhat (VMware),
	Alexey Makhalov, VMware PV-Drivers Reviewers, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
	virtualization, linux-kernel, Nathan Chancellor,
	Nick Desaulniers, llvm, linux-hardening

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] x86/paravirt: add extra clobbers with ZERO_CALL_USED_REGS enabled
@ 2022-09-03  7:18     ` Kees Cook
  0 siblings, 0 replies; 20+ 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] 20+ messages in thread

* Re: [PATCH 1/2] x86/paravirt: clean up typos and grammaros
  2022-09-03  4:28     ` Borislav Petkov
  (?)
@ 2022-09-04  2:13     ` Bill Wendling
  -1 siblings, 0 replies; 20+ messages in thread
From: Bill Wendling @ 2022-09-04  2:13 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Juergen Gross, Srivatsa S. Bhat (VMware),
	Alexey Makhalov, VMware PV-Drivers Reviewers, Thomas Gleixner,
	Ingo Molnar, Dave Hansen,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	H. Peter Anvin, virtualization, LKML, Nathan Chancellor,
	Nick Desaulniers, clang-built-linux

On Fri, Sep 2, 2022 at 9:28 PM Borislav Petkov <bp@alien8.de> wrote:
>
> 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?
>
> :)
>
It was in a comment from that file. :-)

-bw

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] x86/paravirt: add extra clobbers with ZERO_CALL_USED_REGS enabled
  2022-09-03  7:18     ` Kees Cook
  (?)
@ 2022-09-05  6:02     ` Bill Wendling
  2022-09-07  6:00         ` Nick Desaulniers via Virtualization
  -1 siblings, 1 reply; 20+ messages in thread
From: Bill Wendling @ 2022-09-05  6:02 UTC (permalink / raw)
  To: Kees Cook
  Cc: Juergen Gross, Srivatsa S. Bhat (VMware),
	Alexey Makhalov, VMware PV-Drivers Reviewers, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, Dave Hansen,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	H. Peter Anvin, virtualization, LKML, Nathan Chancellor,
	Nick Desaulniers, clang-built-linux, linux-hardening

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.

> > [...]
> > --- 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?
>
Kinda. It seems that the usage here is to let the compiler know that a
register may be modified by the callee, not just that it's an "actual"
return value. So it's suitable for void functions.

> 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).
>
Right.

> 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).
>
Good point. And there are some forms of these macros that specify
those as clobbers.

> 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...
>
[switches stations]

> 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.
>
Possibly, though it may cause significant code degradation when the
compiler needs to store a value that's live over the ASM statement,
but the register it's in isn't actually modified. I saw that in the
example I gave in the description. In the case where a "movq" is used,
there's a useless move of "rdi" into "r11".

> (Also, please CC linux-hardening@vger.kernel.org.)
>
Doh! Someday I'll learn email.

-bw

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] x86/paravirt: add extra clobbers with ZERO_CALL_USED_REGS enabled
  2022-09-05  6:02     ` Bill Wendling
@ 2022-09-07  6:00         ` Nick Desaulniers via Virtualization
  0 siblings, 0 replies; 20+ messages in thread
From: Nick Desaulniers @ 2022-09-07  6:00 UTC (permalink / raw)
  To: Bill Wendling
  Cc: Kees Cook, Juergen Gross, Srivatsa S. Bhat (VMware),
	Alexey Makhalov, VMware PV-Drivers Reviewers, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, Dave Hansen,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	H. Peter Anvin, virtualization, LKML, Nathan Chancellor,
	clang-built-linux, linux-hardening

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

^ permalink raw reply related	[flat|nested] 20+ 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
  0 siblings, 0 replies; 20+ 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] 20+ 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
  -1 siblings, 0 replies; 20+ messages in thread
From: Peter Zijlstra @ 2022-09-07  8:50 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Bill Wendling, Kees Cook, Juergen Gross,
	Srivatsa S. Bhat (VMware),
	Alexey Makhalov, VMware PV-Drivers Reviewers, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, Dave Hansen,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	H. Peter Anvin, virtualization, LKML, Nathan Chancellor,
	clang-built-linux, 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);

^ permalink raw reply related	[flat|nested] 20+ 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
  0 siblings, 0 replies; 20+ 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] 20+ 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
  -1 siblings, 0 replies; 20+ messages in thread
From: Kees Cook @ 2022-09-07 23:10 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Nick Desaulniers, Bill Wendling, Juergen Gross,
	Srivatsa S. Bhat (VMware),
	Alexey Makhalov, VMware PV-Drivers Reviewers, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, Dave Hansen,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	H. Peter Anvin, virtualization, LKML, Nathan Chancellor,
	clang-built-linux, 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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] x86/paravirt: add extra clobbers with ZERO_CALL_USED_REGS enabled
@ 2022-09-07 23:10             ` Kees Cook
  0 siblings, 0 replies; 20+ 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] 20+ messages in thread

* Re: [PATCH 2/2] x86/paravirt: add extra clobbers with ZERO_CALL_USED_REGS enabled
  2022-09-07 23:10             ` Kees Cook
  (?)
@ 2022-09-08 21:16             ` Bill Wendling
  -1 siblings, 0 replies; 20+ messages in thread
From: Bill Wendling @ 2022-09-08 21:16 UTC (permalink / raw)
  To: Kees Cook
  Cc: Peter Zijlstra, Nick Desaulniers, Juergen Gross,
	Srivatsa S. Bhat (VMware),
	Alexey Makhalov, VMware PV-Drivers Reviewers, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, Dave Hansen,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	H. Peter Anvin, virtualization, LKML, Nathan Chancellor,
	clang-built-linux, linux-hardening

On Thu, Sep 8, 2022 at 12:10 AM Kees Cook <keescook@chromium.org> wrote:
>
> 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?
>
[sorry for late response]

Let me give it a shot. I'll also test out Peter's suggestion, which
might be a better option in the long run. I suspect that we'll need to
devise similar patches for other places, but it shouldn't be hard to
find them all.

-bw

-bw

^ permalink raw reply	[flat|nested] 20+ 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-14 14:40           ` Nathan Chancellor
  2022-09-14 15:49             ` Bill Wendling
  -1 siblings, 1 reply; 20+ messages in thread
From: Nathan Chancellor @ 2022-09-14 14:40 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Nick Desaulniers, Bill Wendling, Kees Cook, Juergen Gross,
	Srivatsa S. Bhat (VMware),
	Alexey Makhalov, VMware PV-Drivers Reviewers, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, Dave Hansen,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	H. Peter Anvin, virtualization, LKML, clang-built-linux,
	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)
> 
> 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.

This appears to work fine for me in QEMU, as I can still boot with
CONFIG_ZERO_CALL_USED_REGS and spawn a nested guest without any issues.

> 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);
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] x86/paravirt: add extra clobbers with ZERO_CALL_USED_REGS enabled
  2022-09-14 14:40           ` Nathan Chancellor
@ 2022-09-14 15:49             ` Bill Wendling
  0 siblings, 0 replies; 20+ messages in thread
From: Bill Wendling @ 2022-09-14 15:49 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Peter Zijlstra, Nick Desaulniers, Kees Cook, Juergen Gross,
	Srivatsa S. Bhat (VMware),
	Alexey Makhalov, VMware PV-Drivers Reviewers, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, Dave Hansen,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	H. Peter Anvin, virtualization, LKML, clang-built-linux,
	linux-hardening

On Wed, Sep 14, 2022 at 3:41 PM Nathan Chancellor <nathan@kernel.org> wrote:
> 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)
> >
> > 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.
>
> This appears to work fine for me in QEMU, as I can still boot with
> CONFIG_ZERO_CALL_USED_REGS and spawn a nested guest without any issues.
>
Thanks, Nathan. I much prefer to use this patch then and file a
separate issue to investigate the clobbers issue for later.

-bw

> > 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);
> >

^ permalink raw reply	[flat|nested] 20+ messages in thread

* 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
  0 siblings, 0 replies; 20+ 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] 20+ messages in thread

* Re: [PATCH v2 1/1] x86/paravirt: write paravirt ident function in assembly
@ 2022-09-15  6:59     ` Juergen Gross
  0 siblings, 0 replies; 20+ messages in thread
From: Juergen Gross @ 2022-09-15  6:59 UTC (permalink / raw)
  To: Bill Wendling, linux-hardening
  Cc: Kees Cook, Nick Desaulniers, Srivatsa S. Bhat (VMware),
	Alexey Makhalov, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, H. Peter Anvin, VMware PV-Drivers Reviewers, x86,
	virtualization, linux-kernel, llvm, Peter Zijlstra,
	Nathan Chancellor


[-- Attachment #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.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2022-09-15  6:59 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-02 21:37 [PATCH 0/2] fix clobbers list with ZERO_CALL_USED_REGS feature Bill Wendling
2022-09-02 21:37 ` [PATCH 1/2] x86/paravirt: clean up typos and grammaros Bill Wendling
2022-09-03  4:28   ` Borislav Petkov
2022-09-03  4:28     ` Borislav Petkov
2022-09-04  2:13     ` Bill Wendling
2022-09-02 21:37 ` [PATCH 2/2] x86/paravirt: add extra clobbers with ZERO_CALL_USED_REGS enabled Bill Wendling
2022-09-03  7:18   ` Kees Cook
2022-09-03  7:18     ` Kees Cook
2022-09-05  6:02     ` Bill Wendling
2022-09-07  6:00       ` Nick Desaulniers
2022-09-07  6:00         ` Nick Desaulniers via Virtualization
2022-09-07  8:50         ` Peter Zijlstra
2022-09-07  8:50           ` Peter Zijlstra
2022-09-07 23:10           ` Kees Cook
2022-09-07 23:10             ` Kees Cook
2022-09-08 21:16             ` Bill Wendling
2022-09-14 14:40           ` Nathan Chancellor
2022-09-14 15:49             ` Bill Wendling
     [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
2022-09-15  6:59     ` Juergen Gross

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.