Re: [PATCH 01/13] x86/entry: Fix AC assertion
On Wed, Sep 2, 2020 at 12:31 PM wrote:
>
> On Wed, Sep 02, 2020 at 06:24:27PM +0200, Jürgen Groß wrote:
> > On 02.09.20 17:58, Brian Gerst wrote:
> > > On Wed, Sep 2, 2020 at 9:38 AM Peter Zijlstra
> > > wrote:
> > > >
> > > > From: Peter Zijlstra
> > > >
> > > > The WARN added in commit 3c73b81a9164 ("x86/entry, selftests: Further
> > > > improve user entry sanity checks") unconditionally triggers on my IVB
> > > > machine because it does not support SMAP.
> > > >
> > > > For !SMAP hardware we patch out CLAC/STAC instructions and thus if
> > > > userspace sets AC, we'll still have it set after entry.
> > > >
> > > > Fixes: 3c73b81a9164 ("x86/entry, selftests: Further improve user entry
> > > > sanity checks")
> > > > Signed-off-by: Peter Zijlstra (Intel)
> > > > Acked-by: Andy Lutomirski
> > > > ---
> > > > arch/x86/include/asm/entry-common.h | 11 +--
> > > > 1 file changed, 9 insertions(+), 2 deletions(-)
> > > >
> > > > --- a/arch/x86/include/asm/entry-common.h
> > > > +++ b/arch/x86/include/asm/entry-common.h
> > > > @@ -18,8 +18,16 @@ static __always_inline void arch_check_u
> > > > * state, not the interrupt state as imagined by Xen.
> > > > */
> > > > unsigned long flags = native_save_fl();
> > > > - WARN_ON_ONCE(flags & (X86_EFLAGS_AC | X86_EFLAGS_DF |
> > > > - X86_EFLAGS_NT));
> > > > + unsigned long mask = X86_EFLAGS_DF | X86_EFLAGS_NT;
> > > > +
> > > > + /*
> > > > +* For !SMAP hardware we patch out CLAC on entry.
> > > > +*/
> > > > + if (boot_cpu_has(X86_FEATURE_SMAP) ||
> > > > + (IS_ENABLED(CONFIG_64_BIT) &&
> > > > boot_cpu_has(X86_FEATURE_XENPV)))
> > > > + mask |= X86_EFLAGS_AC;
> > >
> > > Is the explicit Xen check necessary? IIRC the Xen hypervisor will
> > > filter out the SMAP bit in the cpuid pvop.
> >
> > Right, and this test will nevertheless result in setting AC in the mask.
> > IIRC this was the main objective here.
>
> Correct, this asserts that 64bit Xen-PV will never have AC set; it had
> better not have it set since it runs in ring 3.
Ok. That should be added to the comment to avoid confusion.
--
Brian Gerst
Re: [PATCH 01/13] x86/entry: Fix AC assertion
On Wed, Sep 02, 2020 at 06:24:27PM +0200, Jürgen Groß wrote:
> On 02.09.20 17:58, Brian Gerst wrote:
> > On Wed, Sep 2, 2020 at 9:38 AM Peter Zijlstra wrote:
> > >
> > > From: Peter Zijlstra
> > >
> > > The WARN added in commit 3c73b81a9164 ("x86/entry, selftests: Further
> > > improve user entry sanity checks") unconditionally triggers on my IVB
> > > machine because it does not support SMAP.
> > >
> > > For !SMAP hardware we patch out CLAC/STAC instructions and thus if
> > > userspace sets AC, we'll still have it set after entry.
> > >
> > > Fixes: 3c73b81a9164 ("x86/entry, selftests: Further improve user entry
> > > sanity checks")
> > > Signed-off-by: Peter Zijlstra (Intel)
> > > Acked-by: Andy Lutomirski
> > > ---
> > > arch/x86/include/asm/entry-common.h | 11 +--
> > > 1 file changed, 9 insertions(+), 2 deletions(-)
> > >
> > > --- a/arch/x86/include/asm/entry-common.h
> > > +++ b/arch/x86/include/asm/entry-common.h
> > > @@ -18,8 +18,16 @@ static __always_inline void arch_check_u
> > > * state, not the interrupt state as imagined by Xen.
> > > */
> > > unsigned long flags = native_save_fl();
> > > - WARN_ON_ONCE(flags & (X86_EFLAGS_AC | X86_EFLAGS_DF |
> > > - X86_EFLAGS_NT));
> > > + unsigned long mask = X86_EFLAGS_DF | X86_EFLAGS_NT;
> > > +
> > > + /*
> > > +* For !SMAP hardware we patch out CLAC on entry.
> > > +*/
> > > + if (boot_cpu_has(X86_FEATURE_SMAP) ||
> > > + (IS_ENABLED(CONFIG_64_BIT) &&
> > > boot_cpu_has(X86_FEATURE_XENPV)))
> > > + mask |= X86_EFLAGS_AC;
> >
> > Is the explicit Xen check necessary? IIRC the Xen hypervisor will
> > filter out the SMAP bit in the cpuid pvop.
>
> Right, and this test will nevertheless result in setting AC in the mask.
> IIRC this was the main objective here.
Correct, this asserts that 64bit Xen-PV will never have AC set; it had
better not have it set since it runs in ring 3.
Re: [PATCH 01/13] x86/entry: Fix AC assertion
On 02/09/2020 16:58, Brian Gerst wrote:
> On Wed, Sep 2, 2020 at 9:38 AM Peter Zijlstra wrote:
>> From: Peter Zijlstra
>>
>> The WARN added in commit 3c73b81a9164 ("x86/entry, selftests: Further
>> improve user entry sanity checks") unconditionally triggers on my IVB
>> machine because it does not support SMAP.
>>
>> For !SMAP hardware we patch out CLAC/STAC instructions and thus if
>> userspace sets AC, we'll still have it set after entry.
>>
>> Fixes: 3c73b81a9164 ("x86/entry, selftests: Further improve user entry
>> sanity checks")
>> Signed-off-by: Peter Zijlstra (Intel)
>> Acked-by: Andy Lutomirski
>> ---
>> arch/x86/include/asm/entry-common.h | 11 +--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> --- a/arch/x86/include/asm/entry-common.h
>> +++ b/arch/x86/include/asm/entry-common.h
>> @@ -18,8 +18,16 @@ static __always_inline void arch_check_u
>> * state, not the interrupt state as imagined by Xen.
>> */
>> unsigned long flags = native_save_fl();
>> - WARN_ON_ONCE(flags & (X86_EFLAGS_AC | X86_EFLAGS_DF |
>> - X86_EFLAGS_NT));
>> + unsigned long mask = X86_EFLAGS_DF | X86_EFLAGS_NT;
>> +
>> + /*
>> +* For !SMAP hardware we patch out CLAC on entry.
>> +*/
>> + if (boot_cpu_has(X86_FEATURE_SMAP) ||
>> + (IS_ENABLED(CONFIG_64_BIT) &&
>> boot_cpu_has(X86_FEATURE_XENPV)))
>> + mask |= X86_EFLAGS_AC;
> Is the explicit Xen check necessary? IIRC the Xen hypervisor will
> filter out the SMAP bit in the cpuid pvop.
The Xen check isn't anything to do with SMAP.
64bit PV guest kernels run in Ring3, so userspace's choice of AC for
real alignment check purposes needs to not leak into kernel context.
Xen's ABI for a user => kernel context switch should clear AC on behalf
of the kernel, but the fact still remains that if AC actually leaks into
context for whatever reason, stuff is going to break.
~Andrew
Re: [PATCH 01/13] x86/entry: Fix AC assertion
On 02.09.20 17:58, Brian Gerst wrote:
On Wed, Sep 2, 2020 at 9:38 AM Peter Zijlstra wrote:
From: Peter Zijlstra
The WARN added in commit 3c73b81a9164 ("x86/entry, selftests: Further
improve user entry sanity checks") unconditionally triggers on my IVB
machine because it does not support SMAP.
For !SMAP hardware we patch out CLAC/STAC instructions and thus if
userspace sets AC, we'll still have it set after entry.
Fixes: 3c73b81a9164 ("x86/entry, selftests: Further improve user entry sanity
checks")
Signed-off-by: Peter Zijlstra (Intel)
Acked-by: Andy Lutomirski
---
arch/x86/include/asm/entry-common.h | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
--- a/arch/x86/include/asm/entry-common.h
+++ b/arch/x86/include/asm/entry-common.h
@@ -18,8 +18,16 @@ static __always_inline void arch_check_u
* state, not the interrupt state as imagined by Xen.
*/
unsigned long flags = native_save_fl();
- WARN_ON_ONCE(flags & (X86_EFLAGS_AC | X86_EFLAGS_DF |
- X86_EFLAGS_NT));
+ unsigned long mask = X86_EFLAGS_DF | X86_EFLAGS_NT;
+
+ /*
+* For !SMAP hardware we patch out CLAC on entry.
+*/
+ if (boot_cpu_has(X86_FEATURE_SMAP) ||
+ (IS_ENABLED(CONFIG_64_BIT) &&
boot_cpu_has(X86_FEATURE_XENPV)))
+ mask |= X86_EFLAGS_AC;
Is the explicit Xen check necessary? IIRC the Xen hypervisor will
filter out the SMAP bit in the cpuid pvop.
Right, and this test will nevertheless result in setting AC in the mask.
IIRC this was the main objective here.
Juergen
Re: [PATCH 01/13] x86/entry: Fix AC assertion
On Wed, Sep 2, 2020 at 9:38 AM Peter Zijlstra wrote:
>
> From: Peter Zijlstra
>
> The WARN added in commit 3c73b81a9164 ("x86/entry, selftests: Further
> improve user entry sanity checks") unconditionally triggers on my IVB
> machine because it does not support SMAP.
>
> For !SMAP hardware we patch out CLAC/STAC instructions and thus if
> userspace sets AC, we'll still have it set after entry.
>
> Fixes: 3c73b81a9164 ("x86/entry, selftests: Further improve user entry sanity
> checks")
> Signed-off-by: Peter Zijlstra (Intel)
> Acked-by: Andy Lutomirski
> ---
> arch/x86/include/asm/entry-common.h | 11 +--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> --- a/arch/x86/include/asm/entry-common.h
> +++ b/arch/x86/include/asm/entry-common.h
> @@ -18,8 +18,16 @@ static __always_inline void arch_check_u
> * state, not the interrupt state as imagined by Xen.
> */
> unsigned long flags = native_save_fl();
> - WARN_ON_ONCE(flags & (X86_EFLAGS_AC | X86_EFLAGS_DF |
> - X86_EFLAGS_NT));
> + unsigned long mask = X86_EFLAGS_DF | X86_EFLAGS_NT;
> +
> + /*
> +* For !SMAP hardware we patch out CLAC on entry.
> +*/
> + if (boot_cpu_has(X86_FEATURE_SMAP) ||
> + (IS_ENABLED(CONFIG_64_BIT) &&
> boot_cpu_has(X86_FEATURE_XENPV)))
> + mask |= X86_EFLAGS_AC;
Is the explicit Xen check necessary? IIRC the Xen hypervisor will
filter out the SMAP bit in the cpuid pvop.
--
Brian Gerst
[PATCH 01/13] x86/entry: Fix AC assertion
From: Peter Zijlstra
The WARN added in commit 3c73b81a9164 ("x86/entry, selftests: Further
improve user entry sanity checks") unconditionally triggers on my IVB
machine because it does not support SMAP.
For !SMAP hardware we patch out CLAC/STAC instructions and thus if
userspace sets AC, we'll still have it set after entry.
Fixes: 3c73b81a9164 ("x86/entry, selftests: Further improve user entry sanity
checks")
Signed-off-by: Peter Zijlstra (Intel)
Acked-by: Andy Lutomirski
---
arch/x86/include/asm/entry-common.h | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
--- a/arch/x86/include/asm/entry-common.h
+++ b/arch/x86/include/asm/entry-common.h
@@ -18,8 +18,16 @@ static __always_inline void arch_check_u
* state, not the interrupt state as imagined by Xen.
*/
unsigned long flags = native_save_fl();
- WARN_ON_ONCE(flags & (X86_EFLAGS_AC | X86_EFLAGS_DF |
- X86_EFLAGS_NT));
+ unsigned long mask = X86_EFLAGS_DF | X86_EFLAGS_NT;
+
+ /*
+* For !SMAP hardware we patch out CLAC on entry.
+*/
+ if (boot_cpu_has(X86_FEATURE_SMAP) ||
+ (IS_ENABLED(CONFIG_64_BIT) &&
boot_cpu_has(X86_FEATURE_XENPV)))
+ mask |= X86_EFLAGS_AC;
+
+ WARN_ON_ONCE(flags & mask);
/* We think we came from user mode. Make sure pt_regs agrees. */
WARN_ON_ONCE(!user_mode(regs));
