| Message ID | 4250bba0-1719-60ae-3d1f-350fb5d2021d@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] x86/stackframe/32: repair 32-bit Xen PV | expand |
On Tue, 29 Oct 2019, Jan Beulich wrote: > Once again RPL checks have been introduced which don't account for a > 32-bit kernel living in ring 1 when running in a PV Xen domain. > > The case in FIXUP_FRAME has been preventing boot; adjust BUG_IF_WRONG_CR3 > as well just in case. Either it's required and correct or it's not. Just in case is not helpful at all. > Fixes: 3c88c692c287 ("x86/stackframe/32: Provide consistent pt_regs") > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- a/arch/x86/entry/entry_32.S > +++ b/arch/x86/entry/entry_32.S > @@ -48,6 +48,13 @@ > > #include "calling.h" > > +/* > + * When running on Xen PV, the actual %cs register's RPL in the kernel is 1, > + * not 0. If we need to distinguish between a %cs from kernel mode and a %cs > + * from user mode, we can do test $2 instead of test $3. > + */ > +#define USER_SEGMENT_RPL_MASK 2 No. The define want's to be right next to the SEGMENT_RPL_MASK define including a less ASM focussed comment like this: /* * When running on Xen PV, the actual priviledge level of the kernel is 1, * not 0. Testing the Requested Priviledge Level in a segment selector to * determine whether the context is user mode or kernel mode with * SEGMENT_RPL_MASK is wrong because the PV kernels priviledge level * matches the 0x03 mask. * * Testing with USER_SEGMENT_RPL_MASK is valid for both native and Xen PV * kernels because Priviledge Level 2 is never used. */ Hmm? Thanks, tglx
On 04.11.2019 23:44, Thomas Gleixner wrote: > On Tue, 29 Oct 2019, Jan Beulich wrote: > >> Once again RPL checks have been introduced which don't account for a >> 32-bit kernel living in ring 1 when running in a PV Xen domain. >> >> The case in FIXUP_FRAME has been preventing boot; adjust BUG_IF_WRONG_CR3 >> as well just in case. > > Either it's required and correct or it's not. Just in case is not helpful > at all. _If_ this macro sits on any path reachable when running PV under Xen, then it's wrong. If any such use gets added down the road, then it's latently wrong, which is bad enough imo, and hence warrants the change even without analyzing whether there's already an affected path. >> Fixes: 3c88c692c287 ("x86/stackframe/32: Provide consistent pt_regs") >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >> --- a/arch/x86/entry/entry_32.S >> +++ b/arch/x86/entry/entry_32.S >> @@ -48,6 +48,13 @@ >> >> #include "calling.h" >> >> +/* >> + * When running on Xen PV, the actual %cs register's RPL in the kernel is 1, >> + * not 0. If we need to distinguish between a %cs from kernel mode and a %cs >> + * from user mode, we can do test $2 instead of test $3. >> + */ >> +#define USER_SEGMENT_RPL_MASK 2 > > No. The define want's to be right next to the SEGMENT_RPL_MASK define > including a less ASM focussed comment like this: > > /* > * When running on Xen PV, the actual priviledge level of the kernel is 1, > * not 0. Testing the Requested Priviledge Level in a segment selector to > * determine whether the context is user mode or kernel mode with > * SEGMENT_RPL_MASK is wrong because the PV kernels priviledge level > * matches the 0x03 mask. > * > * Testing with USER_SEGMENT_RPL_MASK is valid for both native and Xen PV > * kernels because Priviledge Level 2 is never used. > */ > > Hmm? I simply used almost exactly what Andy had suggested as a comment. He also didn't object to the definition sitting here (it's not needed after all outside of this file). Can the two of you please reach agreement, for me to act upon? Jan
Andy, On 29.10.2019 10:28, Jan Beulich wrote: > Once again RPL checks have been introduced which don't account for a > 32-bit kernel living in ring 1 when running in a PV Xen domain. The > case in FIXUP_FRAME has been preventing boot; adjust BUG_IF_WRONG_CR3 > as well just in case. > > Fixes: 3c88c692c287 ("x86/stackframe/32: Provide consistent pt_regs") > Signed-off-by: Jan Beulich <jbeulich@suse.com> would you mind clarifying whether I should follow Thomas' request, overriding what you had asked for an I did carry out for v2? I don't think this regression should be left unfixed for much longer (as much as the other part of it, addressed by a later 2-patch series). Thanks, Jan > --- > v2: Avoid #ifdef and alter comment along the lines of Andy's suggestion. > > --- a/arch/x86/entry/entry_32.S > +++ b/arch/x86/entry/entry_32.S > @@ -48,6 +48,13 @@ > > #include "calling.h" > > +/* > + * When running on Xen PV, the actual %cs register's RPL in the kernel is 1, > + * not 0. If we need to distinguish between a %cs from kernel mode and a %cs > + * from user mode, we can do test $2 instead of test $3. > + */ > +#define USER_SEGMENT_RPL_MASK 2 > + > .section .entry.text, "ax" > > /* > @@ -172,7 +179,7 @@ > ALTERNATIVE "jmp .Lend_\@", "", X86_FEATURE_PTI > .if \no_user_check == 0 > /* coming from usermode? */ > - testl $SEGMENT_RPL_MASK, PT_CS(%esp) > + testl $USER_SEGMENT_RPL_MASK, PT_CS(%esp) > jz .Lend_\@ > .endif > /* On user-cr3? */ > @@ -217,7 +224,7 @@ > testl $X86_EFLAGS_VM, 4*4(%esp) > jnz .Lfrom_usermode_no_fixup_\@ > #endif > - testl $SEGMENT_RPL_MASK, 3*4(%esp) > + testl $USER_SEGMENT_RPL_MASK, 3*4(%esp) > jnz .Lfrom_usermode_no_fixup_\@ > > orl $CS_FROM_KERNEL, 3*4(%esp) >
On Fri, Nov 15, 2019 at 6:30 AM Jan Beulich <jbeulich@suse.com> wrote: > > Andy, > > On 29.10.2019 10:28, Jan Beulich wrote: > > Once again RPL checks have been introduced which don't account for a > > 32-bit kernel living in ring 1 when running in a PV Xen domain. The > > case in FIXUP_FRAME has been preventing boot; adjust BUG_IF_WRONG_CR3 > > as well just in case. > > > > Fixes: 3c88c692c287 ("x86/stackframe/32: Provide consistent pt_regs") > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > would you mind clarifying whether I should follow Thomas' request, > overriding what you had asked for an I did carry out for v2? I don't > think this regression should be left unfixed for much longer (as > much as the other part of it, addressed by a later 2-patch series). > I'm fine with doing it Thomas' way. --Andy
--- a/arch/x86/entry/entry_32.S +++ b/arch/x86/entry/entry_32.S @@ -48,6 +48,13 @@ #include "calling.h" +/* + * When running on Xen PV, the actual %cs register's RPL in the kernel is 1, + * not 0. If we need to distinguish between a %cs from kernel mode and a %cs + * from user mode, we can do test $2 instead of test $3. + */ +#define USER_SEGMENT_RPL_MASK 2 + .section .entry.text, "ax" /* @@ -172,7 +179,7 @@ ALTERNATIVE "jmp .Lend_\@", "", X86_FEATURE_PTI .if \no_user_check == 0 /* coming from usermode? */ - testl $SEGMENT_RPL_MASK, PT_CS(%esp) + testl $USER_SEGMENT_RPL_MASK, PT_CS(%esp) jz .Lend_\@ .endif /* On user-cr3? */ @@ -217,7 +224,7 @@ testl $X86_EFLAGS_VM, 4*4(%esp) jnz .Lfrom_usermode_no_fixup_\@ #endif - testl $SEGMENT_RPL_MASK, 3*4(%esp) + testl $USER_SEGMENT_RPL_MASK, 3*4(%esp) jnz .Lfrom_usermode_no_fixup_\@ orl $CS_FROM_KERNEL, 3*4(%esp)
Once again RPL checks have been introduced which don't account for a 32-bit kernel living in ring 1 when running in a PV Xen domain. The case in FIXUP_FRAME has been preventing boot; adjust BUG_IF_WRONG_CR3 as well just in case. Fixes: 3c88c692c287 ("x86/stackframe/32: Provide consistent pt_regs") Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: Avoid #ifdef and alter comment along the lines of Andy's suggestion.