| Message ID | 13d1d621-21db-0e59-6603-2b22b6a9d180@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/PV: avoid speculation abuse through guest accessors plus ... | expand |
On Thu, Jan 14, 2021 at 04:04:11PM +0100, Jan Beulich wrote: > The "guest" variants are intended to work with (potentially) fully guest > controlled addresses, while the "unsafe" variants are not. Just to clarify, both work against user addresses, but guest variants need to be more careful because the guest provided address can also be modified? I'm trying to understand the difference between "fully guest controlled" and "guest controlled". Maybe an example would help clarify. Thanks, Roger.
On 05.02.2021 16:43, Roger Pau Monné wrote: > On Thu, Jan 14, 2021 at 04:04:11PM +0100, Jan Beulich wrote: >> The "guest" variants are intended to work with (potentially) fully guest >> controlled addresses, while the "unsafe" variants are not. > > Just to clarify, both work against user addresses, but guest variants > need to be more careful because the guest provided address can also be > modified? > > I'm trying to understand the difference between "fully guest > controlled" and "guest controlled". Not exactly, not. "unsafe" means access to anything which may fault, guest controlled or not. do_invalid_op()'s reading of the insn stream is a good example - the faulting insn there isn't guest controlled at all, but we still want to be careful when trying to read these bytes, as we don't want to fully trust %rip there. Jan
On Fri, Feb 05, 2021 at 05:13:22PM +0100, Jan Beulich wrote: > On 05.02.2021 16:43, Roger Pau Monné wrote: > > On Thu, Jan 14, 2021 at 04:04:11PM +0100, Jan Beulich wrote: > >> The "guest" variants are intended to work with (potentially) fully guest > >> controlled addresses, while the "unsafe" variants are not. > > > > Just to clarify, both work against user addresses, but guest variants > > need to be more careful because the guest provided address can also be > > modified? > > > > I'm trying to understand the difference between "fully guest > > controlled" and "guest controlled". > > Not exactly, not. "unsafe" means access to anything which may > fault, guest controlled or not. do_invalid_op()'s reading of > the insn stream is a good example - the faulting insn there > isn't guest controlled at all, but we still want to be careful > when trying to read these bytes, as we don't want to fully > trust %rip there. Would it make sense to threat everything as 'guest' accesses for the sake of not having this difference? I think having two accessors it's likely to cause confusion and could possibly lead to the wrong one being used in unexpected contexts. Does it add a too big performance penalty to always use the most restrictive one? Thanks, Roger.
On 05.02.2021 17:18, Roger Pau Monné wrote: > On Fri, Feb 05, 2021 at 05:13:22PM +0100, Jan Beulich wrote: >> On 05.02.2021 16:43, Roger Pau Monné wrote: >>> On Thu, Jan 14, 2021 at 04:04:11PM +0100, Jan Beulich wrote: >>>> The "guest" variants are intended to work with (potentially) fully guest >>>> controlled addresses, while the "unsafe" variants are not. >>> >>> Just to clarify, both work against user addresses, but guest variants >>> need to be more careful because the guest provided address can also be >>> modified? >>> >>> I'm trying to understand the difference between "fully guest >>> controlled" and "guest controlled". >> >> Not exactly, not. "unsafe" means access to anything which may >> fault, guest controlled or not. do_invalid_op()'s reading of >> the insn stream is a good example - the faulting insn there >> isn't guest controlled at all, but we still want to be careful >> when trying to read these bytes, as we don't want to fully >> trust %rip there. > > Would it make sense to threat everything as 'guest' accesses for the > sake of not having this difference? That's what we've been doing until now. It is the purpose of this change to allow the two to behave differently. > I think having two accessors it's likely to cause confusion and could > possibly lead to the wrong one being used in unexpected contexts. Does > it add a too big performance penalty to always use the most > restrictive one? The problem is the most restrictive one is going to be too restrictive - we wouldn't be able to access Xen space anymore e.g. from the place pointed at above as example. This is because for guest accesses (but not for "unsafe" ones) we're going to divert them into non-canonical space (and hence make speculation impossible, as such an access would fault) if it would touch Xen space. Jan
On Fri, Feb 05, 2021 at 05:26:33PM +0100, Jan Beulich wrote: > On 05.02.2021 17:18, Roger Pau Monné wrote: > > On Fri, Feb 05, 2021 at 05:13:22PM +0100, Jan Beulich wrote: > >> On 05.02.2021 16:43, Roger Pau Monné wrote: > >>> On Thu, Jan 14, 2021 at 04:04:11PM +0100, Jan Beulich wrote: > >>>> The "guest" variants are intended to work with (potentially) fully guest > >>>> controlled addresses, while the "unsafe" variants are not. > >>> > >>> Just to clarify, both work against user addresses, but guest variants > >>> need to be more careful because the guest provided address can also be > >>> modified? > >>> > >>> I'm trying to understand the difference between "fully guest > >>> controlled" and "guest controlled". > >> > >> Not exactly, not. "unsafe" means access to anything which may > >> fault, guest controlled or not. do_invalid_op()'s reading of > >> the insn stream is a good example - the faulting insn there > >> isn't guest controlled at all, but we still want to be careful > >> when trying to read these bytes, as we don't want to fully > >> trust %rip there. Oh, I see. It's possible that %rip points to an unmapped address there, and we need to be careful when reading, even if the value of %rip cannot be controlled by the guest and can legitimacy point to Xen's address space. > > Would it make sense to threat everything as 'guest' accesses for the > > sake of not having this difference? > > That's what we've been doing until now. It is the purpose of > this change to allow the two to behave differently. > > > I think having two accessors it's likely to cause confusion and could > > possibly lead to the wrong one being used in unexpected contexts. Does > > it add a too big performance penalty to always use the most > > restrictive one? > > The problem is the most restrictive one is going to be too > restrictive - we wouldn't be able to access Xen space anymore > e.g. from the place pointed at above as example. This is > because for guest accesses (but not for "unsafe" ones) we're > going to divert them into non-canonical space (and hence make > speculation impossible, as such an access would fault) if it > would touch Xen space. Yes, I understand now. I think it would have been helpful (for me) to have the first sentence as: The "guest" variants are intended to work with (potentially) fully guest controlled addresses, while the "unsafe" variants are expected to be used in order to access addresses not under the guest control, but that could trigger faults anyway (like accessing the instruction stream in do_invalid_op). Thanks, Roger.
On 09.02.2021 14:07, Roger Pau Monné wrote: > On Fri, Feb 05, 2021 at 05:26:33PM +0100, Jan Beulich wrote: >> On 05.02.2021 17:18, Roger Pau Monné wrote: >>> On Fri, Feb 05, 2021 at 05:13:22PM +0100, Jan Beulich wrote: >>>> On 05.02.2021 16:43, Roger Pau Monné wrote: >>>>> On Thu, Jan 14, 2021 at 04:04:11PM +0100, Jan Beulich wrote: >>>>>> The "guest" variants are intended to work with (potentially) fully guest >>>>>> controlled addresses, while the "unsafe" variants are not. >>>>> >>>>> Just to clarify, both work against user addresses, but guest variants >>>>> need to be more careful because the guest provided address can also be >>>>> modified? >>>>> >>>>> I'm trying to understand the difference between "fully guest >>>>> controlled" and "guest controlled". >>>> >>>> Not exactly, not. "unsafe" means access to anything which may >>>> fault, guest controlled or not. do_invalid_op()'s reading of >>>> the insn stream is a good example - the faulting insn there >>>> isn't guest controlled at all, but we still want to be careful >>>> when trying to read these bytes, as we don't want to fully >>>> trust %rip there. > > Oh, I see. It's possible that %rip points to an unmapped address > there, and we need to be careful when reading, even if the value of > %rip cannot be controlled by the guest and can legitimacy point to > Xen's address space. > >>> Would it make sense to threat everything as 'guest' accesses for the >>> sake of not having this difference? >> >> That's what we've been doing until now. It is the purpose of >> this change to allow the two to behave differently. >> >>> I think having two accessors it's likely to cause confusion and could >>> possibly lead to the wrong one being used in unexpected contexts. Does >>> it add a too big performance penalty to always use the most >>> restrictive one? >> >> The problem is the most restrictive one is going to be too >> restrictive - we wouldn't be able to access Xen space anymore >> e.g. from the place pointed at above as example. This is >> because for guest accesses (but not for "unsafe" ones) we're >> going to divert them into non-canonical space (and hence make >> speculation impossible, as such an access would fault) if it >> would touch Xen space. > > Yes, I understand now. I think it would have been helpful (for me) to > have the first sentence as: > > The "guest" variants are intended to work with (potentially) fully guest > controlled addresses, while the "unsafe" variants are expected to be > used in order to access addresses not under the guest control, but > that could trigger faults anyway (like accessing the instruction > stream in do_invalid_op). I can use some of this, but in particular "access addresses not under the guest control" isn't entirely correct. The question isn't whether there's a guest control aspect, but which part of the address space the addresses are in. See specifically x86/PV: use get_unsafe() instead of copy_from_unsafe()" for two pretty good examples. The address within the linear page tables are - in a way at least - still somewhat guest controlled, but we're deliberately accessing Xen virtual addresses there. Jan
On Tue, Feb 09, 2021 at 02:15:18PM +0100, Jan Beulich wrote: > On 09.02.2021 14:07, Roger Pau Monné wrote: > > On Fri, Feb 05, 2021 at 05:26:33PM +0100, Jan Beulich wrote: > >> On 05.02.2021 17:18, Roger Pau Monné wrote: > >>> On Fri, Feb 05, 2021 at 05:13:22PM +0100, Jan Beulich wrote: > >>>> On 05.02.2021 16:43, Roger Pau Monné wrote: > >>>>> On Thu, Jan 14, 2021 at 04:04:11PM +0100, Jan Beulich wrote: > >>>>>> The "guest" variants are intended to work with (potentially) fully guest > >>>>>> controlled addresses, while the "unsafe" variants are not. > >>>>> > >>>>> Just to clarify, both work against user addresses, but guest variants > >>>>> need to be more careful because the guest provided address can also be > >>>>> modified? > >>>>> > >>>>> I'm trying to understand the difference between "fully guest > >>>>> controlled" and "guest controlled". > >>>> > >>>> Not exactly, not. "unsafe" means access to anything which may > >>>> fault, guest controlled or not. do_invalid_op()'s reading of > >>>> the insn stream is a good example - the faulting insn there > >>>> isn't guest controlled at all, but we still want to be careful > >>>> when trying to read these bytes, as we don't want to fully > >>>> trust %rip there. > > > > Oh, I see. It's possible that %rip points to an unmapped address > > there, and we need to be careful when reading, even if the value of > > %rip cannot be controlled by the guest and can legitimacy point to > > Xen's address space. > > > >>> Would it make sense to threat everything as 'guest' accesses for the > >>> sake of not having this difference? > >> > >> That's what we've been doing until now. It is the purpose of > >> this change to allow the two to behave differently. > >> > >>> I think having two accessors it's likely to cause confusion and could > >>> possibly lead to the wrong one being used in unexpected contexts. Does > >>> it add a too big performance penalty to always use the most > >>> restrictive one? > >> > >> The problem is the most restrictive one is going to be too > >> restrictive - we wouldn't be able to access Xen space anymore > >> e.g. from the place pointed at above as example. This is > >> because for guest accesses (but not for "unsafe" ones) we're > >> going to divert them into non-canonical space (and hence make > >> speculation impossible, as such an access would fault) if it > >> would touch Xen space. > > > > Yes, I understand now. I think it would have been helpful (for me) to > > have the first sentence as: > > > > The "guest" variants are intended to work with (potentially) fully guest > > controlled addresses, while the "unsafe" variants are expected to be > > used in order to access addresses not under the guest control, but > > that could trigger faults anyway (like accessing the instruction > > stream in do_invalid_op). > > I can use some of this, but in particular "access addresses not > under the guest control" isn't entirely correct. The question isn't > whether there's a guest control aspect, but which part of the > address space the addresses are in. See specifically x86/PV: use > get_unsafe() instead of copy_from_unsafe()" for two pretty good > examples. The address within the linear page tables are - in a > way at least - still somewhat guest controlled, but we're > deliberately accessing Xen virtual addresses there. OK, could this be somehow added to the commit message then? Maybe it would be better to have something like: The "guest" variants are intended to work with addresses belonging to the guest address space, while the "unsafe" variants should be used for addresses that fall into the Xen address space. I think it's important to list exactly how the distinction between the guest/unsafe accessors is made, or else it's impossible to review that the changes done here are correct. Thanks, Roger.
On Thu, Jan 14, 2021 at 04:04:11PM +0100, Jan Beulich wrote: > The "guest" variants are intended to work with (potentially) fully guest > controlled addresses, while the "unsafe" variants are not. (For > descriptor table accesses the low bits of the addresses may still be > guest controlled, but this still won't allow speculation to "escape" > into unwanted areas.) Descriptor table is also in guest address space, and hence would be fine using the _guest accessors? (even if not in guest control and thus unsuitable as an speculation vector) > Subsequently we will want them to have different > behavior, so as first step identify which one is which. For now, both > groups of constructs alias one another. > > Double underscore prefixes are retained only on __{get,put}_guest(), to > allow still distinguishing them from their "checking" counterparts once > they also get renamed (to {get,put}_guest()). > > Since for them it's almost a full re-write, move what becomes > {get,put}_unsafe_size() into the "common" uaccess.h (x86_64/*.h should > disappear at some point anyway). > > In __copy_to_user() one of the two casts in each put_guest_size() > invocation gets dropped. They're not needed and did break symmetry with > __copy_from_user(). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/mm/shadow/multi.c > +++ b/xen/arch/x86/mm/shadow/multi.c > @@ -776,9 +776,9 @@ shadow_write_entries(void *d, void *s, i > /* Because we mirror access rights at all levels in the shadow, an > * l2 (or higher) entry with the RW bit cleared will leave us with > * no write access through the linear map. > - * We detect that by writing to the shadow with __put_user() and > + * We detect that by writing to the shadow with put_unsafe() and > * using map_domain_page() to get a writeable mapping if we need to. */ > - if ( __put_user(*dst, dst) ) > + if ( put_unsafe(*dst, dst) ) > { > perfc_incr(shadow_linear_map_failed); > map = map_domain_page(mfn); > --- a/xen/arch/x86/pv/emul-gate-op.c > +++ b/xen/arch/x86/pv/emul-gate-op.c > @@ -40,7 +40,7 @@ static int read_gate_descriptor(unsigned > ((gate_sel >> 3) + !is_pv_32bit_vcpu(v) >= > (gate_sel & 4 ? v->arch.pv.ldt_ents > : v->arch.pv.gdt_ents)) || > - __get_user(desc, pdesc) ) > + get_unsafe(desc, pdesc) ) > return 0; > > *sel = (desc.a >> 16) & 0x0000fffc; > @@ -59,7 +59,7 @@ static int read_gate_descriptor(unsigned > { > if ( (*ar & 0x1f00) != 0x0c00 || > /* Limit check done above already. */ > - __get_user(desc, pdesc + 1) || > + get_unsafe(desc, pdesc + 1) || > (desc.b & 0x1f00) ) > return 0; > > @@ -294,7 +294,7 @@ void pv_emulate_gate_op(struct cpu_user_ > { \ > --stkp; \ > esp -= 4; \ > - rc = __put_user(item, stkp); \ > + rc = __put_guest(item, stkp); \ > if ( rc ) \ > { \ > pv_inject_page_fault(PFEC_write_access, \ > @@ -362,7 +362,7 @@ void pv_emulate_gate_op(struct cpu_user_ > unsigned int parm; > > --ustkp; > - rc = __get_user(parm, ustkp); > + rc = __get_guest(parm, ustkp); > if ( rc ) > { > pv_inject_page_fault(0, (unsigned long)(ustkp + 1) - rc); > --- a/xen/arch/x86/pv/emulate.c > +++ b/xen/arch/x86/pv/emulate.c > @@ -34,13 +34,13 @@ int pv_emul_read_descriptor(unsigned int > if ( sel < 4 || > /* > * Don't apply the GDT limit here, as the selector may be a Xen > - * provided one. __get_user() will fail (without taking further > + * provided one. get_unsafe() will fail (without taking further > * action) for ones falling in the gap between guest populated > * and Xen ones. > */ > ((sel & 4) && (sel >> 3) >= v->arch.pv.ldt_ents) ) > desc.b = desc.a = 0; > - else if ( __get_user(desc, gdt_ldt_desc_ptr(sel)) ) > + else if ( get_unsafe(desc, gdt_ldt_desc_ptr(sel)) ) > return 0; > if ( !insn_fetch ) > desc.b &= ~_SEGMENT_L; > --- a/xen/arch/x86/pv/iret.c > +++ b/xen/arch/x86/pv/iret.c > @@ -114,15 +114,15 @@ unsigned int compat_iret(void) > regs->rsp = (u32)regs->rsp; > > /* Restore EAX (clobbered by hypercall). */ > - if ( unlikely(__get_user(regs->eax, (u32 *)regs->rsp)) ) > + if ( unlikely(__get_guest(regs->eax, (u32 *)regs->rsp)) ) > { > domain_crash(v->domain); > return 0; > } > > /* Restore CS and EIP. */ > - if ( unlikely(__get_user(regs->eip, (u32 *)regs->rsp + 1)) || > - unlikely(__get_user(regs->cs, (u32 *)regs->rsp + 2)) ) > + if ( unlikely(__get_guest(regs->eip, (u32 *)regs->rsp + 1)) || > + unlikely(__get_guest(regs->cs, (u32 *)regs->rsp + 2)) ) > { > domain_crash(v->domain); > return 0; > @@ -132,7 +132,7 @@ unsigned int compat_iret(void) > * Fix up and restore EFLAGS. We fix up in a local staging area > * to avoid firing the BUG_ON(IOPL) check in arch_get_info_guest. > */ > - if ( unlikely(__get_user(eflags, (u32 *)regs->rsp + 3)) ) > + if ( unlikely(__get_guest(eflags, (u32 *)regs->rsp + 3)) ) > { > domain_crash(v->domain); > return 0; > @@ -164,16 +164,16 @@ unsigned int compat_iret(void) > { > for (i = 1; i < 10; ++i) > { > - rc |= __get_user(x, (u32 *)regs->rsp + i); > - rc |= __put_user(x, (u32 *)(unsigned long)ksp + i); > + rc |= __get_guest(x, (u32 *)regs->rsp + i); > + rc |= __put_guest(x, (u32 *)(unsigned long)ksp + i); > } > } > else if ( ksp > regs->esp ) > { > for ( i = 9; i > 0; --i ) > { > - rc |= __get_user(x, (u32 *)regs->rsp + i); > - rc |= __put_user(x, (u32 *)(unsigned long)ksp + i); > + rc |= __get_guest(x, (u32 *)regs->rsp + i); > + rc |= __put_guest(x, (u32 *)(unsigned long)ksp + i); > } > } > if ( rc ) > @@ -189,7 +189,7 @@ unsigned int compat_iret(void) > eflags &= ~X86_EFLAGS_IF; > regs->eflags &= ~(X86_EFLAGS_VM|X86_EFLAGS_RF| > X86_EFLAGS_NT|X86_EFLAGS_TF); > - if ( unlikely(__put_user(0, (u32 *)regs->rsp)) ) > + if ( unlikely(__put_guest(0, (u32 *)regs->rsp)) ) > { > domain_crash(v->domain); > return 0; > @@ -205,8 +205,8 @@ unsigned int compat_iret(void) > else if ( ring_1(regs) ) > regs->esp += 16; > /* Return to ring 2/3: restore ESP and SS. */ > - else if ( __get_user(regs->ss, (u32 *)regs->rsp + 5) || > - __get_user(regs->esp, (u32 *)regs->rsp + 4) ) > + else if ( __get_guest(regs->ss, (u32 *)regs->rsp + 5) || > + __get_guest(regs->esp, (u32 *)regs->rsp + 4) ) > { > domain_crash(v->domain); > return 0; > --- a/xen/arch/x86/traps.c > +++ b/xen/arch/x86/traps.c > @@ -274,7 +274,7 @@ static void compat_show_guest_stack(stru > { > if ( (((long)stack - 1) ^ ((long)(stack + 1) - 1)) & mask ) > break; > - if ( __get_user(addr, stack) ) > + if ( get_unsafe(addr, stack) ) > { > if ( i != 0 ) > printk("\n "); > @@ -343,7 +343,7 @@ static void show_guest_stack(struct vcpu > { > if ( (((long)stack - 1) ^ ((long)(stack + 1) - 1)) & mask ) > break; > - if ( __get_user(addr, stack) ) > + if ( get_unsafe(addr, stack) ) Shouldn't accessing the guest stack use the _guest accessors? Or has this address been verified by Xen and not in idrect control of the guest, and thus can't be used for speculation purposes? I feel like this should be using the _guest accessors anyway, as the guest stack is an address in guest space? Thanks, Roger.
On 09.02.2021 15:46, Roger Pau Monné wrote: > On Tue, Feb 09, 2021 at 02:15:18PM +0100, Jan Beulich wrote: >> On 09.02.2021 14:07, Roger Pau Monné wrote: >>> On Fri, Feb 05, 2021 at 05:26:33PM +0100, Jan Beulich wrote: >>>> On 05.02.2021 17:18, Roger Pau Monné wrote: >>>>> On Fri, Feb 05, 2021 at 05:13:22PM +0100, Jan Beulich wrote: >>>>>> On 05.02.2021 16:43, Roger Pau Monné wrote: >>>>>>> On Thu, Jan 14, 2021 at 04:04:11PM +0100, Jan Beulich wrote: >>>>>>>> The "guest" variants are intended to work with (potentially) fully guest >>>>>>>> controlled addresses, while the "unsafe" variants are not. >>>>>>> >>>>>>> Just to clarify, both work against user addresses, but guest variants >>>>>>> need to be more careful because the guest provided address can also be >>>>>>> modified? >>>>>>> >>>>>>> I'm trying to understand the difference between "fully guest >>>>>>> controlled" and "guest controlled". >>>>>> >>>>>> Not exactly, not. "unsafe" means access to anything which may >>>>>> fault, guest controlled or not. do_invalid_op()'s reading of >>>>>> the insn stream is a good example - the faulting insn there >>>>>> isn't guest controlled at all, but we still want to be careful >>>>>> when trying to read these bytes, as we don't want to fully >>>>>> trust %rip there. >>> >>> Oh, I see. It's possible that %rip points to an unmapped address >>> there, and we need to be careful when reading, even if the value of >>> %rip cannot be controlled by the guest and can legitimacy point to >>> Xen's address space. >>> >>>>> Would it make sense to threat everything as 'guest' accesses for the >>>>> sake of not having this difference? >>>> >>>> That's what we've been doing until now. It is the purpose of >>>> this change to allow the two to behave differently. >>>> >>>>> I think having two accessors it's likely to cause confusion and could >>>>> possibly lead to the wrong one being used in unexpected contexts. Does >>>>> it add a too big performance penalty to always use the most >>>>> restrictive one? >>>> >>>> The problem is the most restrictive one is going to be too >>>> restrictive - we wouldn't be able to access Xen space anymore >>>> e.g. from the place pointed at above as example. This is >>>> because for guest accesses (but not for "unsafe" ones) we're >>>> going to divert them into non-canonical space (and hence make >>>> speculation impossible, as such an access would fault) if it >>>> would touch Xen space. >>> >>> Yes, I understand now. I think it would have been helpful (for me) to >>> have the first sentence as: >>> >>> The "guest" variants are intended to work with (potentially) fully guest >>> controlled addresses, while the "unsafe" variants are expected to be >>> used in order to access addresses not under the guest control, but >>> that could trigger faults anyway (like accessing the instruction >>> stream in do_invalid_op). >> >> I can use some of this, but in particular "access addresses not >> under the guest control" isn't entirely correct. The question isn't >> whether there's a guest control aspect, but which part of the >> address space the addresses are in. See specifically x86/PV: use >> get_unsafe() instead of copy_from_unsafe()" for two pretty good >> examples. The address within the linear page tables are - in a >> way at least - still somewhat guest controlled, but we're >> deliberately accessing Xen virtual addresses there. > > OK, could this be somehow added to the commit message then? > > Maybe it would be better to have something like: > > The "guest" variants are intended to work with addresses belonging to > the guest address space, while the "unsafe" variants should be used > for addresses that fall into the Xen address space. > > I think it's important to list exactly how the distinction between the > guest/unsafe accessors is made, or else it's impossible to review that > the changes done here are correct. I did change the paragraph to read The "guest" variants are intended to work with (potentially) fully guest controlled addresses, while the "unsafe" variants are intended to be used in order to access addresses not (directly) under guest control, within Xen's part of virtual address space. (For linear page table and descriptor table accesses the low bits of the addresses may still be guest controlled, but this still won't allow speculation to "escape" into unwanted areas.) Subsequently we will want them to have distinct behavior, so as first step identify which one is which. For now, both groups of constructs alias one another. earlier today. Is this clear enough? Relevant parts I've also mirrored into patch 3's description? Jan
On 09.02.2021 15:55, Roger Pau Monné wrote: > On Thu, Jan 14, 2021 at 04:04:11PM +0100, Jan Beulich wrote: >> The "guest" variants are intended to work with (potentially) fully guest >> controlled addresses, while the "unsafe" variants are not. (For >> descriptor table accesses the low bits of the addresses may still be >> guest controlled, but this still won't allow speculation to "escape" >> into unwanted areas.) > > Descriptor table is also in guest address space, and hence would be > fine using the _guest accessors? (even if not in guest control and > thus unsuitable as an speculation vector) No, we don't access descriptor tables in guest space, I don't think. See read_gate_descriptor() for an example. After all PV guests don't even have the full concept of self-managed (in their VA space) descriptor tables (GDT gets specified in terms of frames, while LDT gets specified in terms of (VA,size) tuples, but just for Xen to read the underlying page table entries upon 1st access). >> --- a/xen/arch/x86/traps.c >> +++ b/xen/arch/x86/traps.c >> @@ -274,7 +274,7 @@ static void compat_show_guest_stack(stru >> { >> if ( (((long)stack - 1) ^ ((long)(stack + 1) - 1)) & mask ) >> break; >> - if ( __get_user(addr, stack) ) >> + if ( get_unsafe(addr, stack) ) >> { >> if ( i != 0 ) >> printk("\n "); >> @@ -343,7 +343,7 @@ static void show_guest_stack(struct vcpu >> { >> if ( (((long)stack - 1) ^ ((long)(stack + 1) - 1)) & mask ) >> break; >> - if ( __get_user(addr, stack) ) >> + if ( get_unsafe(addr, stack) ) > > Shouldn't accessing the guest stack use the _guest accessors? Hmm, yes indeed. > Or has this address been verified by Xen and not in idrect control of > the guest, and thus can't be used for speculation purposes? > > I feel like this should be using the _guest accessors anyway, as the > guest stack is an address in guest space? I think this being a debugging function only, not directly accessible by guests, is what made me think speculation is not an issue here and hence the "unsafe" variants are fine to use (they're slightly cheaper after all, once the subsequent changes are in place). But I guess I will better switch these two around. Jan
On Tue, Feb 09, 2021 at 03:57:22PM +0100, Jan Beulich wrote: > I did change the paragraph to read > > The "guest" variants are intended to work with (potentially) fully guest > controlled addresses, while the "unsafe" variants are intended to be > used in order to access addresses not (directly) under guest control, > within Xen's part of virtual address space. (For linear page table and > descriptor table accesses the low bits of the addresses may still be > guest controlled, but this still won't allow speculation to "escape" > into unwanted areas.) Subsequently we will want them to have distinct > behavior, so as first step identify which one is which. For now, both > groups of constructs alias one another. > > earlier today. Is this clear enough? Relevant parts I've also mirrored > into patch 3's description? Yes, that does indeed seem clearer, thanks. Roger.
On Tue, Feb 09, 2021 at 04:14:41PM +0100, Jan Beulich wrote: > On 09.02.2021 15:55, Roger Pau Monné wrote: > > On Thu, Jan 14, 2021 at 04:04:11PM +0100, Jan Beulich wrote: > >> The "guest" variants are intended to work with (potentially) fully guest > >> controlled addresses, while the "unsafe" variants are not. (For > >> descriptor table accesses the low bits of the addresses may still be > >> guest controlled, but this still won't allow speculation to "escape" > >> into unwanted areas.) > > > > Descriptor table is also in guest address space, and hence would be > > fine using the _guest accessors? (even if not in guest control and > > thus unsuitable as an speculation vector) > > No, we don't access descriptor tables in guest space, I don't > think. See read_gate_descriptor() for an example. After all PV > guests don't even have the full concept of self-managed (in > their VA space) descriptor tables (GDT gets specified in terms > of frames, while LDT gets specified in terms of (VA,size) > tuples, but just for Xen to read the underlying page table > entries upon 1st access). I see, read_gate_descriptor uses gdt_ldt_desc_ptr which points into the per-domain Xen virt space, so it's indeed an address in Xen address space. I realize it doesn't make sense for the descriptor table to be in guest address space, as it's not fully under guest control. > >> --- a/xen/arch/x86/traps.c > >> +++ b/xen/arch/x86/traps.c > >> @@ -274,7 +274,7 @@ static void compat_show_guest_stack(stru > >> { > >> if ( (((long)stack - 1) ^ ((long)(stack + 1) - 1)) & mask ) > >> break; > >> - if ( __get_user(addr, stack) ) > >> + if ( get_unsafe(addr, stack) ) > >> { > >> if ( i != 0 ) > >> printk("\n "); > >> @@ -343,7 +343,7 @@ static void show_guest_stack(struct vcpu > >> { > >> if ( (((long)stack - 1) ^ ((long)(stack + 1) - 1)) & mask ) > >> break; > >> - if ( __get_user(addr, stack) ) > >> + if ( get_unsafe(addr, stack) ) > > > > Shouldn't accessing the guest stack use the _guest accessors? > > Hmm, yes indeed. > > > Or has this address been verified by Xen and not in idrect control of > > the guest, and thus can't be used for speculation purposes? > > > > I feel like this should be using the _guest accessors anyway, as the > > guest stack is an address in guest space? > > I think this being a debugging function only, not directly > accessible by guests, is what made me think speculation is > not an issue here and hence the "unsafe" variants are fine > to use (they're slightly cheaper after all, once the > subsequent changes are in place). But I guess I will better > switch these two around. With that: Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger.
--- a/xen/arch/x86/mm/shadow/multi.c +++ b/xen/arch/x86/mm/shadow/multi.c @@ -776,9 +776,9 @@ shadow_write_entries(void *d, void *s, i /* Because we mirror access rights at all levels in the shadow, an * l2 (or higher) entry with the RW bit cleared will leave us with * no write access through the linear map. - * We detect that by writing to the shadow with __put_user() and + * We detect that by writing to the shadow with put_unsafe() and * using map_domain_page() to get a writeable mapping if we need to. */ - if ( __put_user(*dst, dst) ) + if ( put_unsafe(*dst, dst) ) { perfc_incr(shadow_linear_map_failed); map = map_domain_page(mfn); --- a/xen/arch/x86/pv/emul-gate-op.c +++ b/xen/arch/x86/pv/emul-gate-op.c @@ -40,7 +40,7 @@ static int read_gate_descriptor(unsigned ((gate_sel >> 3) + !is_pv_32bit_vcpu(v) >= (gate_sel & 4 ? v->arch.pv.ldt_ents : v->arch.pv.gdt_ents)) || - __get_user(desc, pdesc) ) + get_unsafe(desc, pdesc) ) return 0; *sel = (desc.a >> 16) & 0x0000fffc; @@ -59,7 +59,7 @@ static int read_gate_descriptor(unsigned { if ( (*ar & 0x1f00) != 0x0c00 || /* Limit check done above already. */ - __get_user(desc, pdesc + 1) || + get_unsafe(desc, pdesc + 1) || (desc.b & 0x1f00) ) return 0; @@ -294,7 +294,7 @@ void pv_emulate_gate_op(struct cpu_user_ { \ --stkp; \ esp -= 4; \ - rc = __put_user(item, stkp); \ + rc = __put_guest(item, stkp); \ if ( rc ) \ { \ pv_inject_page_fault(PFEC_write_access, \ @@ -362,7 +362,7 @@ void pv_emulate_gate_op(struct cpu_user_ unsigned int parm; --ustkp; - rc = __get_user(parm, ustkp); + rc = __get_guest(parm, ustkp); if ( rc ) { pv_inject_page_fault(0, (unsigned long)(ustkp + 1) - rc); --- a/xen/arch/x86/pv/emulate.c +++ b/xen/arch/x86/pv/emulate.c @@ -34,13 +34,13 @@ int pv_emul_read_descriptor(unsigned int if ( sel < 4 || /* * Don't apply the GDT limit here, as the selector may be a Xen - * provided one. __get_user() will fail (without taking further + * provided one. get_unsafe() will fail (without taking further * action) for ones falling in the gap between guest populated * and Xen ones. */ ((sel & 4) && (sel >> 3) >= v->arch.pv.ldt_ents) ) desc.b = desc.a = 0; - else if ( __get_user(desc, gdt_ldt_desc_ptr(sel)) ) + else if ( get_unsafe(desc, gdt_ldt_desc_ptr(sel)) ) return 0; if ( !insn_fetch ) desc.b &= ~_SEGMENT_L; --- a/xen/arch/x86/pv/iret.c +++ b/xen/arch/x86/pv/iret.c @@ -114,15 +114,15 @@ unsigned int compat_iret(void) regs->rsp = (u32)regs->rsp; /* Restore EAX (clobbered by hypercall). */ - if ( unlikely(__get_user(regs->eax, (u32 *)regs->rsp)) ) + if ( unlikely(__get_guest(regs->eax, (u32 *)regs->rsp)) ) { domain_crash(v->domain); return 0; } /* Restore CS and EIP. */ - if ( unlikely(__get_user(regs->eip, (u32 *)regs->rsp + 1)) || - unlikely(__get_user(regs->cs, (u32 *)regs->rsp + 2)) ) + if ( unlikely(__get_guest(regs->eip, (u32 *)regs->rsp + 1)) || + unlikely(__get_guest(regs->cs, (u32 *)regs->rsp + 2)) ) { domain_crash(v->domain); return 0; @@ -132,7 +132,7 @@ unsigned int compat_iret(void) * Fix up and restore EFLAGS. We fix up in a local staging area * to avoid firing the BUG_ON(IOPL) check in arch_get_info_guest. */ - if ( unlikely(__get_user(eflags, (u32 *)regs->rsp + 3)) ) + if ( unlikely(__get_guest(eflags, (u32 *)regs->rsp + 3)) ) { domain_crash(v->domain); return 0; @@ -164,16 +164,16 @@ unsigned int compat_iret(void) { for (i = 1; i < 10; ++i) { - rc |= __get_user(x, (u32 *)regs->rsp + i); - rc |= __put_user(x, (u32 *)(unsigned long)ksp + i); + rc |= __get_guest(x, (u32 *)regs->rsp + i); + rc |= __put_guest(x, (u32 *)(unsigned long)ksp + i); } } else if ( ksp > regs->esp ) { for ( i = 9; i > 0; --i ) { - rc |= __get_user(x, (u32 *)regs->rsp + i); - rc |= __put_user(x, (u32 *)(unsigned long)ksp + i); + rc |= __get_guest(x, (u32 *)regs->rsp + i); + rc |= __put_guest(x, (u32 *)(unsigned long)ksp + i); } } if ( rc ) @@ -189,7 +189,7 @@ unsigned int compat_iret(void) eflags &= ~X86_EFLAGS_IF; regs->eflags &= ~(X86_EFLAGS_VM|X86_EFLAGS_RF| X86_EFLAGS_NT|X86_EFLAGS_TF); - if ( unlikely(__put_user(0, (u32 *)regs->rsp)) ) + if ( unlikely(__put_guest(0, (u32 *)regs->rsp)) ) { domain_crash(v->domain); return 0; @@ -205,8 +205,8 @@ unsigned int compat_iret(void) else if ( ring_1(regs) ) regs->esp += 16; /* Return to ring 2/3: restore ESP and SS. */ - else if ( __get_user(regs->ss, (u32 *)regs->rsp + 5) || - __get_user(regs->esp, (u32 *)regs->rsp + 4) ) + else if ( __get_guest(regs->ss, (u32 *)regs->rsp + 5) || + __get_guest(regs->esp, (u32 *)regs->rsp + 4) ) { domain_crash(v->domain); return 0; --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -274,7 +274,7 @@ static void compat_show_guest_stack(stru { if ( (((long)stack - 1) ^ ((long)(stack + 1) - 1)) & mask ) break; - if ( __get_user(addr, stack) ) + if ( get_unsafe(addr, stack) ) { if ( i != 0 ) printk("\n "); @@ -343,7 +343,7 @@ static void show_guest_stack(struct vcpu { if ( (((long)stack - 1) ^ ((long)(stack + 1) - 1)) & mask ) break; - if ( __get_user(addr, stack) ) + if ( get_unsafe(addr, stack) ) { if ( i != 0 ) printk("\n "); --- a/xen/include/asm-x86/uaccess.h +++ b/xen/include/asm-x86/uaccess.h @@ -60,13 +60,11 @@ extern void __put_user_bad(void); __put_user_check((__typeof__(*(ptr)))(x),(ptr),sizeof(*(ptr))) /** - * __get_user: - Get a simple variable from user space, with less checking. + * __get_guest: - Get a simple variable from guest space, with less checking. * @x: Variable to store result. - * @ptr: Source address, in user space. + * @ptr: Source address, in guest space. * - * Context: User context only. This function may sleep. - * - * This macro copies a single simple variable from user space to kernel + * This macro copies a single simple variable from guest space to hypervisor * space. It supports simple types like char and int, but not larger * data types like structures or arrays. * @@ -79,17 +77,15 @@ extern void __put_user_bad(void); * Returns zero on success, or -EFAULT on error. * On error, the variable @x is set to zero. */ -#define __get_user(x,ptr) \ - __get_user_nocheck((x),(ptr),sizeof(*(ptr))) +#define __get_guest(x, ptr) get_guest_nocheck(x, ptr, sizeof(*(ptr))) +#define get_unsafe __get_guest /** - * __put_user: - Write a simple value into user space, with less checking. - * @x: Value to copy to user space. - * @ptr: Destination address, in user space. + * __put_guest: - Write a simple value into guest space, with less checking. + * @x: Value to store in guest space. + * @ptr: Destination address, in guest space. * - * Context: User context only. This function may sleep. - * - * This macro copies a single simple value from kernel space to user + * This macro copies a single simple value from hypervisor space to guest * space. It supports simple types like char and int, but not larger * data types like structures or arrays. * @@ -101,13 +97,14 @@ extern void __put_user_bad(void); * * Returns zero on success, or -EFAULT on error. */ -#define __put_user(x,ptr) \ - __put_user_nocheck((__typeof__(*(ptr)))(x),(ptr),sizeof(*(ptr))) +#define __put_guest(x, ptr) \ + put_guest_nocheck((__typeof__(*(ptr)))(x), ptr, sizeof(*(ptr))) +#define put_unsafe __put_guest -#define __put_user_nocheck(x, ptr, size) \ +#define put_guest_nocheck(x, ptr, size) \ ({ \ int err_; \ - __put_user_size(x, ptr, size, err_, -EFAULT); \ + put_guest_size(x, ptr, size, err_, -EFAULT); \ err_; \ }) @@ -115,14 +112,14 @@ extern void __put_user_bad(void); ({ \ __typeof__(*(ptr)) __user *ptr_ = (ptr); \ __typeof__(size) size_ = (size); \ - access_ok(ptr_, size_) ? __put_user_nocheck(x, ptr_, size_) \ + access_ok(ptr_, size_) ? put_guest_nocheck(x, ptr_, size_) \ : -EFAULT; \ }) -#define __get_user_nocheck(x, ptr, size) \ +#define get_guest_nocheck(x, ptr, size) \ ({ \ int err_; \ - __get_user_size(x, ptr, size, err_, -EFAULT); \ + get_guest_size(x, ptr, size, err_, -EFAULT); \ err_; \ }) @@ -130,7 +127,7 @@ extern void __put_user_bad(void); ({ \ __typeof__(*(ptr)) __user *ptr_ = (ptr); \ __typeof__(size) size_ = (size); \ - access_ok(ptr_, size_) ? __get_user_nocheck(x, ptr_, size_) \ + access_ok(ptr_, size_) ? get_guest_nocheck(x, ptr_, size_) \ : -EFAULT; \ }) @@ -142,7 +139,7 @@ struct __large_struct { unsigned long bu * we do not write to any memory gcc knows about, so there are no * aliasing issues. */ -#define __put_user_asm(x, addr, err, itype, rtype, ltype, errret) \ +#define put_unsafe_asm(x, addr, err, itype, rtype, ltype, errret) \ stac(); \ __asm__ __volatile__( \ "1: mov"itype" %"rtype"1,%2\n" \ @@ -156,7 +153,7 @@ struct __large_struct { unsigned long bu : ltype (x), "m"(__m(addr)), "i"(errret), "0"(err)); \ clac() -#define __get_user_asm(x, addr, err, itype, rtype, ltype, errret) \ +#define get_unsafe_asm(x, addr, err, itype, rtype, ltype, errret) \ stac(); \ __asm__ __volatile__( \ "1: mov"itype" %2,%"rtype"1\n" \ @@ -171,6 +168,34 @@ struct __large_struct { unsigned long bu : "m"(__m(addr)), "i"(errret), "0"(err)); \ clac() +#define put_unsafe_size(x, ptr, size, retval, errret) \ +do { \ + retval = 0; \ + switch ( size ) \ + { \ + case 1: put_unsafe_asm(x, ptr, retval, "b", "b", "iq", errret); break; \ + case 2: put_unsafe_asm(x, ptr, retval, "w", "w", "ir", errret); break; \ + case 4: put_unsafe_asm(x, ptr, retval, "l", "k", "ir", errret); break; \ + case 8: put_unsafe_asm(x, ptr, retval, "q", "", "ir", errret); break; \ + default: __put_user_bad(); \ + } \ +} while ( false ) +#define put_guest_size put_unsafe_size + +#define get_unsafe_size(x, ptr, size, retval, errret) \ +do { \ + retval = 0; \ + switch ( size ) \ + { \ + case 1: get_unsafe_asm(x, ptr, retval, "b", "b", "=q", errret); break; \ + case 2: get_unsafe_asm(x, ptr, retval, "w", "w", "=r", errret); break; \ + case 4: get_unsafe_asm(x, ptr, retval, "l", "k", "=r", errret); break; \ + case 8: get_unsafe_asm(x, ptr, retval, "q", "", "=r", errret); break; \ + default: __get_user_bad(); \ + } \ +} while ( false ) +#define get_guest_size get_unsafe_size + /** * __copy_to_user: - Copy a block of data into user space, with less checking * @to: Destination address, in user space. @@ -193,16 +218,16 @@ __copy_to_user(void __user *to, const vo switch (n) { case 1: - __put_user_size(*(const u8 *)from, (u8 __user *)to, 1, ret, 1); + put_guest_size(*(const uint8_t *)from, to, 1, ret, 1); return ret; case 2: - __put_user_size(*(const u16 *)from, (u16 __user *)to, 2, ret, 2); + put_guest_size(*(const uint16_t *)from, to, 2, ret, 2); return ret; case 4: - __put_user_size(*(const u32 *)from, (u32 __user *)to, 4, ret, 4); + put_guest_size(*(const uint32_t *)from, to, 4, ret, 4); return ret; case 8: - __put_user_size(*(const u64 *)from, (u64 __user *)to, 8, ret, 8); + put_guest_size(*(const uint64_t *)from, to, 8, ret, 8); return ret; } } @@ -234,16 +259,16 @@ __copy_from_user(void *to, const void __ switch (n) { case 1: - __get_user_size(*(u8 *)to, from, 1, ret, 1); + get_guest_size(*(uint8_t *)to, from, 1, ret, 1); return ret; case 2: - __get_user_size(*(u16 *)to, from, 2, ret, 2); + get_guest_size(*(uint16_t *)to, from, 2, ret, 2); return ret; case 4: - __get_user_size(*(u32 *)to, from, 4, ret, 4); + get_guest_size(*(uint32_t *)to, from, 4, ret, 4); return ret; case 8: - __get_user_size(*(u64*)to, from, 8, ret, 8); + get_guest_size(*(uint64_t *)to, from, 8, ret, 8); return ret; } } --- a/xen/include/asm-x86/x86_64/uaccess.h +++ b/xen/include/asm-x86/x86_64/uaccess.h @@ -57,28 +57,4 @@ extern void *xlat_malloc(unsigned long * (likely((count) < (~0U / (size))) && \ compat_access_ok(addr, 0 + (count) * (size))) -#define __put_user_size(x,ptr,size,retval,errret) \ -do { \ - retval = 0; \ - switch (size) { \ - case 1: __put_user_asm(x,ptr,retval,"b","b","iq",errret);break; \ - case 2: __put_user_asm(x,ptr,retval,"w","w","ir",errret);break; \ - case 4: __put_user_asm(x,ptr,retval,"l","k","ir",errret);break; \ - case 8: __put_user_asm(x,ptr,retval,"q","","ir",errret);break; \ - default: __put_user_bad(); \ - } \ -} while (0) - -#define __get_user_size(x,ptr,size,retval,errret) \ -do { \ - retval = 0; \ - switch (size) { \ - case 1: __get_user_asm(x,ptr,retval,"b","b","=q",errret);break; \ - case 2: __get_user_asm(x,ptr,retval,"w","w","=r",errret);break; \ - case 4: __get_user_asm(x,ptr,retval,"l","k","=r",errret);break; \ - case 8: __get_user_asm(x,ptr,retval,"q","","=r",errret); break; \ - default: __get_user_bad(); \ - } \ -} while (0) - #endif /* __X86_64_UACCESS_H */ --- a/xen/test/livepatch/xen_hello_world_func.c +++ b/xen/test/livepatch/xen_hello_world_func.c @@ -26,7 +26,7 @@ const char *xen_hello_world(void) * Any BUG, or WARN_ON will contain symbol and payload name. Furthermore * exceptions will be caught and processed properly. */ - rc = __get_user(tmp, non_canonical_addr); + rc = get_unsafe(tmp, non_canonical_addr); BUG_ON(rc != -EFAULT); #endif #if defined(CONFIG_ARM)
The "guest" variants are intended to work with (potentially) fully guest controlled addresses, while the "unsafe" variants are not. (For descriptor table accesses the low bits of the addresses may still be guest controlled, but this still won't allow speculation to "escape" into unwanted areas.) Subsequently we will want them to have different behavior, so as first step identify which one is which. For now, both groups of constructs alias one another. Double underscore prefixes are retained only on __{get,put}_guest(), to allow still distinguishing them from their "checking" counterparts once they also get renamed (to {get,put}_guest()). Since for them it's almost a full re-write, move what becomes {get,put}_unsafe_size() into the "common" uaccess.h (x86_64/*.h should disappear at some point anyway). In __copy_to_user() one of the two casts in each put_guest_size() invocation gets dropped. They're not needed and did break symmetry with __copy_from_user(). Signed-off-by: Jan Beulich <jbeulich@suse.com>