| Message ID | 20221004084335.2838-3-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/pv: sanitize xen pv guest msr accesses | expand |
On 04.10.2022 10:43, Juergen Gross wrote: > Refactor and rename xen_read_msr_safe() and xen_write_msr_safe() to > support both cases of MSR accesses, safe ones and potentially GP-fault > generating ones. > > This will prepare to no longer swallow GPs silently in xen_read_msr() > and xen_write_msr(). > > Signed-off-by: Juergen Gross <jgross@suse.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> Albeit ... > @@ -933,23 +937,39 @@ static u64 xen_read_msr_safe(unsigned int msr, int *err) > return val; > } > > -static int xen_write_msr_safe(unsigned int msr, unsigned low, unsigned high) > +static void set_seg(unsigned int which, unsigned int low, unsigned int high, > + int *err) > { > - int ret; > - unsigned int which; > - u64 base; > + u64 base = ((u64)high << 32) | low; > + > + if (HYPERVISOR_set_segment_base(which, base) == 0) > + return; > > - ret = 0; > + if (err) > + *err = -EIO; ... I don't see a good reason to override the error code handed to us by the hypervisor here; I do realize though that this ... > + else > + WARN(1, "Xen set_segment_base(%u, %llx) failed\n", which, base); > +} > > +/* > + * Support write_msr_safe() and write_msr() semantics. > + * With err == NULL write_msr() semantics are selected. > + * Supplying an err pointer requires err to be pre-initialized with 0. > + */ > +static void xen_do_write_msr(unsigned int msr, unsigned int low, > + unsigned int high, int *err) > +{ > switch (msr) { > - case MSR_FS_BASE: which = SEGBASE_FS; goto set; > - case MSR_KERNEL_GS_BASE: which = SEGBASE_GS_USER; goto set; > - case MSR_GS_BASE: which = SEGBASE_GS_KERNEL; goto set; > - > - set: > - base = ((u64)high << 32) | low; > - if (HYPERVISOR_set_segment_base(which, base) != 0) > - ret = -EIO; ... was this way before. Jan
On 04.10.22 13:03, Jan Beulich wrote: > On 04.10.2022 10:43, Juergen Gross wrote: >> Refactor and rename xen_read_msr_safe() and xen_write_msr_safe() to >> support both cases of MSR accesses, safe ones and potentially GP-fault >> generating ones. >> >> This will prepare to no longer swallow GPs silently in xen_read_msr() >> and xen_write_msr(). >> >> Signed-off-by: Juergen Gross <jgross@suse.com> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > Albeit ... > >> @@ -933,23 +937,39 @@ static u64 xen_read_msr_safe(unsigned int msr, int *err) >> return val; >> } >> >> -static int xen_write_msr_safe(unsigned int msr, unsigned low, unsigned high) >> +static void set_seg(unsigned int which, unsigned int low, unsigned int high, >> + int *err) >> { >> - int ret; >> - unsigned int which; >> - u64 base; >> + u64 base = ((u64)high << 32) | low; >> + >> + if (HYPERVISOR_set_segment_base(which, base) == 0) >> + return; >> >> - ret = 0; >> + if (err) >> + *err = -EIO; > > ... I don't see a good reason to override the error code handed to us > by the hypervisor here; I do realize though that this ... > >> + else >> + WARN(1, "Xen set_segment_base(%u, %llx) failed\n", which, base); >> +} >> >> +/* >> + * Support write_msr_safe() and write_msr() semantics. >> + * With err == NULL write_msr() semantics are selected. >> + * Supplying an err pointer requires err to be pre-initialized with 0. >> + */ >> +static void xen_do_write_msr(unsigned int msr, unsigned int low, >> + unsigned int high, int *err) >> +{ >> switch (msr) { >> - case MSR_FS_BASE: which = SEGBASE_FS; goto set; >> - case MSR_KERNEL_GS_BASE: which = SEGBASE_GS_USER; goto set; >> - case MSR_GS_BASE: which = SEGBASE_GS_KERNEL; goto set; >> - >> - set: >> - base = ((u64)high << 32) | low; >> - if (HYPERVISOR_set_segment_base(which, base) != 0) >> - ret = -EIO; > > ... was this way before. And on bare metal write_msr_safe() will return -EIO, too. Juergen
diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c index 9b1a58dda935..d5b0844a1b7c 100644 --- a/arch/x86/xen/enlighten_pv.c +++ b/arch/x86/xen/enlighten_pv.c @@ -917,14 +917,18 @@ static void xen_write_cr4(unsigned long cr4) native_write_cr4(cr4); } -static u64 xen_read_msr_safe(unsigned int msr, int *err) +static u64 xen_do_read_msr(unsigned int msr, int *err) { - u64 val; + u64 val = 0; /* Avoid uninitialized value for safe variant. */ if (pmu_msr_read(msr, &val, err)) return val; - val = native_read_msr_safe(msr, err); + if (err) + val = native_read_msr_safe(msr, err); + else + val = native_read_msr(msr); + switch (msr) { case MSR_IA32_APICBASE: val &= ~X2APIC_ENABLE; @@ -933,23 +937,39 @@ static u64 xen_read_msr_safe(unsigned int msr, int *err) return val; } -static int xen_write_msr_safe(unsigned int msr, unsigned low, unsigned high) +static void set_seg(unsigned int which, unsigned int low, unsigned int high, + int *err) { - int ret; - unsigned int which; - u64 base; + u64 base = ((u64)high << 32) | low; + + if (HYPERVISOR_set_segment_base(which, base) == 0) + return; - ret = 0; + if (err) + *err = -EIO; + else + WARN(1, "Xen set_segment_base(%u, %llx) failed\n", which, base); +} +/* + * Support write_msr_safe() and write_msr() semantics. + * With err == NULL write_msr() semantics are selected. + * Supplying an err pointer requires err to be pre-initialized with 0. + */ +static void xen_do_write_msr(unsigned int msr, unsigned int low, + unsigned int high, int *err) +{ switch (msr) { - case MSR_FS_BASE: which = SEGBASE_FS; goto set; - case MSR_KERNEL_GS_BASE: which = SEGBASE_GS_USER; goto set; - case MSR_GS_BASE: which = SEGBASE_GS_KERNEL; goto set; - - set: - base = ((u64)high << 32) | low; - if (HYPERVISOR_set_segment_base(which, base) != 0) - ret = -EIO; + case MSR_FS_BASE: + set_seg(SEGBASE_FS, low, high, err); + break; + + case MSR_KERNEL_GS_BASE: + set_seg(SEGBASE_GS_USER, low, high, err); + break; + + case MSR_GS_BASE: + set_seg(SEGBASE_GS_KERNEL, low, high, err); break; case MSR_STAR: @@ -965,11 +985,28 @@ static int xen_write_msr_safe(unsigned int msr, unsigned low, unsigned high) break; default: - if (!pmu_msr_write(msr, low, high, &ret)) - ret = native_write_msr_safe(msr, low, high); + if (!pmu_msr_write(msr, low, high, err)) { + if (err) + *err = native_write_msr_safe(msr, low, high); + else + native_write_msr(msr, low, high); + } } +} + +static u64 xen_read_msr_safe(unsigned int msr, int *err) +{ + return xen_do_read_msr(msr, err); +} + +static int xen_write_msr_safe(unsigned int msr, unsigned int low, + unsigned int high) +{ + int err = 0; + + xen_do_write_msr(msr, low, high, &err); - return ret; + return err; } static u64 xen_read_msr(unsigned int msr)
Refactor and rename xen_read_msr_safe() and xen_write_msr_safe() to support both cases of MSR accesses, safe ones and potentially GP-fault generating ones. This will prepare to no longer swallow GPs silently in xen_read_msr() and xen_write_msr(). Signed-off-by: Juergen Gross <jgross@suse.com> --- V2: - init val in xen_do_read_msr() to 0 (Jan Beulich) --- arch/x86/xen/enlighten_pv.c | 75 +++++++++++++++++++++++++++---------- 1 file changed, 56 insertions(+), 19 deletions(-)