Virtualization - Sep 2022 - [PATCH 2/2] x86/paravirt: add extra clobbers with ZERO_CALL_USED_REGS enabled

On Sun, Sep 4, 2022 at 11:02 PM Bill Wendling <morbo at google.com>
wrote:
>
> On Sat, Sep 3, 2022 at 12:18 AM Kees Cook <keescook at 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

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 at google.com>
wrote:
> >
> > On Sat, Sep 3, 2022 at 12:18 AM Kees Cook <keescook at
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);