diff mbox series

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

Message ID 20220214125632.24563-6-andrew.cooper3@citrix.com (mailing list archive)
State New, archived
Headers show
Series x86: Further harden function pointers | expand

Commit Message

Andrew Cooper Feb. 14, 2022, 12:56 p.m. UTC 
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(-)

Comments

Jan Beulich Feb. 14, 2022, 1:10 p.m. UTC | #1 
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
Andrew Cooper Feb. 14, 2022, 1:35 p.m. UTC | #2 
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
Andrew Cooper Feb. 14, 2022, 4:39 p.m. UTC | #3 
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
Jan Beulich Feb. 14, 2022, 4:45 p.m. UTC | #4 
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
diff mbox series

Patch

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,