[PATCH v2 0/7] x86: Further harden function pointers

[PATCH v2 0/7] x86: Further harden function pointers

[Andrew Cooper]

Additional runtime hardning of indirect branches.  Depends on the CET-IBT
series.

Andrew Cooper (7):
  xen/altcall: Use __ro_after_init now that it exists
  x86/altcall: Check and optimise altcall targets
  x86/altcall: Optimise away endbr64 instruction where possible
  xsm: Use __initconst_cf_clobber for xsm_ops
  x86/hvm: Use __initdata_cf_clobber for hvm_funcs
  x86/ucode: Use altcall, and __initconst_cf_clobber
  x86/vpmu: Harden indirect branches

 xen/arch/x86/alternative.c         | 61 ++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/cpu/microcode/amd.c   |  2 +-
 xen/arch/x86/cpu/microcode/core.c  | 38 +++++++++++++-----------
 xen/arch/x86/cpu/microcode/intel.c |  2 +-
 xen/arch/x86/cpu/vpmu_amd.c        |  2 +-
 xen/arch/x86/cpu/vpmu_intel.c      |  2 +-
 xen/arch/x86/hvm/hvm.c             |  2 +-
 xen/arch/x86/hvm/svm/svm.c         |  2 +-
 xen/arch/x86/hvm/vmx/vmx.c         |  2 +-
 xen/arch/x86/xen.lds.S             |  6 ++++
 xen/include/xen/alternative-call.h |  2 +-
 xen/include/xen/init.h             |  3 ++
 xen/xsm/dummy.c                    |  2 +-
 xen/xsm/flask/hooks.c              |  2 +-
 xen/xsm/silo.c                     |  2 +-
 15 files changed, 101 insertions(+), 29 deletions(-)

-- 
2.11.0

[PATCH v2 1/7] xen/altcall: Use __ro_after_init now that it exists

[Andrew Cooper]

For the !CONFIG_ALTERNATIVE_CALL case, the use of __read_mostly was only a
stopgap while nothing better existed.  __ro_after_init now does, so it use.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

v2:
 * New
---
 xen/include/xen/alternative-call.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/xen/alternative-call.h b/xen/include/xen/alternative-call.h
index c2d3b70e312e..5c6b9a562b92 100644
--- a/xen/include/xen/alternative-call.h
+++ b/xen/include/xen/alternative-call.h
@@ -57,7 +57,7 @@
 #define alternative_call(func, args...)  (func)(args)
 #define alternative_vcall(func, args...) (func)(args)
 
-#define __alt_call_maybe_initdata __read_mostly
+#define __alt_call_maybe_initdata __ro_after_init
 
 #endif /* !CONFIG_ALTERNATIVE_CALL */
 #endif /* XEN_ALTERNATIVE_CALL */
-- 
2.11.0

Re: [PATCH v2 1/7] xen/altcall: Use __ro_after_init now that it exists

[Jan Beulich]

On 14.02.2022 13:56, Andrew Cooper wrote:
> For the !CONFIG_ALTERNATIVE_CALL case, the use of __read_mostly was only a
> stopgap while nothing better existed.  __ro_after_init now does, so it use.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

[PATCH v2 2/7] x86/altcall: Check and optimise altcall targets

[Andrew Cooper]

When converting indirect to direct calls, there is no need to execute endbr64
instructions.  Detect and optimise this case, leaving a warning in the case
that no endbr64 was found, as it likely indicates a build error.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/alternative.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index ec24692e9595..65537fe1f0bd 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -18,6 +18,7 @@
 #include <xen/delay.h>
 #include <xen/types.h>
 #include <asm/apic.h>
+#include <asm/endbr.h>
 #include <asm/processor.h>
 #include <asm/alternative.h>
 #include <xen/init.h>
@@ -279,6 +280,28 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start,
 
                 if ( dest )
                 {
+                    /*
+                     * When building for CET-IBT, all function pointer targets
+                     * should have an endbr64 instruction.
+                     *
+                     * If this is not the case, leave a warning because
+                     * something is probably wrong with the build.  A CET-IBT
+                     * enabled system might have exploded already.
+                     *
+                     * Otherwise, skip the endbr64 instruction.  This is a
+                     * marginal perf improvement which saves on instruction
+                     * decode bandwidth.
+                     */
+                    if ( IS_ENABLED(CONFIG_HAS_CC_CET_IBT) )
+                    {
+                        if ( is_endbr64(dest) )
+                            dest += 4;
+                        else
+                            printk(XENLOG_WARNING
+                                   "altcall %ps dest %ps has no endbr64\n",
+                                   orig, dest);
+                    }
+
                     disp = dest - (orig + 5);
                     ASSERT(disp == (int32_t)disp);
                     *(int32_t *)(buf + 1) = disp;
-- 
2.11.0

[PATCH v2 3/7] x86/altcall: Optimise away endbr64 instruction where possible

[Andrew Cooper]

With altcall, we convert indirect branches into direct ones.  With that
complete, none of the potential targets need an endbr64 instruction.

Furthermore, removing the endbr64 instructions is a security defence-in-depth
improvement, because it limits the options available to an attacker who has
managed to hijack a function pointer.

Introduce new .init.{ro,}data.cf_clobber sections.  Have _apply_alternatives()
walk over this, looking for any pointers into .text, and clobber an endbr64
instruction if found.  This is some minor structure (ab)use but it works
alarmingly well.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

It would be nice for the printk() to say "optimised away %u of %u", but the
latter number can only feasibly come from post-processing of xen-syms during
the build.

v2:
 * Drop hard tabs
 * Add __initconst_cf_clobber too
 * Change types to reduce casting
---
 xen/arch/x86/alternative.c | 38 ++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/xen.lds.S     |  6 ++++++
 xen/include/xen/init.h     |  3 +++
 3 files changed, 47 insertions(+)

diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index 65537fe1f0bd..dd4609070001 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -173,6 +173,9 @@ text_poke(void *addr, const void *opcode, size_t len)
     return memcpy(addr, opcode, len);
 }
 
+extern void *const __initdata_cf_clobber_start[];
+extern void *const __initdata_cf_clobber_end[];
+
 /*
  * Replace instructions with better alternatives for this CPU type.
  * This runs before SMP is initialized to avoid SMP problems with
@@ -330,6 +333,41 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start,
         add_nops(buf + a->repl_len, total_len - a->repl_len);
         text_poke(orig, buf, total_len);
     }
+
+    /*
+     * Clobber endbr64 instructions now that altcall has finished optimising
+     * all indirect branches to direct ones.
+     */
+    if ( force && cpu_has_xen_ibt )
+    {
+        void *const *val;
+        unsigned int clobbered = 0;
+
+        /*
+         * This is some minor structure (ab)use.  We walk the entire contents
+         * of .init.{ro,}data.cf_clobber as if it were an array of pointers.
+         *
+         * If the pointer points into .text, and at an endbr64 instruction,
+         * nop out the endbr64.  This causes the pointer to no longer be a
+         * legal indirect branch target under CET-IBT.  This is a
+         * defence-in-depth measure, to reduce the options available to an
+         * adversary who has managed to hijack a function pointer.
+         */
+        for ( val = __initdata_cf_clobber_start;
+              val < __initdata_cf_clobber_end;
+              val++ )
+        {
+            void *ptr = *val;
+
+            if ( !is_kernel_text(ptr) || !is_endbr64(ptr) )
+                continue;
+
+            add_nops(ptr, 4);
+            clobbered++;
+        }
+
+        printk("altcall: Optimised away %u endbr64 instructions\n", clobbered);
+    }
 }
 
 void init_or_livepatch apply_alternatives(struct alt_instr *start,
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index ca22e984f807..c399178ac123 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -221,6 +221,12 @@ SECTIONS
        *(.initcall1.init)
        __initcall_end = .;
 
+       . = ALIGN(POINTER_ALIGN);
+       __initdata_cf_clobber_start = .;
+       *(.init.data.cf_clobber)
+       *(.init.rodata.cf_clobber)
+       __initdata_cf_clobber_end = .;
+
        *(.init.data)
        *(.init.data.rel)
        *(.init.data.rel.*)
diff --git a/xen/include/xen/init.h b/xen/include/xen/init.h
index bfe789e93f6b..0af0e234ec80 100644
--- a/xen/include/xen/init.h
+++ b/xen/include/xen/init.h
@@ -18,6 +18,9 @@
 #define __init_call(lvl)  __used_section(".initcall" lvl ".init")
 #define __exit_call       __used_section(".exitcall.exit")
 
+#define __initdata_cf_clobber  __section(".init.data.cf_clobber")
+#define __initconst_cf_clobber __section(".init.rodata.cf_clobber")
+
 /* These macros are used to mark some functions or 
  * initialized data (doesn't apply to uninitialized data)
  * as `initialization' functions. The kernel can take this
-- 
2.11.0

Re: [PATCH v2 3/7] x86/altcall: Optimise away endbr64 instruction where possible

[Jan Beulich]

On 14.02.2022 13:56, Andrew Cooper wrote:
> With altcall, we convert indirect branches into direct ones.  With that
> complete, none of the potential targets need an endbr64 instruction.
> 
> Furthermore, removing the endbr64 instructions is a security defence-in-depth
> improvement, because it limits the options available to an attacker who has
> managed to hijack a function pointer.
> 
> Introduce new .init.{ro,}data.cf_clobber sections.  Have _apply_alternatives()
> walk over this, looking for any pointers into .text, and clobber an endbr64
> instruction if found.  This is some minor structure (ab)use but it works
> alarmingly well.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with two remarks, which ideally would be addressed by respective
small adjustments:

> @@ -330,6 +333,41 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start,
>          add_nops(buf + a->repl_len, total_len - a->repl_len);
>          text_poke(orig, buf, total_len);
>      }
> +
> +    /*
> +     * Clobber endbr64 instructions now that altcall has finished optimising
> +     * all indirect branches to direct ones.
> +     */
> +    if ( force && cpu_has_xen_ibt )
> +    {
> +        void *const *val;
> +        unsigned int clobbered = 0;
> +
> +        /*
> +         * This is some minor structure (ab)use.  We walk the entire contents
> +         * of .init.{ro,}data.cf_clobber as if it were an array of pointers.
> +         *
> +         * If the pointer points into .text, and at an endbr64 instruction,
> +         * nop out the endbr64.  This causes the pointer to no longer be a
> +         * legal indirect branch target under CET-IBT.  This is a
> +         * defence-in-depth measure, to reduce the options available to an
> +         * adversary who has managed to hijack a function pointer.
> +         */
> +        for ( val = __initdata_cf_clobber_start;
> +              val < __initdata_cf_clobber_end;
> +              val++ )
> +        {
> +            void *ptr = *val;
> +
> +            if ( !is_kernel_text(ptr) || !is_endbr64(ptr) )
> +                continue;
> +
> +            add_nops(ptr, 4);

This literal 4 would be nice to have a #define next to where the ENDBR64
encoding has its central place.

> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -221,6 +221,12 @@ SECTIONS
>         *(.initcall1.init)
>         __initcall_end = .;
>  
> +       . = ALIGN(POINTER_ALIGN);
> +       __initdata_cf_clobber_start = .;
> +       *(.init.data.cf_clobber)
> +       *(.init.rodata.cf_clobber)
> +       __initdata_cf_clobber_end = .;
> +
>         *(.init.data)
>         *(.init.data.rel)
>         *(.init.data.rel.*)

With r/o data ahead and r/w data following, may I suggest to flip the
order of the two section specifiers you add?

Jan

Re: [PATCH v2 3/7] x86/altcall: Optimise away endbr64 instruction where possible

[Andrew Cooper]

On 14/02/2022 13:06, Jan Beulich wrote:
> On 14.02.2022 13:56, Andrew Cooper wrote:
>> With altcall, we convert indirect branches into direct ones.  With that
>> complete, none of the potential targets need an endbr64 instruction.
>>
>> Furthermore, removing the endbr64 instructions is a security defence-in-depth
>> improvement, because it limits the options available to an attacker who has
>> managed to hijack a function pointer.
>>
>> Introduce new .init.{ro,}data.cf_clobber sections.  Have _apply_alternatives()
>> walk over this, looking for any pointers into .text, and clobber an endbr64
>> instruction if found.  This is some minor structure (ab)use but it works
>> alarmingly well.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks,

> with two remarks, which ideally would be addressed by respective
> small adjustments:
>
>> @@ -330,6 +333,41 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start,
>>          add_nops(buf + a->repl_len, total_len - a->repl_len);
>>          text_poke(orig, buf, total_len);
>>      }
>> +
>> +    /*
>> +     * Clobber endbr64 instructions now that altcall has finished optimising
>> +     * all indirect branches to direct ones.
>> +     */
>> +    if ( force && cpu_has_xen_ibt )
>> +    {
>> +        void *const *val;
>> +        unsigned int clobbered = 0;
>> +
>> +        /*
>> +         * This is some minor structure (ab)use.  We walk the entire contents
>> +         * of .init.{ro,}data.cf_clobber as if it were an array of pointers.
>> +         *
>> +         * If the pointer points into .text, and at an endbr64 instruction,
>> +         * nop out the endbr64.  This causes the pointer to no longer be a
>> +         * legal indirect branch target under CET-IBT.  This is a
>> +         * defence-in-depth measure, to reduce the options available to an
>> +         * adversary who has managed to hijack a function pointer.
>> +         */
>> +        for ( val = __initdata_cf_clobber_start;
>> +              val < __initdata_cf_clobber_end;
>> +              val++ )
>> +        {
>> +            void *ptr = *val;
>> +
>> +            if ( !is_kernel_text(ptr) || !is_endbr64(ptr) )
>> +                continue;
>> +
>> +            add_nops(ptr, 4);
> This literal 4 would be nice to have a #define next to where the ENDBR64
> encoding has its central place.

We don't have an encoding of ENDBR64 in a central place.

The best you can probably have is

#define ENDBR64_LEN 4

in endbr.h ?

>
>> --- a/xen/arch/x86/xen.lds.S
>> +++ b/xen/arch/x86/xen.lds.S
>> @@ -221,6 +221,12 @@ SECTIONS
>>         *(.initcall1.init)
>>         __initcall_end = .;
>>  
>> +       . = ALIGN(POINTER_ALIGN);
>> +       __initdata_cf_clobber_start = .;
>> +       *(.init.data.cf_clobber)
>> +       *(.init.rodata.cf_clobber)
>> +       __initdata_cf_clobber_end = .;
>> +
>>         *(.init.data)
>>         *(.init.data.rel)
>>         *(.init.data.rel.*)
> With r/o data ahead and r/w data following, may I suggest to flip the
> order of the two section specifiers you add?

I don't follow.  This is all initdata which is merged together into a
single section.

The only reason const data is split out in the first place is to appease
the toolchains, not because it makes a difference.

~Andrew

Re: [PATCH v2 3/7] x86/altcall: Optimise away endbr64 instruction where possible

[Jan Beulich]

On 14.02.2022 14:31, Andrew Cooper wrote:
> On 14/02/2022 13:06, Jan Beulich wrote:
>> On 14.02.2022 13:56, Andrew Cooper wrote:
>>> @@ -330,6 +333,41 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start,
>>>          add_nops(buf + a->repl_len, total_len - a->repl_len);
>>>          text_poke(orig, buf, total_len);
>>>      }
>>> +
>>> +    /*
>>> +     * Clobber endbr64 instructions now that altcall has finished optimising
>>> +     * all indirect branches to direct ones.
>>> +     */
>>> +    if ( force && cpu_has_xen_ibt )
>>> +    {
>>> +        void *const *val;
>>> +        unsigned int clobbered = 0;
>>> +
>>> +        /*
>>> +         * This is some minor structure (ab)use.  We walk the entire contents
>>> +         * of .init.{ro,}data.cf_clobber as if it were an array of pointers.
>>> +         *
>>> +         * If the pointer points into .text, and at an endbr64 instruction,
>>> +         * nop out the endbr64.  This causes the pointer to no longer be a
>>> +         * legal indirect branch target under CET-IBT.  This is a
>>> +         * defence-in-depth measure, to reduce the options available to an
>>> +         * adversary who has managed to hijack a function pointer.
>>> +         */
>>> +        for ( val = __initdata_cf_clobber_start;
>>> +              val < __initdata_cf_clobber_end;
>>> +              val++ )
>>> +        {
>>> +            void *ptr = *val;
>>> +
>>> +            if ( !is_kernel_text(ptr) || !is_endbr64(ptr) )
>>> +                continue;
>>> +
>>> +            add_nops(ptr, 4);
>> This literal 4 would be nice to have a #define next to where the ENDBR64
>> encoding has its central place.
> 
> We don't have an encoding of ENDBR64 in a central place.
> 
> The best you can probably have is
> 
> #define ENDBR64_LEN 4
> 
> in endbr.h ?

Perhaps. That's not in this series nor in staging already, so it's a little
hard to check. By "central place" I really meant is_enbr64() if that's the
only place where the encoding actually appears.

>>> --- a/xen/arch/x86/xen.lds.S
>>> +++ b/xen/arch/x86/xen.lds.S
>>> @@ -221,6 +221,12 @@ SECTIONS
>>>         *(.initcall1.init)
>>>         __initcall_end = .;
>>>  
>>> +       . = ALIGN(POINTER_ALIGN);
>>> +       __initdata_cf_clobber_start = .;
>>> +       *(.init.data.cf_clobber)
>>> +       *(.init.rodata.cf_clobber)
>>> +       __initdata_cf_clobber_end = .;
>>> +
>>>         *(.init.data)
>>>         *(.init.data.rel)
>>>         *(.init.data.rel.*)
>> With r/o data ahead and r/w data following, may I suggest to flip the
>> order of the two section specifiers you add?
> 
> I don't follow.  This is all initdata which is merged together into a
> single section.
> 
> The only reason const data is split out in the first place is to appease
> the toolchains, not because it makes a difference.

It's marginal, I agree, but it would still seem more clean to me if all
(pseudo) r/o init data lived side by side.

Jan

Re: [PATCH v2 3/7] x86/altcall: Optimise away endbr64 instruction where possible

[Andrew Cooper]

On 14/02/2022 13:51, Jan Beulich wrote:
> On 14.02.2022 14:31, Andrew Cooper wrote:
>> On 14/02/2022 13:06, Jan Beulich wrote:
>>> On 14.02.2022 13:56, Andrew Cooper wrote:
>>>> @@ -330,6 +333,41 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start,
>>>>          add_nops(buf + a->repl_len, total_len - a->repl_len);
>>>>          text_poke(orig, buf, total_len);
>>>>      }
>>>> +
>>>> +    /*
>>>> +     * Clobber endbr64 instructions now that altcall has finished optimising
>>>> +     * all indirect branches to direct ones.
>>>> +     */
>>>> +    if ( force && cpu_has_xen_ibt )
>>>> +    {
>>>> +        void *const *val;
>>>> +        unsigned int clobbered = 0;
>>>> +
>>>> +        /*
>>>> +         * This is some minor structure (ab)use.  We walk the entire contents
>>>> +         * of .init.{ro,}data.cf_clobber as if it were an array of pointers.
>>>> +         *
>>>> +         * If the pointer points into .text, and at an endbr64 instruction,
>>>> +         * nop out the endbr64.  This causes the pointer to no longer be a
>>>> +         * legal indirect branch target under CET-IBT.  This is a
>>>> +         * defence-in-depth measure, to reduce the options available to an
>>>> +         * adversary who has managed to hijack a function pointer.
>>>> +         */
>>>> +        for ( val = __initdata_cf_clobber_start;
>>>> +              val < __initdata_cf_clobber_end;
>>>> +              val++ )
>>>> +        {
>>>> +            void *ptr = *val;
>>>> +
>>>> +            if ( !is_kernel_text(ptr) || !is_endbr64(ptr) )
>>>> +                continue;
>>>> +
>>>> +            add_nops(ptr, 4);
>>> This literal 4 would be nice to have a #define next to where the ENDBR64
>>> encoding has its central place.
>> We don't have an encoding of ENDBR64 in a central place.
>>
>> The best you can probably have is
>>
>> #define ENDBR64_LEN 4
>>
>> in endbr.h ?
> Perhaps. That's not in this series nor in staging already, so it's a little
> hard to check. By "central place" I really meant is_enbr64() if that's the
> only place where the encoding actually appears.

endbr.h is the header which contains is_endbr64(), and deliberately does
not contain the raw encoding.

>
>>>> --- a/xen/arch/x86/xen.lds.S
>>>> +++ b/xen/arch/x86/xen.lds.S
>>>> @@ -221,6 +221,12 @@ SECTIONS
>>>>         *(.initcall1.init)
>>>>         __initcall_end = .;
>>>>  
>>>> +       . = ALIGN(POINTER_ALIGN);
>>>> +       __initdata_cf_clobber_start = .;
>>>> +       *(.init.data.cf_clobber)
>>>> +       *(.init.rodata.cf_clobber)
>>>> +       __initdata_cf_clobber_end = .;
>>>> +
>>>>         *(.init.data)
>>>>         *(.init.data.rel)
>>>>         *(.init.data.rel.*)
>>> With r/o data ahead and r/w data following, may I suggest to flip the
>>> order of the two section specifiers you add?
>> I don't follow.  This is all initdata which is merged together into a
>> single section.
>>
>> The only reason const data is split out in the first place is to appease
>> the toolchains, not because it makes a difference.
> It's marginal, I agree, but it would still seem more clean to me if all
> (pseudo) r/o init data lived side by side.

I still don't understand what you're asking.

There is no such thing as actually read-only init data.

Wherever the .init.rodata goes in here, it's bounded by .init.data.

~Andrew

Re: [PATCH v2 3/7] x86/altcall: Optimise away endbr64 instruction where possible

[Jan Beulich]

On 14.02.2022 17:03, Andrew Cooper wrote:
> On 14/02/2022 13:51, Jan Beulich wrote:
>> On 14.02.2022 14:31, Andrew Cooper wrote:
>>> On 14/02/2022 13:06, Jan Beulich wrote:
>>>> On 14.02.2022 13:56, Andrew Cooper wrote:
>>>>> @@ -330,6 +333,41 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start,
>>>>>          add_nops(buf + a->repl_len, total_len - a->repl_len);
>>>>>          text_poke(orig, buf, total_len);
>>>>>      }
>>>>> +
>>>>> +    /*
>>>>> +     * Clobber endbr64 instructions now that altcall has finished optimising
>>>>> +     * all indirect branches to direct ones.
>>>>> +     */
>>>>> +    if ( force && cpu_has_xen_ibt )
>>>>> +    {
>>>>> +        void *const *val;
>>>>> +        unsigned int clobbered = 0;
>>>>> +
>>>>> +        /*
>>>>> +         * This is some minor structure (ab)use.  We walk the entire contents
>>>>> +         * of .init.{ro,}data.cf_clobber as if it were an array of pointers.
>>>>> +         *
>>>>> +         * If the pointer points into .text, and at an endbr64 instruction,
>>>>> +         * nop out the endbr64.  This causes the pointer to no longer be a
>>>>> +         * legal indirect branch target under CET-IBT.  This is a
>>>>> +         * defence-in-depth measure, to reduce the options available to an
>>>>> +         * adversary who has managed to hijack a function pointer.
>>>>> +         */
>>>>> +        for ( val = __initdata_cf_clobber_start;
>>>>> +              val < __initdata_cf_clobber_end;
>>>>> +              val++ )
>>>>> +        {
>>>>> +            void *ptr = *val;
>>>>> +
>>>>> +            if ( !is_kernel_text(ptr) || !is_endbr64(ptr) )
>>>>> +                continue;
>>>>> +
>>>>> +            add_nops(ptr, 4);
>>>> This literal 4 would be nice to have a #define next to where the ENDBR64
>>>> encoding has its central place.
>>> We don't have an encoding of ENDBR64 in a central place.
>>>
>>> The best you can probably have is
>>>
>>> #define ENDBR64_LEN 4
>>>
>>> in endbr.h ?
>> Perhaps. That's not in this series nor in staging already, so it's a little
>> hard to check. By "central place" I really meant is_enbr64() if that's the
>> only place where the encoding actually appears.
> 
> endbr.h is the header which contains is_endbr64(), and deliberately does
> not contain the raw encoding.

Well, yes, it's intentionally the inverted encoding, but I thought
you would get the point.

>>>>> --- a/xen/arch/x86/xen.lds.S
>>>>> +++ b/xen/arch/x86/xen.lds.S
>>>>> @@ -221,6 +221,12 @@ SECTIONS
>>>>>         *(.initcall1.init)
>>>>>         __initcall_end = .;
>>>>>  
>>>>> +       . = ALIGN(POINTER_ALIGN);
>>>>> +       __initdata_cf_clobber_start = .;
>>>>> +       *(.init.data.cf_clobber)
>>>>> +       *(.init.rodata.cf_clobber)
>>>>> +       __initdata_cf_clobber_end = .;
>>>>> +
>>>>>         *(.init.data)
>>>>>         *(.init.data.rel)
>>>>>         *(.init.data.rel.*)
>>>> With r/o data ahead and r/w data following, may I suggest to flip the
>>>> order of the two section specifiers you add?
>>> I don't follow.  This is all initdata which is merged together into a
>>> single section.
>>>
>>> The only reason const data is split out in the first place is to appease
>>> the toolchains, not because it makes a difference.
>> It's marginal, I agree, but it would still seem more clean to me if all
>> (pseudo) r/o init data lived side by side.
> 
> I still don't understand what you're asking.
> 
> There is no such thing as actually read-only init data.
> 
> Wherever the .init.rodata goes in here, it's bounded by .init.data.

Well, looking at the linker script again I notice that while r/o items
like .init.setup and .initcall*.init come first, some further ones
(.init_array etc) come quite late. Personally I'd prefer if all r/o
items sat side by side, no matter that currently we munge them all
into a single section. Then, if we decided to stop this practice, all
it would take would be to insert an output section closing and re-
opening. (Or it would have been so until now; with your addition it
wouldn't be as simple anymore anyway.)

But anyway, if at this point I still didn't get my point across, then
please leave things as you have them.

Jan

Re: [PATCH v2 3/7] x86/altcall: Optimise away endbr64 instruction where possible

[Jan Beulich]

On 14.02.2022 13:56, Andrew Cooper wrote:
> @@ -330,6 +333,41 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start,
>          add_nops(buf + a->repl_len, total_len - a->repl_len);
>          text_poke(orig, buf, total_len);
>      }
> +
> +    /*
> +     * Clobber endbr64 instructions now that altcall has finished optimising
> +     * all indirect branches to direct ones.
> +     */
> +    if ( force && cpu_has_xen_ibt )

Btw, this is now also entered when the function is called from
apply_alternatives() (i.e. when livepatching), but ...

> +    {
> +        void *const *val;
> +        unsigned int clobbered = 0;
> +
> +        /*
> +         * This is some minor structure (ab)use.  We walk the entire contents
> +         * of .init.{ro,}data.cf_clobber as if it were an array of pointers.
> +         *
> +         * If the pointer points into .text, and at an endbr64 instruction,
> +         * nop out the endbr64.  This causes the pointer to no longer be a
> +         * legal indirect branch target under CET-IBT.  This is a
> +         * defence-in-depth measure, to reduce the options available to an
> +         * adversary who has managed to hijack a function pointer.
> +         */
> +        for ( val = __initdata_cf_clobber_start;
> +              val < __initdata_cf_clobber_end;

... this being main binary boundaries, no action would be taken on
the livepatch binary. Hence (also due to having been here before
during boot), all that I understand will happen ...

> +              val++ )
> +        {
> +            void *ptr = *val;
> +
> +            if ( !is_kernel_text(ptr) || !is_endbr64(ptr) )
> +                continue;
> +
> +            add_nops(ptr, 4);
> +            clobbered++;
> +        }
> +
> +        printk("altcall: Optimised away %u endbr64 instructions\n", clobbered);

... that this message be logged once per patch load (with a number
of 0). I think the enclosing if() wants to be amended by
"&& system_state < SYS_STATE_active". If you agree, I can easily
make a patch.

Jan

Re: [PATCH v2 3/7] x86/altcall: Optimise away endbr64 instruction where possible

[Andrew Cooper]

On 01/03/2022 11:59, Jan Beulich wrote:
> On 14.02.2022 13:56, Andrew Cooper wrote:
>> @@ -330,6 +333,41 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start,
>>          add_nops(buf + a->repl_len, total_len - a->repl_len);
>>          text_poke(orig, buf, total_len);
>>      }
>> +
>> +    /*
>> +     * Clobber endbr64 instructions now that altcall has finished optimising
>> +     * all indirect branches to direct ones.
>> +     */
>> +    if ( force && cpu_has_xen_ibt )
> Btw, this is now also entered when the function is called from
> apply_alternatives() (i.e. when livepatching), but ...
>
>> +    {
>> +        void *const *val;
>> +        unsigned int clobbered = 0;
>> +
>> +        /*
>> +         * This is some minor structure (ab)use.  We walk the entire contents
>> +         * of .init.{ro,}data.cf_clobber as if it were an array of pointers.
>> +         *
>> +         * If the pointer points into .text, and at an endbr64 instruction,
>> +         * nop out the endbr64.  This causes the pointer to no longer be a
>> +         * legal indirect branch target under CET-IBT.  This is a
>> +         * defence-in-depth measure, to reduce the options available to an
>> +         * adversary who has managed to hijack a function pointer.
>> +         */
>> +        for ( val = __initdata_cf_clobber_start;
>> +              val < __initdata_cf_clobber_end;
> ... this being main binary boundaries, no action would be taken on
> the livepatch binary. Hence (also due to having been here before
> during boot), all that I understand will happen ...
>
>> +              val++ )
>> +        {
>> +            void *ptr = *val;
>> +
>> +            if ( !is_kernel_text(ptr) || !is_endbr64(ptr) )
>> +                continue;
>> +
>> +            add_nops(ptr, 4);
>> +            clobbered++;
>> +        }
>> +
>> +        printk("altcall: Optimised away %u endbr64 instructions\n", clobbered);
> ... that this message be logged once per patch load (with a number
> of 0). I think the enclosing if() wants to be amended by
> "&& system_state < SYS_STATE_active". If you agree, I can easily
> make a patch.

Hmm.  There are other livepatching fixes going on, but they're starting
with fixing the build system breakage.  (The major livepatching fix is
to adjust how we patch an old function that has an ENDBR64 at the start.)

That said, a livepatch needs to contain a section equivalent to
__initdata_cf_clobber, to be processed during load, dependent on
cpu_has_xen_ibt.

Perhaps the best option is to break the clobber out into a helper that
takes a start/end pair and returns the number clobbered.  That way, it
can be reused by the livepatch logic, and independently of this printk().

~Andrew

Re: [PATCH v2 3/7] x86/altcall: Optimise away endbr64 instruction where possible

[Jan Beulich]

On 01.03.2022 15:51, Andrew Cooper wrote:
> On 01/03/2022 11:59, Jan Beulich wrote:
>> On 14.02.2022 13:56, Andrew Cooper wrote:
>>> @@ -330,6 +333,41 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start,
>>>          add_nops(buf + a->repl_len, total_len - a->repl_len);
>>>          text_poke(orig, buf, total_len);
>>>      }
>>> +
>>> +    /*
>>> +     * Clobber endbr64 instructions now that altcall has finished optimising
>>> +     * all indirect branches to direct ones.
>>> +     */
>>> +    if ( force && cpu_has_xen_ibt )
>> Btw, this is now also entered when the function is called from
>> apply_alternatives() (i.e. when livepatching), but ...
>>
>>> +    {
>>> +        void *const *val;
>>> +        unsigned int clobbered = 0;
>>> +
>>> +        /*
>>> +         * This is some minor structure (ab)use.  We walk the entire contents
>>> +         * of .init.{ro,}data.cf_clobber as if it were an array of pointers.
>>> +         *
>>> +         * If the pointer points into .text, and at an endbr64 instruction,
>>> +         * nop out the endbr64.  This causes the pointer to no longer be a
>>> +         * legal indirect branch target under CET-IBT.  This is a
>>> +         * defence-in-depth measure, to reduce the options available to an
>>> +         * adversary who has managed to hijack a function pointer.
>>> +         */
>>> +        for ( val = __initdata_cf_clobber_start;
>>> +              val < __initdata_cf_clobber_end;
>> ... this being main binary boundaries, no action would be taken on
>> the livepatch binary. Hence (also due to having been here before
>> during boot), all that I understand will happen ...
>>
>>> +              val++ )
>>> +        {
>>> +            void *ptr = *val;
>>> +
>>> +            if ( !is_kernel_text(ptr) || !is_endbr64(ptr) )
>>> +                continue;
>>> +
>>> +            add_nops(ptr, 4);
>>> +            clobbered++;
>>> +        }
>>> +
>>> +        printk("altcall: Optimised away %u endbr64 instructions\n", clobbered);
>> ... that this message be logged once per patch load (with a number
>> of 0). I think the enclosing if() wants to be amended by
>> "&& system_state < SYS_STATE_active". If you agree, I can easily
>> make a patch.
> 
> Hmm.  There are other livepatching fixes going on, but they're starting
> with fixing the build system breakage.  (The major livepatching fix is
> to adjust how we patch an old function that has an ENDBR64 at the start.)
> 
> That said, a livepatch needs to contain a section equivalent to
> __initdata_cf_clobber, to be processed during load, dependent on
> cpu_has_xen_ibt.

IOW you say altcall patching can occur in live patches? If so, then ...

> Perhaps the best option is to break the clobber out into a helper that
> takes a start/end pair and returns the number clobbered.  That way, it
> can be reused by the livepatch logic, and independently of this printk().

... yes, parametrizing would be necessary.

Jan

[PATCH v2 4/7] xsm: Use __initconst_cf_clobber for xsm_ops

[Andrew Cooper]

All calls through xsm_ops are fully altcall'd.  Harden all fnptr targets.

This yields:

  (XEN) altcall: Optimised away 197 endbr64 instructions

of 1655 on an everything-enabled build of Xen, which is ~12%.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 xen/xsm/dummy.c       | 2 +-
 xen/xsm/flask/hooks.c | 2 +-
 xen/xsm/silo.c        | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index 4d29a9aa5b9f..8c044ef61500 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -13,7 +13,7 @@
 #define XSM_NO_WRAPPERS
 #include <xsm/dummy.h>
 
-static const struct xsm_ops __initconstrel dummy_ops = {
+static const struct xsm_ops __initconst_cf_clobber dummy_ops = {
     .security_domaininfo           = xsm_security_domaininfo,
     .domain_create                 = xsm_domain_create,
     .getdomaininfo                 = xsm_getdomaininfo,
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 63484e323c09..0bf63ffa84c4 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1765,7 +1765,7 @@ static int cf_check flask_argo_send(
 
 #endif
 
-static const struct xsm_ops __initconstrel flask_ops = {
+static const struct xsm_ops __initconst_cf_clobber flask_ops = {
     .security_domaininfo = flask_security_domaininfo,
     .domain_create = flask_domain_create,
     .getdomaininfo = flask_getdomaininfo,
diff --git a/xen/xsm/silo.c b/xen/xsm/silo.c
index 4d5fc98e7e54..b89b36428784 100644
--- a/xen/xsm/silo.c
+++ b/xen/xsm/silo.c
@@ -102,7 +102,7 @@ static int cf_check silo_argo_send(
 
 #endif
 
-static const struct xsm_ops __initconstrel silo_xsm_ops = {
+static const struct xsm_ops __initconst_cf_clobber silo_xsm_ops = {
     .evtchn_unbound = silo_evtchn_unbound,
     .evtchn_interdomain = silo_evtchn_interdomain,
     .grant_mapref = silo_grant_mapref,
-- 
2.11.0

[PATCH v2 5/7] x86/hvm: Use __initdata_cf_clobber for hvm_funcs

[Andrew Cooper]

All calls through hvm_funcs are fully altcall'd.  Harden all function pointer
targets.

This optimises away 106 targets.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/hvm/hvm.c     | 2 +-
 xen/arch/x86/hvm/svm/svm.c | 2 +-
 xen/arch/x86/hvm/vmx/vmx.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index cdd1529014f2..709a4191efe8 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -88,7 +88,7 @@ unsigned int opt_hvm_debug_level __read_mostly;
 integer_param("hvm_debug", opt_hvm_debug_level);
 #endif
 
-struct hvm_function_table hvm_funcs __read_mostly;
+struct hvm_function_table __ro_after_init hvm_funcs;
 
 /*
  * The I/O permission bitmap is globally shared by all HVM guests except
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 63535a74b504..b80d4af6cb90 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2513,7 +2513,7 @@ static void cf_check svm_set_reg(struct vcpu *v, unsigned int reg, uint64_t val)
     }
 }
 
-static struct hvm_function_table __initdata svm_function_table = {
+static struct hvm_function_table __initdata_cf_clobber svm_function_table = {
     .name                 = "SVM",
     .cpu_up_prepare       = svm_cpu_up_prepare,
     .cpu_dead             = svm_cpu_dead,
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 41db538a9e3d..758df3321884 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2473,7 +2473,7 @@ static void cf_check vmx_set_reg(struct vcpu *v, unsigned int reg, uint64_t val)
     vmx_vmcs_exit(v);
 }
 
-static struct hvm_function_table __initdata vmx_function_table = {
+static struct hvm_function_table __initdata_cf_clobber vmx_function_table = {
     .name                 = "VMX",
     .cpu_up_prepare       = vmx_cpu_up_prepare,
     .cpu_dead             = vmx_cpu_dead,
-- 
2.11.0

Re: [PATCH v2 5/7] x86/hvm: Use __initdata_cf_clobber for hvm_funcs

[Jan Beulich]

On 14.02.2022 13:56, Andrew Cooper wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -88,7 +88,7 @@ unsigned int opt_hvm_debug_level __read_mostly;
>  integer_param("hvm_debug", opt_hvm_debug_level);
>  #endif
>  
> -struct hvm_function_table hvm_funcs __read_mostly;
> +struct hvm_function_table __ro_after_init hvm_funcs;

Strictly speaking this is an unrelated change. I'm fine with it living here,
but half a sentence would be nice in the description.

> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2513,7 +2513,7 @@ static void cf_check svm_set_reg(struct vcpu *v, unsigned int reg, uint64_t val)
>      }
>  }
>  
> -static struct hvm_function_table __initdata svm_function_table = {
> +static struct hvm_function_table __initdata_cf_clobber svm_function_table = {
>      .name                 = "SVM",
>      .cpu_up_prepare       = svm_cpu_up_prepare,
>      .cpu_dead             = svm_cpu_dead,
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 41db538a9e3d..758df3321884 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2473,7 +2473,7 @@ static void cf_check vmx_set_reg(struct vcpu *v, unsigned int reg, uint64_t val)
>      vmx_vmcs_exit(v);
>  }
>  
> -static struct hvm_function_table __initdata vmx_function_table = {
> +static struct hvm_function_table __initdata_cf_clobber vmx_function_table = {
>      .name                 = "VMX",
>      .cpu_up_prepare       = vmx_cpu_up_prepare,
>      .cpu_dead             = vmx_cpu_dead,

While I'd like to re-raise my concern regarding the non-pointer fields
in these structure instances (just consider a sequence of enough bool
bitfields, which effectively can express any value, including ones
which would appear like pointers into .text), since for now all is okay
afaict:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v2 5/7] x86/hvm: Use __initdata_cf_clobber for hvm_funcs

[Andrew Cooper]

On 14/02/2022 13:10, Jan Beulich wrote:
> On 14.02.2022 13:56, Andrew Cooper wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -88,7 +88,7 @@ unsigned int opt_hvm_debug_level __read_mostly;
>>  integer_param("hvm_debug", opt_hvm_debug_level);
>>  #endif
>>  
>> -struct hvm_function_table hvm_funcs __read_mostly;
>> +struct hvm_function_table __ro_after_init hvm_funcs;
> Strictly speaking this is an unrelated change. I'm fine with it living here,
> but half a sentence would be nice in the description.

I could split it out, but we could probably make 200 patches of
"sprinkle some __ro_after_init around, now that it exists".

>
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -2513,7 +2513,7 @@ static void cf_check svm_set_reg(struct vcpu *v, unsigned int reg, uint64_t val)
>>      }
>>  }
>>  
>> -static struct hvm_function_table __initdata svm_function_table = {
>> +static struct hvm_function_table __initdata_cf_clobber svm_function_table = {
>>      .name                 = "SVM",
>>      .cpu_up_prepare       = svm_cpu_up_prepare,
>>      .cpu_dead             = svm_cpu_dead,
>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>> index 41db538a9e3d..758df3321884 100644
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -2473,7 +2473,7 @@ static void cf_check vmx_set_reg(struct vcpu *v, unsigned int reg, uint64_t val)
>>      vmx_vmcs_exit(v);
>>  }
>>  
>> -static struct hvm_function_table __initdata vmx_function_table = {
>> +static struct hvm_function_table __initdata_cf_clobber vmx_function_table = {
>>      .name                 = "VMX",
>>      .cpu_up_prepare       = vmx_cpu_up_prepare,
>>      .cpu_dead             = vmx_cpu_dead,
> While I'd like to re-raise my concern regarding the non-pointer fields
> in these structure instances (just consider a sequence of enough bool
> bitfields, which effectively can express any value, including ones
> which would appear like pointers into .text), since for now all is okay
> afaict:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

I should probably put something in the commit message too.  It is a
theoretical risk, but not (IMO) a practical one.

~Andrew

Re: [PATCH v2 5/7] x86/hvm: Use __initdata_cf_clobber for hvm_funcs

[Andrew Cooper]

On 14/02/2022 13:35, Andrew Cooper wrote:
> On 14/02/2022 13:10, Jan Beulich wrote:
>> On 14.02.2022 13:56, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -88,7 +88,7 @@ unsigned int opt_hvm_debug_level __read_mostly;
>>>  integer_param("hvm_debug", opt_hvm_debug_level);
>>>  #endif
>>>  
>>> -struct hvm_function_table hvm_funcs __read_mostly;
>>> +struct hvm_function_table __ro_after_init hvm_funcs;
>> Strictly speaking this is an unrelated change. I'm fine with it living here,
>> but half a sentence would be nice in the description.
> I could split it out, but we could probably make 200 patches of
> "sprinkle some __ro_after_init around, now that it exists".
>
>>> --- a/xen/arch/x86/hvm/svm/svm.c
>>> +++ b/xen/arch/x86/hvm/svm/svm.c
>>> @@ -2513,7 +2513,7 @@ static void cf_check svm_set_reg(struct vcpu *v, unsigned int reg, uint64_t val)
>>>      }
>>>  }
>>>  
>>> -static struct hvm_function_table __initdata svm_function_table = {
>>> +static struct hvm_function_table __initdata_cf_clobber svm_function_table = {
>>>      .name                 = "SVM",
>>>      .cpu_up_prepare       = svm_cpu_up_prepare,
>>>      .cpu_dead             = svm_cpu_dead,
>>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>>> index 41db538a9e3d..758df3321884 100644
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -2473,7 +2473,7 @@ static void cf_check vmx_set_reg(struct vcpu *v, unsigned int reg, uint64_t val)
>>>      vmx_vmcs_exit(v);
>>>  }
>>>  
>>> -static struct hvm_function_table __initdata vmx_function_table = {
>>> +static struct hvm_function_table __initdata_cf_clobber vmx_function_table = {
>>>      .name                 = "VMX",
>>>      .cpu_up_prepare       = vmx_cpu_up_prepare,
>>>      .cpu_dead             = vmx_cpu_dead,
>> While I'd like to re-raise my concern regarding the non-pointer fields
>> in these structure instances (just consider a sequence of enough bool
>> bitfields, which effectively can express any value, including ones
>> which would appear like pointers into .text), since for now all is okay
>> afaict:
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> I should probably put something in the commit message too.  It is a
> theoretical risk, but not (IMO) a practical one.

Updated commit message:

x86/hvm: Use __initdata_cf_clobber for hvm_funcs

Now that all calls through hvm_funcs are fully altcall'd, harden all the svm
and vmx function pointer targets.  This drops 106 endbr64 instructions.

Clobbering does come with a theoretical risk.  The non-pointer fields of
{svm,vmx}_function_table can in theory happen to form a bit pattern
matching a
pointer into .text at a legal endbr64 instruction, but this is expected
to be
implausible for anything liable to pass code review.

While at it, move hvm_funcs into __ro_after_init now that this exists.

~Andrew

Re: [PATCH v2 5/7] x86/hvm: Use __initdata_cf_clobber for hvm_funcs

[Jan Beulich]

On 14.02.2022 17:39, Andrew Cooper wrote:
> On 14/02/2022 13:35, Andrew Cooper wrote:
>> On 14/02/2022 13:10, Jan Beulich wrote:
>>> On 14.02.2022 13:56, Andrew Cooper wrote:
>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>> @@ -88,7 +88,7 @@ unsigned int opt_hvm_debug_level __read_mostly;
>>>>  integer_param("hvm_debug", opt_hvm_debug_level);
>>>>  #endif
>>>>  
>>>> -struct hvm_function_table hvm_funcs __read_mostly;
>>>> +struct hvm_function_table __ro_after_init hvm_funcs;
>>> Strictly speaking this is an unrelated change. I'm fine with it living here,
>>> but half a sentence would be nice in the description.
>> I could split it out, but we could probably make 200 patches of
>> "sprinkle some __ro_after_init around, now that it exists".
>>
>>>> --- a/xen/arch/x86/hvm/svm/svm.c
>>>> +++ b/xen/arch/x86/hvm/svm/svm.c
>>>> @@ -2513,7 +2513,7 @@ static void cf_check svm_set_reg(struct vcpu *v, unsigned int reg, uint64_t val)
>>>>      }
>>>>  }
>>>>  
>>>> -static struct hvm_function_table __initdata svm_function_table = {
>>>> +static struct hvm_function_table __initdata_cf_clobber svm_function_table = {
>>>>      .name                 = "SVM",
>>>>      .cpu_up_prepare       = svm_cpu_up_prepare,
>>>>      .cpu_dead             = svm_cpu_dead,
>>>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>>>> index 41db538a9e3d..758df3321884 100644
>>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>>> @@ -2473,7 +2473,7 @@ static void cf_check vmx_set_reg(struct vcpu *v, unsigned int reg, uint64_t val)
>>>>      vmx_vmcs_exit(v);
>>>>  }
>>>>  
>>>> -static struct hvm_function_table __initdata vmx_function_table = {
>>>> +static struct hvm_function_table __initdata_cf_clobber vmx_function_table = {
>>>>      .name                 = "VMX",
>>>>      .cpu_up_prepare       = vmx_cpu_up_prepare,
>>>>      .cpu_dead             = vmx_cpu_dead,
>>> While I'd like to re-raise my concern regarding the non-pointer fields
>>> in these structure instances (just consider a sequence of enough bool
>>> bitfields, which effectively can express any value, including ones
>>> which would appear like pointers into .text), since for now all is okay
>>> afaict:
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> I should probably put something in the commit message too.  It is a
>> theoretical risk, but not (IMO) a practical one.
> 
> Updated commit message:
> 
> x86/hvm: Use __initdata_cf_clobber for hvm_funcs
> 
> Now that all calls through hvm_funcs are fully altcall'd, harden all the svm
> and vmx function pointer targets.  This drops 106 endbr64 instructions.
> 
> Clobbering does come with a theoretical risk.  The non-pointer fields of
> {svm,vmx}_function_table can in theory happen to form a bit pattern
> matching a
> pointer into .text at a legal endbr64 instruction, but this is expected
> to be
> implausible for anything liable to pass code review.
> 
> While at it, move hvm_funcs into __ro_after_init now that this exists.

SGTM, thanks.

Jan

[PATCH v2 6/7] x86/ucode: Use altcall, and __initconst_cf_clobber

[Andrew Cooper]

Microcode loading is not a fastpath, but there are control flow integrity
hardening benefits from using altcall, because it allows us to clobber the
endbr64 instructions on all function pointer targets.

Convert the existing microcode_ops pointer into an __ro_after_init structure,
and move {amd,intel}_ucode_ops into __initconst_cf_clobber.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

v2:
 * Adjust commit message.
 * Use __initconst_cf_clobber and __ro_after_init.
---
 xen/arch/x86/cpu/microcode/amd.c   |  2 +-
 xen/arch/x86/cpu/microcode/core.c  | 38 ++++++++++++++++++++------------------
 xen/arch/x86/cpu/microcode/intel.c |  2 +-
 3 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index 0afa2192bf1d..8195707ee149 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -422,7 +422,7 @@ static struct microcode_patch *cf_check cpu_request_microcode(
     return patch;
 }
 
-const struct microcode_ops amd_ucode_ops = {
+const struct microcode_ops __initconst_cf_clobber amd_ucode_ops = {
     .cpu_request_microcode            = cpu_request_microcode,
     .collect_cpu_info                 = collect_cpu_info,
     .apply_microcode                  = apply_microcode,
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index f84dafa82693..452a7ca77340 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -21,6 +21,7 @@
  * 2 of the License, or (at your option) any later version.
  */
 
+#include <xen/alternative-call.h>
 #include <xen/cpu.h>
 #include <xen/earlycpio.h>
 #include <xen/err.h>
@@ -214,7 +215,7 @@ void __init microcode_grab_module(
         microcode_scan_module(module_map, mbi);
 }
 
-static const struct microcode_ops __read_mostly *microcode_ops;
+static struct microcode_ops __ro_after_init ucode_ops;
 
 static DEFINE_SPINLOCK(microcode_mutex);
 
@@ -241,9 +242,9 @@ static const struct microcode_patch *nmi_patch = ZERO_BLOCK_PTR;
  */
 static struct microcode_patch *parse_blob(const char *buf, size_t len)
 {
-    microcode_ops->collect_cpu_info();
+    alternative_vcall(ucode_ops.collect_cpu_info);
 
-    return microcode_ops->cpu_request_microcode(buf, len);
+    return alternative_call(ucode_ops.cpu_request_microcode, buf, len);
 }
 
 static void microcode_free_patch(struct microcode_patch *patch)
@@ -258,8 +259,8 @@ static bool microcode_update_cache(struct microcode_patch *patch)
 
     if ( !microcode_cache )
         microcode_cache = patch;
-    else if ( microcode_ops->compare_patch(patch,
-                                           microcode_cache) == NEW_UCODE )
+    else if ( alternative_call(ucode_ops.compare_patch,
+                               patch, microcode_cache) == NEW_UCODE )
     {
         microcode_free_patch(microcode_cache);
         microcode_cache = patch;
@@ -311,14 +312,14 @@ static int microcode_update_cpu(const struct microcode_patch *patch)
 {
     int err;
 
-    microcode_ops->collect_cpu_info();
+    alternative_vcall(ucode_ops.collect_cpu_info);
 
     spin_lock(&microcode_mutex);
     if ( patch )
-        err = microcode_ops->apply_microcode(patch);
+        err = alternative_call(ucode_ops.apply_microcode, patch);
     else if ( microcode_cache )
     {
-        err = microcode_ops->apply_microcode(microcode_cache);
+        err = alternative_call(ucode_ops.apply_microcode, microcode_cache);
         if ( err == -EIO )
         {
             microcode_free_patch(microcode_cache);
@@ -368,7 +369,7 @@ static int primary_thread_work(const struct microcode_patch *patch)
     if ( !wait_for_state(LOADING_ENTER) )
         return -EBUSY;
 
-    ret = microcode_ops->apply_microcode(patch);
+    ret = alternative_call(ucode_ops.apply_microcode, patch);
     if ( !ret )
         atomic_inc(&cpu_updated);
     atomic_inc(&cpu_out);
@@ -481,7 +482,7 @@ static int control_thread_fn(const struct microcode_patch *patch)
     }
 
     /* Control thread loads ucode first while others are in NMI handler. */
-    ret = microcode_ops->apply_microcode(patch);
+    ret = alternative_call(ucode_ops.apply_microcode, patch);
     if ( !ret )
         atomic_inc(&cpu_updated);
     atomic_inc(&cpu_out);
@@ -610,7 +611,8 @@ static long cf_check microcode_update_helper(void *data)
      */
     spin_lock(&microcode_mutex);
     if ( microcode_cache &&
-         microcode_ops->compare_patch(patch, microcode_cache) != NEW_UCODE )
+         alternative_call(ucode_ops.compare_patch,
+                          patch, microcode_cache) != NEW_UCODE )
     {
         spin_unlock(&microcode_mutex);
         printk(XENLOG_WARNING "microcode: couldn't find any newer revision "
@@ -678,7 +680,7 @@ int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len)
     if ( len != (uint32_t)len )
         return -E2BIG;
 
-    if ( microcode_ops == NULL )
+    if ( !ucode_ops.apply_microcode )
         return -EINVAL;
 
     buffer = xmalloc_flex_struct(struct ucode_buf, buffer, len);
@@ -722,10 +724,10 @@ __initcall(microcode_init);
 /* Load a cached update to current cpu */
 int microcode_update_one(void)
 {
-    if ( !microcode_ops )
+    if ( !ucode_ops.apply_microcode )
         return -EOPNOTSUPP;
 
-    microcode_ops->collect_cpu_info();
+    alternative_vcall(ucode_ops.collect_cpu_info);
 
     return microcode_update_cpu(NULL);
 }
@@ -780,22 +782,22 @@ int __init early_microcode_init(void)
     {
     case X86_VENDOR_AMD:
         if ( c->x86 >= 0x10 )
-            microcode_ops = &amd_ucode_ops;
+            ucode_ops = amd_ucode_ops;
         break;
 
     case X86_VENDOR_INTEL:
         if ( c->x86 >= 6 )
-            microcode_ops = &intel_ucode_ops;
+            ucode_ops = intel_ucode_ops;
         break;
     }
 
-    if ( !microcode_ops )
+    if ( !ucode_ops.apply_microcode )
     {
         printk(XENLOG_WARNING "Microcode loading not available\n");
         return -ENODEV;
     }
 
-    microcode_ops->collect_cpu_info();
+    alternative_vcall(ucode_ops.collect_cpu_info);
 
     if ( ucode_mod.mod_end || ucode_blob.size )
         rc = early_microcode_update_cpu();
diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index d3864b5ab03e..f5ba6d76d724 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -376,7 +376,7 @@ static struct microcode_patch *cf_check cpu_request_microcode(
     return patch;
 }
 
-const struct microcode_ops intel_ucode_ops = {
+const struct microcode_ops __initconst_cf_clobber intel_ucode_ops = {
     .cpu_request_microcode            = cpu_request_microcode,
     .collect_cpu_info                 = collect_cpu_info,
     .apply_microcode                  = apply_microcode,
-- 
2.11.0

Re: [PATCH v2 6/7] x86/ucode: Use altcall, and __initconst_cf_clobber

[Jan Beulich]

On 14.02.2022 13:56, Andrew Cooper wrote:
> Microcode loading is not a fastpath, but there are control flow integrity
> hardening benefits from using altcall, because it allows us to clobber the
> endbr64 instructions on all function pointer targets.
> 
> Convert the existing microcode_ops pointer into an __ro_after_init structure,
> and move {amd,intel}_ucode_ops into __initconst_cf_clobber.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

[PATCH v2 7/7] x86/vpmu: Harden indirect branches

[Andrew Cooper]

As all function pointer calls are resoved to direct calls on boot, clobber the
endbr64 instructions too to make life harder for an attacker which has managed
to hijack a function pointer.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

v2:
 * Use __initconst_cf_clobber
---
 xen/arch/x86/cpu/vpmu_amd.c   | 2 +-
 xen/arch/x86/cpu/vpmu_intel.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/cpu/vpmu_amd.c b/xen/arch/x86/cpu/vpmu_amd.c
index 5963ce90150a..9bacc02ec135 100644
--- a/xen/arch/x86/cpu/vpmu_amd.c
+++ b/xen/arch/x86/cpu/vpmu_amd.c
@@ -518,7 +518,7 @@ static int cf_check svm_vpmu_initialise(struct vcpu *v)
     return 0;
 }
 
-static const struct arch_vpmu_ops __initconstrel amd_vpmu_ops = {
+static const struct arch_vpmu_ops __initconst_cf_clobber amd_vpmu_ops = {
     .initialise = svm_vpmu_initialise,
     .do_wrmsr = amd_vpmu_do_wrmsr,
     .do_rdmsr = amd_vpmu_do_rdmsr,
diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c
index 48b81ab6f018..8612f46973ef 100644
--- a/xen/arch/x86/cpu/vpmu_intel.c
+++ b/xen/arch/x86/cpu/vpmu_intel.c
@@ -880,7 +880,7 @@ static int cf_check vmx_vpmu_initialise(struct vcpu *v)
     return 0;
 }
 
-static const struct arch_vpmu_ops __initconstrel core2_vpmu_ops = {
+static const struct arch_vpmu_ops __initconst_cf_clobber core2_vpmu_ops = {
     .initialise = vmx_vpmu_initialise,
     .do_wrmsr = core2_vpmu_do_wrmsr,
     .do_rdmsr = core2_vpmu_do_rdmsr,
-- 
2.11.0

Re: [PATCH v2 7/7] x86/vpmu: Harden indirect branches

[Jan Beulich]

On 14.02.2022 13:56, Andrew Cooper wrote:
> As all function pointer calls are resoved to direct calls on boot, clobber the
> endbr64 instructions too to make life harder for an attacker which has managed
> to hijack a function pointer.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

[PATCH v2.1 8/7] x86/IOMMU: Use altcall, and __initconst_cf_clobber

[Andrew Cooper]

Most IOMMU hooks are already altcall for performance reasons.  Convert the
rest of them so we can harden all the hooks in Control Flow Integrity
configurations.  This necessitates the use of iommu_{v,}call() in debug builds
too.

Move the root iommu_ops from __read_mostly to __ro_after_init now that the
latter exists.  There is no need for a forward declaration of vtd_ops any
more, meaning that __initconst_cf_clobber can be used for VTD and AMD.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/include/asm/iommu.h            | 6 ++----
 xen/drivers/passthrough/amd/pci_amd_iommu.c | 2 +-
 xen/drivers/passthrough/iommu.c             | 7 ++++---
 xen/drivers/passthrough/vtd/iommu.c         | 3 +--
 xen/drivers/passthrough/x86/iommu.c         | 4 ++--
 5 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/include/asm/iommu.h b/xen/arch/x86/include/asm/iommu.h
index 8a96ba1f097f..a87f6d416252 100644
--- a/xen/arch/x86/include/asm/iommu.h
+++ b/xen/arch/x86/include/asm/iommu.h
@@ -72,7 +72,6 @@ struct arch_iommu
 
 extern struct iommu_ops iommu_ops;
 
-#ifdef NDEBUG
 # include <asm/alternative.h>
 # define iommu_call(ops, fn, args...) ({      \
     (void)(ops);                              \
@@ -83,7 +82,6 @@ extern struct iommu_ops iommu_ops;
     (void)(ops);                              \
     alternative_vcall(iommu_ops.fn, ## args); \
 })
-#endif
 
 static inline const struct iommu_ops *iommu_get_ops(void)
 {
@@ -106,7 +104,7 @@ int iommu_setup_hpet_msi(struct msi_desc *);
 static inline int iommu_adjust_irq_affinities(void)
 {
     return iommu_ops.adjust_irq_affinities
-           ? iommu_ops.adjust_irq_affinities()
+           ? iommu_call(iommu_ops, adjust_irq_affinities)
            : 0;
 }
 
@@ -122,7 +120,7 @@ int iommu_enable_x2apic(void);
 static inline void iommu_disable_x2apic(void)
 {
     if ( x2apic_enabled && iommu_ops.disable_x2apic )
-        iommu_ops.disable_x2apic();
+        iommu_vcall(iommu_ops, disable_x2apic);
 }
 
 int iommu_identity_mapping(struct domain *d, p2m_access_t p2ma,
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index e57f555d00d1..4b59a4efe9b6 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -628,7 +628,7 @@ static void cf_check amd_dump_page_tables(struct domain *d)
                               hd->arch.amd.paging_mode, 0, 0);
 }
 
-static const struct iommu_ops __initconstrel _iommu_ops = {
+static const struct iommu_ops __initconst_cf_clobber _iommu_ops = {
     .init = amd_iommu_domain_init,
     .hwdom_init = amd_iommu_hwdom_init,
     .quarantine_init = amd_iommu_quarantine_init,
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index e220fea72c2f..c6b2c384d1dd 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -540,7 +540,7 @@ int __init iommu_setup(void)
 int iommu_suspend()
 {
     if ( iommu_enabled )
-        return iommu_get_ops()->suspend();
+        return iommu_call(iommu_get_ops(), suspend);
 
     return 0;
 }
@@ -548,7 +548,7 @@ int iommu_suspend()
 void iommu_resume()
 {
     if ( iommu_enabled )
-        iommu_get_ops()->resume();
+        iommu_vcall(iommu_get_ops(), resume);
 }
 
 int iommu_do_domctl(
@@ -578,7 +578,8 @@ void iommu_crash_shutdown(void)
         return;
 
     if ( iommu_enabled )
-        iommu_get_ops()->crash_shutdown();
+        iommu_vcall(iommu_get_ops(), crash_shutdown);
+
     iommu_enabled = false;
 #ifndef iommu_intremap
     iommu_intremap = iommu_intremap_off;
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 56968a06a100..6a65ba1d8271 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -56,7 +56,6 @@ bool __read_mostly iommu_snoop = true;
 
 static unsigned int __read_mostly nr_iommus;
 
-static struct iommu_ops vtd_ops;
 static struct tasklet vtd_fault_tasklet;
 
 static int cf_check setup_hwdom_device(u8 devfn, struct pci_dev *);
@@ -2794,7 +2793,7 @@ static int __init cf_check intel_iommu_quarantine_init(struct domain *d)
     return rc;
 }
 
-static struct iommu_ops __initdata vtd_ops = {
+static const struct iommu_ops __initconst_cf_clobber vtd_ops = {
     .init = intel_iommu_domain_init,
     .hwdom_init = intel_iommu_hwdom_init,
     .quarantine_init = intel_iommu_quarantine_init,
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index ad5f44e13d98..17c0fe555dd0 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -27,7 +27,7 @@
 #include <asm/setup.h>
 
 const struct iommu_init_ops *__initdata iommu_init_ops;
-struct iommu_ops __read_mostly iommu_ops;
+struct iommu_ops __ro_after_init iommu_ops;
 bool __read_mostly iommu_non_coherent;
 
 enum iommu_intremap __read_mostly iommu_intremap = iommu_intremap_full;
@@ -129,7 +129,7 @@ int iommu_enable_x2apic(void)
     if ( !iommu_ops.enable_x2apic )
         return -EOPNOTSUPP;
 
-    return iommu_ops.enable_x2apic();
+    return iommu_call(iommu_ops, enable_x2apic);
 }
 
 void iommu_update_ire_from_apic(
-- 
2.11.0

Re: [PATCH v2.1 8/7] x86/IOMMU: Use altcall, and __initconst_cf_clobber

[Jan Beulich]

On 21.02.2022 19:03, Andrew Cooper wrote:
> Most IOMMU hooks are already altcall for performance reasons.  Convert the
> rest of them so we can harden all the hooks in Control Flow Integrity
> configurations.  This necessitates the use of iommu_{v,}call() in debug builds
> too.
> 
> Move the root iommu_ops from __read_mostly to __ro_after_init now that the
> latter exists.  There is no need for a forward declaration of vtd_ops any
> more, meaning that __initconst_cf_clobber can be used for VTD and AMD.

The connection between the forward declaration and the annotation addition
isn't really clear to me.

> --- a/xen/arch/x86/include/asm/iommu.h
> +++ b/xen/arch/x86/include/asm/iommu.h
> @@ -72,7 +72,6 @@ struct arch_iommu
>  
>  extern struct iommu_ops iommu_ops;
>  
> -#ifdef NDEBUG
>  # include <asm/alternative.h>
>  # define iommu_call(ops, fn, args...) ({      \
>      (void)(ops);                              \
> @@ -83,7 +82,6 @@ extern struct iommu_ops iommu_ops;
>      (void)(ops);                              \
>      alternative_vcall(iommu_ops.fn, ## args); \
>  })
> -#endif
>  
>  static inline const struct iommu_ops *iommu_get_ops(void)
>  {
> @@ -106,7 +104,7 @@ int iommu_setup_hpet_msi(struct msi_desc *);
>  static inline int iommu_adjust_irq_affinities(void)
>  {
>      return iommu_ops.adjust_irq_affinities
> -           ? iommu_ops.adjust_irq_affinities()
> +           ? iommu_call(iommu_ops, adjust_irq_affinities)

While this (and other instances below) is x86-only code, where - with
the removal of the #ifdef above - we now know the first argument is
always ignored, I think it would still better be of the correct type
(&iommu_ops). Perhaps the "(void)(ops)" in the macro definitions would
better become "ASSERT((ops) == &iommu_ops)", which would check both
type (compile time) and value (runtime).

> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -540,7 +540,7 @@ int __init iommu_setup(void)
>  int iommu_suspend()
>  {
>      if ( iommu_enabled )
> -        return iommu_get_ops()->suspend();
> +        return iommu_call(iommu_get_ops(), suspend);

This use of iommu_get_ops() in such constructs is a pattern we didn't
have so far. Perhaps it just looks bogus, and all is fine in reality
(apart from the whole idea being wrong for Arm, or really any
environment where multiple dissimilar IOMMUs may be in use). Or wait,
there are pre-existing cases (just not immediately visible when
grep-ing for "iommu_v?call") in iommu_get_reserved_device_memory() and
iommu_setup_hpet_msi().

> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -56,7 +56,6 @@ bool __read_mostly iommu_snoop = true;
>  
>  static unsigned int __read_mostly nr_iommus;
>  
> -static struct iommu_ops vtd_ops;
>  static struct tasklet vtd_fault_tasklet;
>  
>  static int cf_check setup_hwdom_device(u8 devfn, struct pci_dev *);
> @@ -2794,7 +2793,7 @@ static int __init cf_check intel_iommu_quarantine_init(struct domain *d)
>      return rc;
>  }
>  
> -static struct iommu_ops __initdata vtd_ops = {
> +static const struct iommu_ops __initconst_cf_clobber vtd_ops = {

Ah yes, the conversion to const (and the dropping of the forward decl)
could have been part of "VT-d / x86: re-arrange cache syncing".

With the missing &-s added and preferably with the description adjusted
a little
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v2.1 8/7] x86/IOMMU: Use altcall, and __initconst_cf_clobber

[Andrew Cooper]

On 22/02/2022 09:29, Jan Beulich wrote:
> On 21.02.2022 19:03, Andrew Cooper wrote:
>> Most IOMMU hooks are already altcall for performance reasons.  Convert the
>> rest of them so we can harden all the hooks in Control Flow Integrity
>> configurations.  This necessitates the use of iommu_{v,}call() in debug builds
>> too.
>>
>> Move the root iommu_ops from __read_mostly to __ro_after_init now that the
>> latter exists.  There is no need for a forward declaration of vtd_ops any
>> more, meaning that __initconst_cf_clobber can be used for VTD and AMD.
> The connection between the forward declaration and the annotation addition
> isn't really clear to me.
>
>> --- a/xen/arch/x86/include/asm/iommu.h
>> +++ b/xen/arch/x86/include/asm/iommu.h
>> @@ -72,7 +72,6 @@ struct arch_iommu
>>  
>>  extern struct iommu_ops iommu_ops;
>>  
>> -#ifdef NDEBUG
>>  # include <asm/alternative.h>
>>  # define iommu_call(ops, fn, args...) ({      \
>>      (void)(ops);                              \
>> @@ -83,7 +82,6 @@ extern struct iommu_ops iommu_ops;
>>      (void)(ops);                              \
>>      alternative_vcall(iommu_ops.fn, ## args); \
>>  })
>> -#endif
>>  
>>  static inline const struct iommu_ops *iommu_get_ops(void)
>>  {
>> @@ -106,7 +104,7 @@ int iommu_setup_hpet_msi(struct msi_desc *);
>>  static inline int iommu_adjust_irq_affinities(void)
>>  {
>>      return iommu_ops.adjust_irq_affinities
>> -           ? iommu_ops.adjust_irq_affinities()
>> +           ? iommu_call(iommu_ops, adjust_irq_affinities)
> While this (and other instances below) is x86-only code, where - with
> the removal of the #ifdef above - we now know the first argument is
> always ignored, I think it would still better be of the correct type
> (&iommu_ops). Perhaps the "(void)(ops)" in the macro definitions would
> better become "ASSERT((ops) == &iommu_ops)", which would check both
> type (compile time) and value (runtime).

I'm happy to fold that change if you want.  It ought to optimise out
completely for being

>
>> --- a/xen/drivers/passthrough/iommu.c
>> +++ b/xen/drivers/passthrough/iommu.c
>> @@ -540,7 +540,7 @@ int __init iommu_setup(void)
>>  int iommu_suspend()
>>  {
>>      if ( iommu_enabled )
>> -        return iommu_get_ops()->suspend();
>> +        return iommu_call(iommu_get_ops(), suspend);
> This use of iommu_get_ops() in such constructs is a pattern we didn't
> have so far. Perhaps it just looks bogus, and all is fine in reality
> (apart from the whole idea being wrong for Arm, or really any
> environment where multiple dissimilar IOMMUs may be in use). Or wait,
> there are pre-existing cases (just not immediately visible when
> grep-ing for "iommu_v?call") in iommu_get_reserved_device_memory() and
> iommu_setup_hpet_msi().

I think this means your happy(ish) with the change?

I agree that this is nonsense on ARM, but the codepath isn't used yet
and someone's going to have to reconcile the conflicting views.

>> --- a/xen/drivers/passthrough/vtd/iommu.c
>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>> @@ -56,7 +56,6 @@ bool __read_mostly iommu_snoop = true;
>>  
>>  static unsigned int __read_mostly nr_iommus;
>>  
>> -static struct iommu_ops vtd_ops;
>>  static struct tasklet vtd_fault_tasklet;
>>  
>>  static int cf_check setup_hwdom_device(u8 devfn, struct pci_dev *);
>> @@ -2794,7 +2793,7 @@ static int __init cf_check intel_iommu_quarantine_init(struct domain *d)
>>      return rc;
>>  }
>>  
>> -static struct iommu_ops __initdata vtd_ops = {
>> +static const struct iommu_ops __initconst_cf_clobber vtd_ops = {
> Ah yes, the conversion to const (and the dropping of the forward decl)
> could have been part of "VT-d / x86: re-arrange cache syncing".
>
> With the missing &-s added and preferably with the description adjusted
> a little
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

~Andrew

Re: [PATCH v2.1 8/7] x86/IOMMU: Use altcall, and __initconst_cf_clobber

[Andrew Cooper]

On 22/02/2022 10:54, Andrew Cooper wrote:
> On 22/02/2022 09:29, Jan Beulich wrote:
>> On 21.02.2022 19:03, Andrew Cooper wrote:
>>> Most IOMMU hooks are already altcall for performance reasons.  Convert the
>>> rest of them so we can harden all the hooks in Control Flow Integrity
>>> configurations.  This necessitates the use of iommu_{v,}call() in debug builds
>>> too.
>>>
>>> Move the root iommu_ops from __read_mostly to __ro_after_init now that the
>>> latter exists.  There is no need for a forward declaration of vtd_ops any
>>> more, meaning that __initconst_cf_clobber can be used for VTD and AMD.
>> The connection between the forward declaration and the annotation addition
>> isn't really clear to me.
>>
>>> --- a/xen/arch/x86/include/asm/iommu.h
>>> +++ b/xen/arch/x86/include/asm/iommu.h
>>> @@ -72,7 +72,6 @@ struct arch_iommu
>>>  
>>>  extern struct iommu_ops iommu_ops;
>>>  
>>> -#ifdef NDEBUG
>>>  # include <asm/alternative.h>
>>>  # define iommu_call(ops, fn, args...) ({      \
>>>      (void)(ops);                              \
>>> @@ -83,7 +82,6 @@ extern struct iommu_ops iommu_ops;
>>>      (void)(ops);                              \
>>>      alternative_vcall(iommu_ops.fn, ## args); \
>>>  })
>>> -#endif
>>>  
>>>  static inline const struct iommu_ops *iommu_get_ops(void)
>>>  {
>>> @@ -106,7 +104,7 @@ int iommu_setup_hpet_msi(struct msi_desc *);
>>>  static inline int iommu_adjust_irq_affinities(void)
>>>  {
>>>      return iommu_ops.adjust_irq_affinities
>>> -           ? iommu_ops.adjust_irq_affinities()
>>> +           ? iommu_call(iommu_ops, adjust_irq_affinities)
>> While this (and other instances below) is x86-only code, where - with
>> the removal of the #ifdef above - we now know the first argument is
>> always ignored, I think it would still better be of the correct type
>> (&iommu_ops). Perhaps the "(void)(ops)" in the macro definitions would
>> better become "ASSERT((ops) == &iommu_ops)", which would check both
>> type (compile time) and value (runtime).
> I'm happy to fold that change if you want.  It ought to optimise out
> completely for being

Bah - sent too early.  "for being tautological."

~Andrew

Re: [PATCH v2.1 8/7] x86/IOMMU: Use altcall, and __initconst_cf_clobber

[Jan Beulich]

On 22.02.2022 12:02, Andrew Cooper wrote:
> On 22/02/2022 10:54, Andrew Cooper wrote:
>> On 22/02/2022 09:29, Jan Beulich wrote:
>>> On 21.02.2022 19:03, Andrew Cooper wrote:
>>>> @@ -106,7 +104,7 @@ int iommu_setup_hpet_msi(struct msi_desc *);
>>>>  static inline int iommu_adjust_irq_affinities(void)
>>>>  {
>>>>      return iommu_ops.adjust_irq_affinities
>>>> -           ? iommu_ops.adjust_irq_affinities()
>>>> +           ? iommu_call(iommu_ops, adjust_irq_affinities)
>>> While this (and other instances below) is x86-only code, where - with
>>> the removal of the #ifdef above - we now know the first argument is
>>> always ignored, I think it would still better be of the correct type
>>> (&iommu_ops). Perhaps the "(void)(ops)" in the macro definitions would
>>> better become "ASSERT((ops) == &iommu_ops)", which would check both
>>> type (compile time) and value (runtime).
>> I'm happy to fold that change if you want.  It ought to optimise out
>> completely for being
> 
> Bah - sent too early.  "for being tautological."

It's tautological here, but not everywhere. But imo the ASSERT() is
good to have anyway, i.e. even if it leaves traces elsewhere in debug
builds.

Jan

Re: [PATCH v2.1 8/7] x86/IOMMU: Use altcall, and __initconst_cf_clobber

[Andrew Cooper]

On 22/02/2022 11:02, Andrew Cooper wrote:
> On 22/02/2022 10:54, Andrew Cooper wrote:
>> On 22/02/2022 09:29, Jan Beulich wrote:
>>> On 21.02.2022 19:03, Andrew Cooper wrote:
>>>> Most IOMMU hooks are already altcall for performance reasons.  Convert the
>>>> rest of them so we can harden all the hooks in Control Flow Integrity
>>>> configurations.  This necessitates the use of iommu_{v,}call() in debug builds
>>>> too.
>>>>
>>>> Move the root iommu_ops from __read_mostly to __ro_after_init now that the
>>>> latter exists.  There is no need for a forward declaration of vtd_ops any
>>>> more, meaning that __initconst_cf_clobber can be used for VTD and AMD.
>>> The connection between the forward declaration and the annotation addition
>>> isn't really clear to me.
>>>
>>>> --- a/xen/arch/x86/include/asm/iommu.h
>>>> +++ b/xen/arch/x86/include/asm/iommu.h
>>>> @@ -72,7 +72,6 @@ struct arch_iommu
>>>>  
>>>>  extern struct iommu_ops iommu_ops;
>>>>  
>>>> -#ifdef NDEBUG
>>>>  # include <asm/alternative.h>
>>>>  # define iommu_call(ops, fn, args...) ({      \
>>>>      (void)(ops);                              \
>>>> @@ -83,7 +82,6 @@ extern struct iommu_ops iommu_ops;
>>>>      (void)(ops);                              \
>>>>      alternative_vcall(iommu_ops.fn, ## args); \
>>>>  })
>>>> -#endif
>>>>  
>>>>  static inline const struct iommu_ops *iommu_get_ops(void)
>>>>  {
>>>> @@ -106,7 +104,7 @@ int iommu_setup_hpet_msi(struct msi_desc *);
>>>>  static inline int iommu_adjust_irq_affinities(void)
>>>>  {
>>>>      return iommu_ops.adjust_irq_affinities
>>>> -           ? iommu_ops.adjust_irq_affinities()
>>>> +           ? iommu_call(iommu_ops, adjust_irq_affinities)
>>> While this (and other instances below) is x86-only code, where - with
>>> the removal of the #ifdef above - we now know the first argument is
>>> always ignored, I think it would still better be of the correct type
>>> (&iommu_ops). Perhaps the "(void)(ops)" in the macro definitions would
>>> better become "ASSERT((ops) == &iommu_ops)", which would check both
>>> type (compile time) and value (runtime).
>> I'm happy to fold that change if you want.  It ought to optimise out
>> completely for being
> Bah - sent too early.  "for being tautological."

Sadly, it turns out it's not.

$ ../scripts/bloat-o-meter -c xen-syms-before xen-syms-after
add/remove: 0/0 grow/shrink: 13/0 up/down: 369/0 (369)
Function                                     old     new   delta
pci_add_device                              1352    1416     +64
pci_remove_device                            716     761     +45
iommu_map                                    341     382     +41
iommu_do_pci_domctl                         1666    1704     +38
iommu_unmap                                  276     310     +34
deassign_device                              353     386     +33
iommu_free_pgtables                          310     329     +19
iommu_iotlb_flush_all                        181     199     +18
iommu_iotlb_flush                            260     278     +18
iommu_hwdom_init                              68      86     +18
iommu_domain_destroy                          54      70     +16
iommu_lookup_page                             53      67     +14
iommu_dump_page_tables                       261     272     +11
Total: Before=2194756, After=2195125, chg +0.02%
add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0)
Data                                         old     new   delta
Total: Before=1699384, After=1699384, chg +0.00%
add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0)
RO Data                                      old     new   delta
Total: Before=0, After=0, chg +0.00%

is the delta in debug builds, while

$ ../scripts/bloat-o-meter -c xen-syms-before xen-syms-after
add/remove: 0/0 grow/shrink: 0/3 up/down: 0/-57 (-57)
Function                                     old     new   delta
iommu_resume                                  34      16     -18
iommu_suspend                                 42      23     -19
iommu_crash_shutdown                          66      46     -20
Total: Before=2112261, After=2112204, chg -0.00%
add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0)
Data                                         old     new   delta
Total: Before=1709424, After=1709424, chg +0.00%
add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0)
RO Data                                      old     new   delta
Total: Before=0, After=0, chg +0.00%

is the delta in release builds.  This is a little weird - it's because
the ASSERT(), in release builds, short circuits the evaluation of its
condition, meaning that the BUG_ON() inside iommu_get_ops() doesn't get
emitted.

Irritatingly, there's no way I can spot to do this check with a
BUILD_BUG_ON(), which would reduce the impact on the debug builds too.

~Andrew

Re: [PATCH v2.1 8/7] x86/IOMMU: Use altcall, and __initconst_cf_clobber

[Jan Beulich]

On 22.02.2022 11:54, Andrew Cooper wrote:
> On 22/02/2022 09:29, Jan Beulich wrote:
>> On 21.02.2022 19:03, Andrew Cooper wrote:
>>> --- a/xen/drivers/passthrough/iommu.c
>>> +++ b/xen/drivers/passthrough/iommu.c
>>> @@ -540,7 +540,7 @@ int __init iommu_setup(void)
>>>  int iommu_suspend()
>>>  {
>>>      if ( iommu_enabled )
>>> -        return iommu_get_ops()->suspend();
>>> +        return iommu_call(iommu_get_ops(), suspend);
>> This use of iommu_get_ops() in such constructs is a pattern we didn't
>> have so far. Perhaps it just looks bogus, and all is fine in reality
>> (apart from the whole idea being wrong for Arm, or really any
>> environment where multiple dissimilar IOMMUs may be in use). Or wait,
>> there are pre-existing cases (just not immediately visible when
>> grep-ing for "iommu_v?call") in iommu_get_reserved_device_memory() and
>> iommu_setup_hpet_msi().
> 
> I think this means your happy(ish) with the change?

Yes. It looks a little odd, but since we have precedents this ought
to be fine.

Jan

[PATCH v2.2 8/7] x86/IOMMU: Use altcall, and __initconst_cf_clobber

[Andrew Cooper]

Most IOMMU hooks are already altcall for performance reasons.  Convert the
rest of them so we can harden all the hooks in Control Flow Integrity
configurations.  This necessitates the use of iommu_{v,}call() in debug builds
too.  Switch to using an ASSERT() as all forms should resolve to &iommu_ops.

Move the root iommu_ops from __read_mostly to __ro_after_init now that the
latter exists.

Since c/s 3330013e6739 ("VT-d / x86: re-arrange cache syncing"), vtd_ops is
not modified and doesn't need a forward declaration, so we can use
__initconst_cf_clobber for both VT-d and AMD.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

v2.1:
 * New
v2.2:
 * Add ASSERT().  Fix indirection of the passed pointer in enable_x2apic().
 * Adjust commit message.
---
 xen/arch/x86/include/asm/iommu.h            | 10 ++++------
 xen/drivers/passthrough/amd/pci_amd_iommu.c |  2 +-
 xen/drivers/passthrough/iommu.c             |  7 ++++---
 xen/drivers/passthrough/vtd/iommu.c         |  3 +--
 xen/drivers/passthrough/x86/iommu.c         |  4 ++--
 5 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/include/asm/iommu.h b/xen/arch/x86/include/asm/iommu.h
index 8a96ba1f097f..d38c33408766 100644
--- a/xen/arch/x86/include/asm/iommu.h
+++ b/xen/arch/x86/include/asm/iommu.h
@@ -72,18 +72,16 @@ struct arch_iommu
 
 extern struct iommu_ops iommu_ops;
 
-#ifdef NDEBUG
 # include <asm/alternative.h>
 # define iommu_call(ops, fn, args...) ({      \
-    (void)(ops);                              \
+    ASSERT((ops) == &iommu_ops);              \
     alternative_call(iommu_ops.fn, ## args);  \
 })
 
 # define iommu_vcall(ops, fn, args...) ({     \
-    (void)(ops);                              \
+    ASSERT((ops) == &iommu_ops);              \
     alternative_vcall(iommu_ops.fn, ## args); \
 })
-#endif
 
 static inline const struct iommu_ops *iommu_get_ops(void)
 {
@@ -106,7 +104,7 @@ int iommu_setup_hpet_msi(struct msi_desc *);
 static inline int iommu_adjust_irq_affinities(void)
 {
     return iommu_ops.adjust_irq_affinities
-           ? iommu_ops.adjust_irq_affinities()
+           ? iommu_call(&iommu_ops, adjust_irq_affinities)
            : 0;
 }
 
@@ -122,7 +120,7 @@ int iommu_enable_x2apic(void);
 static inline void iommu_disable_x2apic(void)
 {
     if ( x2apic_enabled && iommu_ops.disable_x2apic )
-        iommu_ops.disable_x2apic();
+        iommu_vcall(&iommu_ops, disable_x2apic);
 }
 
 int iommu_identity_mapping(struct domain *d, p2m_access_t p2ma,
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index e57f555d00d1..4b59a4efe9b6 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -628,7 +628,7 @@ static void cf_check amd_dump_page_tables(struct domain *d)
                               hd->arch.amd.paging_mode, 0, 0);
 }
 
-static const struct iommu_ops __initconstrel _iommu_ops = {
+static const struct iommu_ops __initconst_cf_clobber _iommu_ops = {
     .init = amd_iommu_domain_init,
     .hwdom_init = amd_iommu_hwdom_init,
     .quarantine_init = amd_iommu_quarantine_init,
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index e220fea72c2f..c6b2c384d1dd 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -540,7 +540,7 @@ int __init iommu_setup(void)
 int iommu_suspend()
 {
     if ( iommu_enabled )
-        return iommu_get_ops()->suspend();
+        return iommu_call(iommu_get_ops(), suspend);
 
     return 0;
 }
@@ -548,7 +548,7 @@ int iommu_suspend()
 void iommu_resume()
 {
     if ( iommu_enabled )
-        iommu_get_ops()->resume();
+        iommu_vcall(iommu_get_ops(), resume);
 }
 
 int iommu_do_domctl(
@@ -578,7 +578,8 @@ void iommu_crash_shutdown(void)
         return;
 
     if ( iommu_enabled )
-        iommu_get_ops()->crash_shutdown();
+        iommu_vcall(iommu_get_ops(), crash_shutdown);
+
     iommu_enabled = false;
 #ifndef iommu_intremap
     iommu_intremap = iommu_intremap_off;
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 56968a06a100..6a65ba1d8271 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -56,7 +56,6 @@ bool __read_mostly iommu_snoop = true;
 
 static unsigned int __read_mostly nr_iommus;
 
-static struct iommu_ops vtd_ops;
 static struct tasklet vtd_fault_tasklet;
 
 static int cf_check setup_hwdom_device(u8 devfn, struct pci_dev *);
@@ -2794,7 +2793,7 @@ static int __init cf_check intel_iommu_quarantine_init(struct domain *d)
     return rc;
 }
 
-static struct iommu_ops __initdata vtd_ops = {
+static const struct iommu_ops __initconst_cf_clobber vtd_ops = {
     .init = intel_iommu_domain_init,
     .hwdom_init = intel_iommu_hwdom_init,
     .quarantine_init = intel_iommu_quarantine_init,
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index ad5f44e13d98..58a422fb5f88 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -27,7 +27,7 @@
 #include <asm/setup.h>
 
 const struct iommu_init_ops *__initdata iommu_init_ops;
-struct iommu_ops __read_mostly iommu_ops;
+struct iommu_ops __ro_after_init iommu_ops;
 bool __read_mostly iommu_non_coherent;
 
 enum iommu_intremap __read_mostly iommu_intremap = iommu_intremap_full;
@@ -129,7 +129,7 @@ int iommu_enable_x2apic(void)
     if ( !iommu_ops.enable_x2apic )
         return -EOPNOTSUPP;
 
-    return iommu_ops.enable_x2apic();
+    return iommu_call(&iommu_ops, enable_x2apic);
 }
 
 void iommu_update_ire_from_apic(
-- 
2.11.0

Re: [PATCH v2.2 8/7] x86/IOMMU: Use altcall, and __initconst_cf_clobber

[Jan Beulich]

On 22.02.2022 12:47, Andrew Cooper wrote:
> Most IOMMU hooks are already altcall for performance reasons.  Convert the
> rest of them so we can harden all the hooks in Control Flow Integrity
> configurations.  This necessitates the use of iommu_{v,}call() in debug builds
> too.  Switch to using an ASSERT() as all forms should resolve to &iommu_ops.
> 
> Move the root iommu_ops from __read_mostly to __ro_after_init now that the
> latter exists.
> 
> Since c/s 3330013e6739 ("VT-d / x86: re-arrange cache syncing"), vtd_ops is
> not modified and doesn't need a forward declaration, so we can use
> __initconst_cf_clobber for both VT-d and AMD.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v2.2 8/7] x86/IOMMU: Use altcall, and __initconst_cf_clobber

[Jan Beulich]

On 22.02.2022 12:47, Andrew Cooper wrote:
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -628,7 +628,7 @@ static void cf_check amd_dump_page_tables(struct domain *d)
>                                hd->arch.amd.paging_mode, 0, 0);
>  }
>  
> -static const struct iommu_ops __initconstrel _iommu_ops = {
> +static const struct iommu_ops __initconst_cf_clobber _iommu_ops = {

Following my initcall related remark on x86'es time.c I'm afraid I don't
see how this and ...

> @@ -2794,7 +2793,7 @@ static int __init cf_check intel_iommu_quarantine_init(struct domain *d)
>      return rc;
>  }
>  
> -static struct iommu_ops __initdata vtd_ops = {
> +static const struct iommu_ops __initconst_cf_clobber vtd_ops = {

... this actually works. But I guess I must be overlooking something, as
I'm sure that you did test the change.

Both ops structures reference a function, through .adjust_irq_affinities,
which isn't __init but which is used (besides here) for an initcall. With
the ENDBR removed by the time initcalls are run, these should cause #CP.

Jan

Re: [PATCH v2.2 8/7] x86/IOMMU: Use altcall, and __initconst_cf_clobber

[Andrew Cooper]

On 25/02/2022 08:24, Jan Beulich wrote:
> On 22.02.2022 12:47, Andrew Cooper wrote:
>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> @@ -628,7 +628,7 @@ static void cf_check amd_dump_page_tables(struct domain *d)
>>                                hd->arch.amd.paging_mode, 0, 0);
>>  }
>>  
>> -static const struct iommu_ops __initconstrel _iommu_ops = {
>> +static const struct iommu_ops __initconst_cf_clobber _iommu_ops = {
> Following my initcall related remark on x86'es time.c I'm afraid I don't
> see how this and ...
>
>> @@ -2794,7 +2793,7 @@ static int __init cf_check intel_iommu_quarantine_init(struct domain *d)
>>      return rc;
>>  }
>>  
>> -static struct iommu_ops __initdata vtd_ops = {
>> +static const struct iommu_ops __initconst_cf_clobber vtd_ops = {
> ... this actually works. But I guess I must be overlooking something, as
> I'm sure that you did test the change.
>
> Both ops structures reference a function, through .adjust_irq_affinities,
> which isn't __init but which is used (besides here) for an initcall. With
> the ENDBR removed by the time initcalls are run, these should cause #CP.

This doesn't explode because the indirect calls are resolved to direct
calls before the ENDBR's are clobbered to NOP4.

But I really do need to write a proper sphinx doc explaining how this
works and the safety considerations.

~Andrew

Re: [PATCH v2.2 8/7] x86/IOMMU: Use altcall, and __initconst_cf_clobber

[Jan Beulich]

On 01.03.2022 15:58, Andrew Cooper wrote:
> On 25/02/2022 08:24, Jan Beulich wrote:
>> On 22.02.2022 12:47, Andrew Cooper wrote:
>>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>> @@ -628,7 +628,7 @@ static void cf_check amd_dump_page_tables(struct domain *d)
>>>                                hd->arch.amd.paging_mode, 0, 0);
>>>  }
>>>  
>>> -static const struct iommu_ops __initconstrel _iommu_ops = {
>>> +static const struct iommu_ops __initconst_cf_clobber _iommu_ops = {
>> Following my initcall related remark on x86'es time.c I'm afraid I don't
>> see how this and ...
>>
>>> @@ -2794,7 +2793,7 @@ static int __init cf_check intel_iommu_quarantine_init(struct domain *d)
>>>      return rc;
>>>  }
>>>  
>>> -static struct iommu_ops __initdata vtd_ops = {
>>> +static const struct iommu_ops __initconst_cf_clobber vtd_ops = {
>> ... this actually works. But I guess I must be overlooking something, as
>> I'm sure that you did test the change.
>>
>> Both ops structures reference a function, through .adjust_irq_affinities,
>> which isn't __init but which is used (besides here) for an initcall. With
>> the ENDBR removed by the time initcalls are run, these should cause #CP.
> 
> This doesn't explode because the indirect calls are resolved to direct
> calls before the ENDBR's are clobbered to NOP4.

I'm afraid I don't understand: The problematic call is in do_initcalls():

    for ( call = __presmp_initcall_end; call < __initcall_end; call++ )
        (*call)();

I don't see how this could be converted to a direct call.

Afaics only pre-SMP initcalls are safe in this regard: do_presmp_initcalls()
is called immediately ahead of alternative_branches().

Isn't this (previously?) working related to your "x86/spec-ctrl: Disable
retpolines with CET-IBT"? With full retpoline, there wouldn't be an
indirect branch, but RET. But with JMP or LFENCE thunks this ought to
fault (already before depending on thunk selection, but unconditionally
now that your change was committed), I would think.

Jan

Re: [PATCH v2.2 8/7] x86/IOMMU: Use altcall, and __initconst_cf_clobber

[Andrew Cooper]

On 02/03/2022 08:10, Jan Beulich wrote:
> On 01.03.2022 15:58, Andrew Cooper wrote:
>> On 25/02/2022 08:24, Jan Beulich wrote:
>>> On 22.02.2022 12:47, Andrew Cooper wrote:
>>>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>>> @@ -628,7 +628,7 @@ static void cf_check amd_dump_page_tables(struct domain *d)
>>>>                                hd->arch.amd.paging_mode, 0, 0);
>>>>  }
>>>>  
>>>> -static const struct iommu_ops __initconstrel _iommu_ops = {
>>>> +static const struct iommu_ops __initconst_cf_clobber _iommu_ops = {
>>> Following my initcall related remark on x86'es time.c I'm afraid I don't
>>> see how this and ...
>>>
>>>> @@ -2794,7 +2793,7 @@ static int __init cf_check intel_iommu_quarantine_init(struct domain *d)
>>>>      return rc;
>>>>  }
>>>>  
>>>> -static struct iommu_ops __initdata vtd_ops = {
>>>> +static const struct iommu_ops __initconst_cf_clobber vtd_ops = {
>>> ... this actually works. But I guess I must be overlooking something, as
>>> I'm sure that you did test the change.
>>>
>>> Both ops structures reference a function, through .adjust_irq_affinities,
>>> which isn't __init but which is used (besides here) for an initcall. With
>>> the ENDBR removed by the time initcalls are run, these should cause #CP.
>> This doesn't explode because the indirect calls are resolved to direct
>> calls before the ENDBR's are clobbered to NOP4.
> I'm afraid I don't understand: The problematic call is in do_initcalls():
>
>     for ( call = __presmp_initcall_end; call < __initcall_end; call++ )
>         (*call)();
>
> I don't see how this could be converted to a direct call.

Oh.  iov_adjust_irq_affinities()'s double use is hiding here.

The safety rule for cf_clobber is that there must not be any
non-alt-called callers.  We need to fix it:

diff --git a/xen/drivers/passthrough/amd/iommu_init.c
b/xen/drivers/passthrough/amd/iommu_init.c
index 657c7f619a51..b1af5085efda 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -831,7 +831,12 @@ int cf_check iov_adjust_irq_affinities(void)
 
     return 0;
 }
-__initcall(iov_adjust_irq_affinities);
+
+int cf_check __init initcall_iov_adjust_irq_affinities(void)
+{
+    return iommu_call(&iommu_ops, adjust_irq_affinities);
+}
+__initcall(initcall_iov_adjust_irq_affinities);
 
 /*
  * Family15h Model 10h-1fh erratum 746 (IOMMU Logging May Stall
Translations)


> Afaics only pre-SMP initcalls are safe in this regard: do_presmp_initcalls()
> is called immediately ahead of alternative_branches().
>
> Isn't this (previously?) working related to your "x86/spec-ctrl: Disable
> retpolines with CET-IBT"?

No.  It's because AMD CPUs don't have CET-IBT at this juncture, and will
never encounter a faulting situation.

This is exactly what the UD1 adjustment in Linux are intended to spot,
because that would cause all AMD hardware to explode, not just the
IBT-enabled ones.

~Andrew

Re: [PATCH v2.2 8/7] x86/IOMMU: Use altcall, and __initconst_cf_clobber

[Jan Beulich]

On 02.03.2022 11:12, Andrew Cooper wrote:
> On 02/03/2022 08:10, Jan Beulich wrote:
>> On 01.03.2022 15:58, Andrew Cooper wrote:
>>> On 25/02/2022 08:24, Jan Beulich wrote:
>>>> On 22.02.2022 12:47, Andrew Cooper wrote:
>>>>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>>>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>>>> @@ -628,7 +628,7 @@ static void cf_check amd_dump_page_tables(struct domain *d)
>>>>>                                hd->arch.amd.paging_mode, 0, 0);
>>>>>  }
>>>>>  
>>>>> -static const struct iommu_ops __initconstrel _iommu_ops = {
>>>>> +static const struct iommu_ops __initconst_cf_clobber _iommu_ops = {
>>>> Following my initcall related remark on x86'es time.c I'm afraid I don't
>>>> see how this and ...
>>>>
>>>>> @@ -2794,7 +2793,7 @@ static int __init cf_check intel_iommu_quarantine_init(struct domain *d)
>>>>>      return rc;
>>>>>  }
>>>>>  
>>>>> -static struct iommu_ops __initdata vtd_ops = {
>>>>> +static const struct iommu_ops __initconst_cf_clobber vtd_ops = {
>>>> ... this actually works. But I guess I must be overlooking something, as
>>>> I'm sure that you did test the change.
>>>>
>>>> Both ops structures reference a function, through .adjust_irq_affinities,
>>>> which isn't __init but which is used (besides here) for an initcall. With
>>>> the ENDBR removed by the time initcalls are run, these should cause #CP.
>>> This doesn't explode because the indirect calls are resolved to direct
>>> calls before the ENDBR's are clobbered to NOP4.
>> I'm afraid I don't understand: The problematic call is in do_initcalls():
>>
>>     for ( call = __presmp_initcall_end; call < __initcall_end; call++ )
>>         (*call)();
>>
>> I don't see how this could be converted to a direct call.
> 
> Oh.  iov_adjust_irq_affinities()'s double use is hiding here.
> 
> The safety rule for cf_clobber is that there must not be any
> non-alt-called callers.  We need to fix it:
> 
> diff --git a/xen/drivers/passthrough/amd/iommu_init.c
> b/xen/drivers/passthrough/amd/iommu_init.c
> index 657c7f619a51..b1af5085efda 100644
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -831,7 +831,12 @@ int cf_check iov_adjust_irq_affinities(void)
>  
>      return 0;
>  }
> -__initcall(iov_adjust_irq_affinities);
> +
> +int cf_check __init initcall_iov_adjust_irq_affinities(void)
> +{
> +    return iommu_call(&iommu_ops, adjust_irq_affinities);
> +}
> +__initcall(initcall_iov_adjust_irq_affinities);
>  
>  /*
>   * Family15h Model 10h-1fh erratum 746 (IOMMU Logging May Stall
> Translations)
> 
> 
>> Afaics only pre-SMP initcalls are safe in this regard: do_presmp_initcalls()
>> is called immediately ahead of alternative_branches().
>>
>> Isn't this (previously?) working related to your "x86/spec-ctrl: Disable
>> retpolines with CET-IBT"?
> 
> No.  It's because AMD CPUs don't have CET-IBT at this juncture, and will
> never encounter a faulting situation.

I'm still lost. An exactly matching construct exists in VT-d code (and
my initial comment also was on VT-d). The AMD one is actually a clone
of that much older one. The initcall really wants to move to vendor
independent code, but I'd still like to understand why no fault was
ever observed.

Jan

Re: [PATCH v2.2 8/7] x86/IOMMU: Use altcall, and __initconst_cf_clobber

[Andrew Cooper]

On 02/03/2022 10:34, Jan Beulich wrote:
> On 02.03.2022 11:12, Andrew Cooper wrote:
>> On 02/03/2022 08:10, Jan Beulich wrote:
>>> On 01.03.2022 15:58, Andrew Cooper wrote:
>>>> On 25/02/2022 08:24, Jan Beulich wrote:
>>>>> On 22.02.2022 12:47, Andrew Cooper wrote:
>>>>>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>>>>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>>>>> @@ -628,7 +628,7 @@ static void cf_check amd_dump_page_tables(struct domain *d)
>>>>>>                                hd->arch.amd.paging_mode, 0, 0);
>>>>>>  }
>>>>>>  
>>>>>> -static const struct iommu_ops __initconstrel _iommu_ops = {
>>>>>> +static const struct iommu_ops __initconst_cf_clobber _iommu_ops = {
>>>>> Following my initcall related remark on x86'es time.c I'm afraid I don't
>>>>> see how this and ...
>>>>>
>>>>>> @@ -2794,7 +2793,7 @@ static int __init cf_check intel_iommu_quarantine_init(struct domain *d)
>>>>>>      return rc;
>>>>>>  }
>>>>>>  
>>>>>> -static struct iommu_ops __initdata vtd_ops = {
>>>>>> +static const struct iommu_ops __initconst_cf_clobber vtd_ops = {
>>>>> ... this actually works. But I guess I must be overlooking something, as
>>>>> I'm sure that you did test the change.
>>>>>
>>>>> Both ops structures reference a function, through .adjust_irq_affinities,
>>>>> which isn't __init but which is used (besides here) for an initcall. With
>>>>> the ENDBR removed by the time initcalls are run, these should cause #CP.
>>>> This doesn't explode because the indirect calls are resolved to direct
>>>> calls before the ENDBR's are clobbered to NOP4.
>>> I'm afraid I don't understand: The problematic call is in do_initcalls():
>>>
>>>     for ( call = __presmp_initcall_end; call < __initcall_end; call++ )
>>>         (*call)();
>>>
>>> I don't see how this could be converted to a direct call.
>> Oh.  iov_adjust_irq_affinities()'s double use is hiding here.
>>
>> The safety rule for cf_clobber is that there must not be any
>> non-alt-called callers.  We need to fix it:
>>
>> diff --git a/xen/drivers/passthrough/amd/iommu_init.c
>> b/xen/drivers/passthrough/amd/iommu_init.c
>> index 657c7f619a51..b1af5085efda 100644
>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>> @@ -831,7 +831,12 @@ int cf_check iov_adjust_irq_affinities(void)
>>  
>>      return 0;
>>  }
>> -__initcall(iov_adjust_irq_affinities);
>> +
>> +int cf_check __init initcall_iov_adjust_irq_affinities(void)
>> +{
>> +    return iommu_call(&iommu_ops, adjust_irq_affinities);
>> +}
>> +__initcall(initcall_iov_adjust_irq_affinities);
>>  
>>  /*
>>   * Family15h Model 10h-1fh erratum 746 (IOMMU Logging May Stall
>> Translations)
>>
>>
>>> Afaics only pre-SMP initcalls are safe in this regard: do_presmp_initcalls()
>>> is called immediately ahead of alternative_branches().
>>>
>>> Isn't this (previously?) working related to your "x86/spec-ctrl: Disable
>>> retpolines with CET-IBT"?
>> No.  It's because AMD CPUs don't have CET-IBT at this juncture, and will
>> never encounter a faulting situation.
> I'm still lost. An exactly matching construct exists in VT-d code (and
> my initial comment also was on VT-d). The AMD one is actually a clone
> of that much older one. The initcall really wants to move to vendor
> independent code, but I'd still like to understand why no fault was
> ever observed.

Lovely.  It's got a vtd infix which is why it escaped my grep.

And yes, I really would expect that to explode on my test system...

~Andrew

Re: [PATCH v2.2 8/7] x86/IOMMU: Use altcall, and __initconst_cf_clobber

[Andrew Cooper]

On 02/03/2022 13:39, Andrew Cooper wrote:
> On 02/03/2022 10:34, Jan Beulich wrote:
>> On 02.03.2022 11:12, Andrew Cooper wrote:
>>> On 02/03/2022 08:10, Jan Beulich wrote:
>>>> On 01.03.2022 15:58, Andrew Cooper wrote:
>>>>> On 25/02/2022 08:24, Jan Beulich wrote:
>>>>>> On 22.02.2022 12:47, Andrew Cooper wrote:
>>>>>>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>>>>>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>>>>>> @@ -628,7 +628,7 @@ static void cf_check amd_dump_page_tables(struct domain *d)
>>>>>>>                                hd->arch.amd.paging_mode, 0, 0);
>>>>>>>  }
>>>>>>>  
>>>>>>> -static const struct iommu_ops __initconstrel _iommu_ops = {
>>>>>>> +static const struct iommu_ops __initconst_cf_clobber _iommu_ops = {
>>>>>> Following my initcall related remark on x86'es time.c I'm afraid I don't
>>>>>> see how this and ...
>>>>>>
>>>>>>> @@ -2794,7 +2793,7 @@ static int __init cf_check intel_iommu_quarantine_init(struct domain *d)
>>>>>>>      return rc;
>>>>>>>  }
>>>>>>>  
>>>>>>> -static struct iommu_ops __initdata vtd_ops = {
>>>>>>> +static const struct iommu_ops __initconst_cf_clobber vtd_ops = {
>>>>>> ... this actually works. But I guess I must be overlooking something, as
>>>>>> I'm sure that you did test the change.
>>>>>>
>>>>>> Both ops structures reference a function, through .adjust_irq_affinities,
>>>>>> which isn't __init but which is used (besides here) for an initcall. With
>>>>>> the ENDBR removed by the time initcalls are run, these should cause #CP.
>>>>> This doesn't explode because the indirect calls are resolved to direct
>>>>> calls before the ENDBR's are clobbered to NOP4.
>>>> I'm afraid I don't understand: The problematic call is in do_initcalls():
>>>>
>>>>     for ( call = __presmp_initcall_end; call < __initcall_end; call++ )
>>>>         (*call)();
>>>>
>>>> I don't see how this could be converted to a direct call.
>>> Oh.  iov_adjust_irq_affinities()'s double use is hiding here.
>>>
>>> The safety rule for cf_clobber is that there must not be any
>>> non-alt-called callers.  We need to fix it:
>>>
>>> diff --git a/xen/drivers/passthrough/amd/iommu_init.c
>>> b/xen/drivers/passthrough/amd/iommu_init.c
>>> index 657c7f619a51..b1af5085efda 100644
>>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>>> @@ -831,7 +831,12 @@ int cf_check iov_adjust_irq_affinities(void)
>>>  
>>>      return 0;
>>>  }
>>> -__initcall(iov_adjust_irq_affinities);
>>> +
>>> +int cf_check __init initcall_iov_adjust_irq_affinities(void)
>>> +{
>>> +    return iommu_call(&iommu_ops, adjust_irq_affinities);
>>> +}
>>> +__initcall(initcall_iov_adjust_irq_affinities);
>>>  
>>>  /*
>>>   * Family15h Model 10h-1fh erratum 746 (IOMMU Logging May Stall
>>> Translations)
>>>
>>>
>>>> Afaics only pre-SMP initcalls are safe in this regard: do_presmp_initcalls()
>>>> is called immediately ahead of alternative_branches().
>>>>
>>>> Isn't this (previously?) working related to your "x86/spec-ctrl: Disable
>>>> retpolines with CET-IBT"?
>>> No.  It's because AMD CPUs don't have CET-IBT at this juncture, and will
>>> never encounter a faulting situation.
>> I'm still lost. An exactly matching construct exists in VT-d code (and
>> my initial comment also was on VT-d). The AMD one is actually a clone
>> of that much older one. The initcall really wants to move to vendor
>> independent code, but I'd still like to understand why no fault was
>> ever observed.
> Lovely.  It's got a vtd infix which is why it escaped my grep.
>
> And yes, I really would expect that to explode on my test system...

And the answer is that the linker script collects .init.rodata.* ahead
of the dedicated .init.rodata.cf_clobber section.

Meaning that __initdata_cf_clobber works as expected but
__initconst_cf_clobber is a no-op.

~Andrew