2020-09-02 13:50:52

by Peter Zijlstra

Subject: [PATCH 01/13] x86/entry: Fix AC assertion

From: Peter Zijlstra <[email protected]>

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) <[email protected]>
Acked-by: Andy Lutomirski <[email protected]>
---
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));

2020-09-02 15:59:43

by Brian Gerst

Subject: Re: [PATCH 01/13] x86/entry: Fix AC assertion

On Wed, Sep 2, 2020 at 9:38 AM Peter Zijlstra <[email protected]> wrote:
>
> From: Peter Zijlstra <[email protected]>
>
> 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) <[email protected]>
> Acked-by: Andy Lutomirski <[email protected]>
> ---
> 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

2020-09-02 16:27:41

by Jürgen Groß

Subject: 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 <[email protected]> wrote:
>>
>> From: Peter Zijlstra <[email protected]>
>>
>> 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) <[email protected]>
>> Acked-by: Andy Lutomirski <[email protected]>
>> ---
>> 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

2020-09-02 16:34:49

by Peter Zijlstra

Subject: 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 <[email protected]> wrote:
> > >
> > > From: Peter Zijlstra <[email protected]>
> > >
> > > 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) <[email protected]>
> > > Acked-by: Andy Lutomirski <[email protected]>
> > > ---
> > > 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.

2020-09-02 17:06:35

by Brian Gerst

Subject: Re: [PATCH 01/13] x86/entry: Fix AC assertion

On Wed, Sep 2, 2020 at 12:31 PM <[email protected]> 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 <[email protected]> wrote:
> > > >
> > > > From: Peter Zijlstra <[email protected]>
> > > >
> > > > 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) <[email protected]>
> > > > Acked-by: Andy Lutomirski <[email protected]>
> > > > ---
> > > > 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

2020-09-04 13:27:54

by tip-bot2 for Alexey Makhalov

Subject: [tip: x86/urgent] x86/entry: Fix AC assertion

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID: 662a0221893a3d58aa72719671844264306f6e4b
Gitweb: https://git.kernel.org/tip/662a0221893a3d58aa72719671844264306f6e4b
Author: Peter Zijlstra <[email protected]>
AuthorDate: Wed, 02 Sep 2020 15:25:50 +02:00
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Fri, 04 Sep 2020 15:09:29 +02:00

x86/entry: Fix AC assertion

The WARN added in commit 3c73b81a9164 ("x86/entry, selftests: Further
improve user entry sanity checks") unconditionally triggers on a IVB
machine because it does not support SMAP.

For !SMAP hardware the CLAC/STAC instructions are patched out and thus if
userspace sets AC, it is still have set after entry.

Fixes: 3c73b81a9164 ("x86/entry, selftests: Further improve user entry sanity checks")
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Tested-by: Daniel Thompson <[email protected]>
Acked-by: Andy Lutomirski <[email protected]>
Cc: [email protected]
Link: https://lore.kernel.org/r/[email protected]

---
arch/x86/include/asm/entry-common.h | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/entry-common.h b/arch/x86/include/asm/entry-common.h
index a8f9315..6fe54b2 100644
--- 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_user_regs(struct pt_regs *regs)
* 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));