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