diff mbox series

[v2,33/39] x86/cpufeatures: Limit shadow stack to Intel CPUs

Message ID 20220929222936.14584-34-rick.p.edgecombe@intel.com (mailing list archive)
State New
Headers show
Series Shadowstacks for userspace | expand

Commit Message

Edgecombe, Rick P Sept. 29, 2022, 10:29 p.m. UTC
Shadow stack is supported on newer AMD processors, but the kernel
implementation has not been tested on them. Prevent basic issues from
showing up for normal users by disabling shadow stack on all CPUs except
Intel until it has been tested. At which point the limitation should be
removed.

Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>

---

v1:
 - New patch.

 arch/x86/kernel/cpu/common.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Kees Cook Oct. 3, 2022, 11:57 p.m. UTC | #1
On Thu, Sep 29, 2022 at 03:29:30PM -0700, Rick Edgecombe wrote:
> Shadow stack is supported on newer AMD processors, but the kernel
> implementation has not been tested on them. Prevent basic issues from
> showing up for normal users by disabling shadow stack on all CPUs except
> Intel until it has been tested. At which point the limitation should be
> removed.
> 
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>

So running the selftests on an AMD system is sufficient to drop this
patch?
Dave Hansen Oct. 4, 2022, 12:09 a.m. UTC | #2
On 10/3/22 16:57, Kees Cook wrote:
> On Thu, Sep 29, 2022 at 03:29:30PM -0700, Rick Edgecombe wrote:
>> Shadow stack is supported on newer AMD processors, but the kernel
>> implementation has not been tested on them. Prevent basic issues from
>> showing up for normal users by disabling shadow stack on all CPUs except
>> Intel until it has been tested. At which point the limitation should be
>> removed.
>>
>> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> So running the selftests on an AMD system is sufficient to drop this
> patch?

Yes, that's enough.

I _thought_ the AMD folks provided some tested-by's at some point in the
past.  But, maybe I'm confusing this for one of the other shared
features.  Either way, I'm sure no tested-by's were dropped on purpose.

I'm sure Rick is eager to trim down his series and this would be a great
patch to drop.  Does anyone want to make that easy for Rick?

<hint> <hint>
Kees Cook Oct. 4, 2022, 4:54 a.m. UTC | #3
On Mon, Oct 03, 2022 at 05:09:04PM -0700, Dave Hansen wrote:
> On 10/3/22 16:57, Kees Cook wrote:
> > On Thu, Sep 29, 2022 at 03:29:30PM -0700, Rick Edgecombe wrote:
> >> Shadow stack is supported on newer AMD processors, but the kernel
> >> implementation has not been tested on them. Prevent basic issues from
> >> showing up for normal users by disabling shadow stack on all CPUs except
> >> Intel until it has been tested. At which point the limitation should be
> >> removed.
> >>
> >> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> > So running the selftests on an AMD system is sufficient to drop this
> > patch?
> 
> Yes, that's enough.
> 
> I _thought_ the AMD folks provided some tested-by's at some point in the
> past.  But, maybe I'm confusing this for one of the other shared
> features.  Either way, I'm sure no tested-by's were dropped on purpose.
> 
> I'm sure Rick is eager to trim down his series and this would be a great
> patch to drop.  Does anyone want to make that easy for Rick?
> 
> <hint> <hint>

Hey Gustavo, Nathan, or Nick! I know y'all have some fancy AMD testing
rigs. Got a moment to spin up this series and run the selftests? :)
Mike Rapoport Oct. 4, 2022, 8:36 a.m. UTC | #4
On Mon, Oct 03, 2022 at 05:09:04PM -0700, Dave Hansen wrote:
> On 10/3/22 16:57, Kees Cook wrote:
> > On Thu, Sep 29, 2022 at 03:29:30PM -0700, Rick Edgecombe wrote:
> >> Shadow stack is supported on newer AMD processors, but the kernel
> >> implementation has not been tested on them. Prevent basic issues from
> >> showing up for normal users by disabling shadow stack on all CPUs except
> >> Intel until it has been tested. At which point the limitation should be
> >> removed.
> >>
> >> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> > So running the selftests on an AMD system is sufficient to drop this
> > patch?
> 
> Yes, that's enough.
> 
> I _thought_ the AMD folks provided some tested-by's at some point in the
> past.  But, maybe I'm confusing this for one of the other shared
> features.  Either way, I'm sure no tested-by's were dropped on purpose.
> 
> I'm sure Rick is eager to trim down his series and this would be a great
> patch to drop.  Does anyone want to make that easy for Rick?

FWIW, I've run CRIU test suite with the previous version of this set on an
AMD machine.
 
> <hint> <hint>
Nathan Chancellor Oct. 4, 2022, 3:47 p.m. UTC | #5
Hi Kees,

On Mon, Oct 03, 2022 at 09:54:26PM -0700, Kees Cook wrote:
> On Mon, Oct 03, 2022 at 05:09:04PM -0700, Dave Hansen wrote:
> > On 10/3/22 16:57, Kees Cook wrote:
> > > On Thu, Sep 29, 2022 at 03:29:30PM -0700, Rick Edgecombe wrote:
> > >> Shadow stack is supported on newer AMD processors, but the kernel
> > >> implementation has not been tested on them. Prevent basic issues from
> > >> showing up for normal users by disabling shadow stack on all CPUs except
> > >> Intel until it has been tested. At which point the limitation should be
> > >> removed.
> > >>
> > >> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> > > So running the selftests on an AMD system is sufficient to drop this
> > > patch?
> > 
> > Yes, that's enough.
> > 
> > I _thought_ the AMD folks provided some tested-by's at some point in the
> > past.  But, maybe I'm confusing this for one of the other shared
> > features.  Either way, I'm sure no tested-by's were dropped on purpose.
> > 
> > I'm sure Rick is eager to trim down his series and this would be a great
> > patch to drop.  Does anyone want to make that easy for Rick?
> > 
> > <hint> <hint>
> 
> Hey Gustavo, Nathan, or Nick! I know y'all have some fancy AMD testing
> rigs. Got a moment to spin up this series and run the selftests? :)

I do have access to a system with an EPYC 7513, which does have Shadow
Stack support (I can see 'shstk' in the "Flags" section of lscpu with
this series). As far as I understand it, AMD only added Shadow Stack
with Zen 3; my regular AMD test system is Zen 2 (probably should look at
procurring a Zen 3 or Zen 4 one at some point).

I applied this series on top of 6.0 and reverted this change then booted
it on that system. After building the selftest (which did require
'make headers_install' and a small addition to make it build beyond
that, see below), I ran it and this was the result. I am not sure if
that is expected or not but the other results seem promising for
dropping this patch.

  $ ./test_shadow_stack_64
  [INFO]  new_ssp = 7f8a36c9fff8, *new_ssp = 7f8a36ca0001
  [INFO]  changing ssp from 7f8a374a0ff0 to 7f8a36c9fff8
  [INFO]  ssp is now 7f8a36ca0000
  [OK]    Shadow stack pivot
  [OK]    Shadow stack faults
  [INFO]  Corrupting shadow stack
  [INFO]  Generated shadow stack violation successfully
  [OK]    Shadow stack violation test
  [INFO]  Gup read -> shstk access success
  [INFO]  Gup write -> shstk access success
  [INFO]  Violation from normal write
  [INFO]  Gup read -> write access success
  [INFO]  Violation from normal write
  [INFO]  Gup write -> write access success
  [INFO]  Cow gup write -> write access success
  [OK]    Shadow gup test
  [INFO]  Violation from shstk access
  [OK]    mprotect() test
  [OK]    Userfaultfd test
  [FAIL]  Alt shadow stack test

  $ echo $?
  1

I am happy to provide any information that would be useful for exploring
this failure and test further iterations of this series as necessary.

Cheers,
Nathan

test_shadow_stack.c: In function ‘create_shstk’:
test_shadow_stack.c:86:70: error: ‘SHADOW_STACK_SET_TOKEN’ undeclared (first use in this function)
   86 |         return (void *)syscall(__NR_map_shadow_stack, addr, SS_SIZE, SHADOW_STACK_SET_TOKEN);
      |                                                                      ^~~~~~~~~~~~~~~~~~~~~~
test_shadow_stack.c:86:70: note: each undeclared identifier is reported only once for each function it appears in
test_shadow_stack.c:87:1: warning: control reaches end of non-void function [-Wreturn-type]
   87 | }
      | ^

diff --git a/tools/testing/selftests/x86/test_shadow_stack.c b/tools/testing/selftests/x86/test_shadow_stack.c
index 22b856de5cdd..958dbb248518 100644
--- a/tools/testing/selftests/x86/test_shadow_stack.c
+++ b/tools/testing/selftests/x86/test_shadow_stack.c
@@ -11,6 +11,7 @@
 #define _GNU_SOURCE
 
 #include <sys/syscall.h>
+#include <asm/mman.h>
 #include <sys/mman.h>
 #include <sys/stat.h>
 #include <sys/wait.h>
John Allen Oct. 4, 2022, 7:43 p.m. UTC | #6
On 10/4/22 10:47 AM, Nathan Chancellor wrote:
> Hi Kees,
> 
> On Mon, Oct 03, 2022 at 09:54:26PM -0700, Kees Cook wrote:
>> On Mon, Oct 03, 2022 at 05:09:04PM -0700, Dave Hansen wrote:
>>> On 10/3/22 16:57, Kees Cook wrote:
>>>> On Thu, Sep 29, 2022 at 03:29:30PM -0700, Rick Edgecombe wrote:
>>>>> Shadow stack is supported on newer AMD processors, but the kernel
>>>>> implementation has not been tested on them. Prevent basic issues from
>>>>> showing up for normal users by disabling shadow stack on all CPUs except
>>>>> Intel until it has been tested. At which point the limitation should be
>>>>> removed.
>>>>>
>>>>> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
>>>> So running the selftests on an AMD system is sufficient to drop this
>>>> patch?
>>>
>>> Yes, that's enough.
>>>
>>> I _thought_ the AMD folks provided some tested-by's at some point in the
>>> past.  But, maybe I'm confusing this for one of the other shared
>>> features.  Either way, I'm sure no tested-by's were dropped on purpose.
>>>
>>> I'm sure Rick is eager to trim down his series and this would be a great
>>> patch to drop.  Does anyone want to make that easy for Rick?
>>>
>>> <hint> <hint>
>>
>> Hey Gustavo, Nathan, or Nick! I know y'all have some fancy AMD testing
>> rigs. Got a moment to spin up this series and run the selftests? :)
> 
> I do have access to a system with an EPYC 7513, which does have Shadow
> Stack support (I can see 'shstk' in the "Flags" section of lscpu with
> this series). As far as I understand it, AMD only added Shadow Stack
> with Zen 3; my regular AMD test system is Zen 2 (probably should look at
> procurring a Zen 3 or Zen 4 one at some point).
> 
> I applied this series on top of 6.0 and reverted this change then booted
> it on that system. After building the selftest (which did require
> 'make headers_install' and a small addition to make it build beyond
> that, see below), I ran it and this was the result. I am not sure if
> that is expected or not but the other results seem promising for
> dropping this patch.
> 
>   $ ./test_shadow_stack_64
>   [INFO]  new_ssp = 7f8a36c9fff8, *new_ssp = 7f8a36ca0001
>   [INFO]  changing ssp from 7f8a374a0ff0 to 7f8a36c9fff8
>   [INFO]  ssp is now 7f8a36ca0000
>   [OK]    Shadow stack pivot
>   [OK]    Shadow stack faults
>   [INFO]  Corrupting shadow stack
>   [INFO]  Generated shadow stack violation successfully
>   [OK]    Shadow stack violation test
>   [INFO]  Gup read -> shstk access success
>   [INFO]  Gup write -> shstk access success
>   [INFO]  Violation from normal write
>   [INFO]  Gup read -> write access success
>   [INFO]  Violation from normal write
>   [INFO]  Gup write -> write access success
>   [INFO]  Cow gup write -> write access success
>   [OK]    Shadow gup test
>   [INFO]  Violation from shstk access
>   [OK]    mprotect() test
>   [OK]    Userfaultfd test
>   [FAIL]  Alt shadow stack test

The selftest is looking OK on my system (Dell PowerEdge R6515 w/ EPYC
7713). I also just pulled a fresh 6.0 kernel and applied the series
including the fix Nathan mentions below.

$ tools/testing/selftests/x86/test_shadow_stack_64
[INFO]  new_ssp = 7f30cccc5ff8, *new_ssp = 7f30cccc6001
[INFO]  changing ssp from 7f30cd4c6ff0 to 7f30cccc5ff8
[INFO]  ssp is now 7f30cccc6000
[OK]    Shadow stack pivot
[OK]    Shadow stack faults
[INFO]  Corrupting shadow stack
[INFO]  Generated shadow stack violation successfully
[OK]    Shadow stack violation test
[INFO]  Gup read -> shstk access success
[INFO]  Gup write -> shstk access success
[INFO]  Violation from normal write
[INFO]  Gup read -> write access success
[INFO]  Violation from normal write
[INFO]  Gup write -> write access success
[INFO]  Cow gup write -> write access success
[OK]    Shadow gup test
[INFO]  Violation from shstk access
[OK]    mprotect() test
[OK]    Userfaultfd test
[OK]    Alt shadow stack test.

> 
>   $ echo $?
>   1
> 
> I am happy to provide any information that would be useful for exploring
> this failure and test further iterations of this series as necessary.
> 
> Cheers,
> Nathan
> 
> test_shadow_stack.c: In function ‘create_shstk’:
> test_shadow_stack.c:86:70: error: ‘SHADOW_STACK_SET_TOKEN’ undeclared (first use in this function)
>    86 |         return (void *)syscall(__NR_map_shadow_stack, addr, SS_SIZE, SHADOW_STACK_SET_TOKEN);
>       |                                                                      ^~~~~~~~~~~~~~~~~~~~~~
> test_shadow_stack.c:86:70: note: each undeclared identifier is reported only once for each function it appears in
> test_shadow_stack.c:87:1: warning: control reaches end of non-void function [-Wreturn-type]
>    87 | }
>       | ^
> 
> diff --git a/tools/testing/selftests/x86/test_shadow_stack.c b/tools/testing/selftests/x86/test_shadow_stack.c
> index 22b856de5cdd..958dbb248518 100644
> --- a/tools/testing/selftests/x86/test_shadow_stack.c
> +++ b/tools/testing/selftests/x86/test_shadow_stack.c
> @@ -11,6 +11,7 @@
>  #define _GNU_SOURCE
>  
>  #include <sys/syscall.h>
> +#include <asm/mman.h>
>  #include <sys/mman.h>
>  #include <sys/stat.h>
>  #include <sys/wait.h>
Edgecombe, Rick P Oct. 4, 2022, 8:34 p.m. UTC | #7
On Tue, 2022-10-04 at 14:43 -0500, John Allen wrote:
> On 10/4/22 10:47 AM, Nathan Chancellor wrote:
> > Hi Kees,
> > 
> > On Mon, Oct 03, 2022 at 09:54:26PM -0700, Kees Cook wrote:
> > > On Mon, Oct 03, 2022 at 05:09:04PM -0700, Dave Hansen wrote:
> > > > On 10/3/22 16:57, Kees Cook wrote:
> > > > > On Thu, Sep 29, 2022 at 03:29:30PM -0700, Rick Edgecombe
> > > > > wrote:
> > > > > > Shadow stack is supported on newer AMD processors, but the
> > > > > > kernel
> > > > > > implementation has not been tested on them. Prevent basic
> > > > > > issues from
> > > > > > showing up for normal users by disabling shadow stack on
> > > > > > all CPUs except
> > > > > > Intel until it has been tested. At which point the
> > > > > > limitation should be
> > > > > > removed.
> > > > > > 
> > > > > > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> > > > > 
> > > > > So running the selftests on an AMD system is sufficient to
> > > > > drop this
> > > > > patch?
> > > > 
> > > > Yes, that's enough.
> > > > 
> > > > I _thought_ the AMD folks provided some tested-by's at some
> > > > point in the
> > > > past.  But, maybe I'm confusing this for one of the other
> > > > shared
> > > > features.  Either way, I'm sure no tested-by's were dropped on
> > > > purpose.
> > > > 
> > > > I'm sure Rick is eager to trim down his series and this would
> > > > be a great
> > > > patch to drop.  Does anyone want to make that easy for Rick?
> > > > 
> > > > <hint> <hint>
> > > 
> > > Hey Gustavo, Nathan, or Nick! I know y'all have some fancy AMD
> > > testing
> > > rigs. Got a moment to spin up this series and run the selftests?
> > > :)
> > 
> > I do have access to a system with an EPYC 7513, which does have
> > Shadow
> > Stack support (I can see 'shstk' in the "Flags" section of lscpu
> > with
> > this series). As far as I understand it, AMD only added Shadow
> > Stack
> > with Zen 3; my regular AMD test system is Zen 2 (probably should
> > look at
> > procurring a Zen 3 or Zen 4 one at some point).
> > 
> > I applied this series on top of 6.0 and reverted this change then
> > booted
> > it on that system. After building the selftest (which did require
> > 'make headers_install' and a small addition to make it build beyond
> > that, see below), I ran it and this was the result. I am not sure
> > if
> > that is expected or not but the other results seem promising for
> > dropping this patch.
> > 
> >    $ ./test_shadow_stack_64
> >    [INFO]  new_ssp = 7f8a36c9fff8, *new_ssp = 7f8a36ca0001
> >    [INFO]  changing ssp from 7f8a374a0ff0 to 7f8a36c9fff8
> >    [INFO]  ssp is now 7f8a36ca0000
> >    [OK]    Shadow stack pivot
> >    [OK]    Shadow stack faults
> >    [INFO]  Corrupting shadow stack
> >    [INFO]  Generated shadow stack violation successfully
> >    [OK]    Shadow stack violation test
> >    [INFO]  Gup read -> shstk access success
> >    [INFO]  Gup write -> shstk access success
> >    [INFO]  Violation from normal write
> >    [INFO]  Gup read -> write access success
> >    [INFO]  Violation from normal write
> >    [INFO]  Gup write -> write access success
> >    [INFO]  Cow gup write -> write access success
> >    [OK]    Shadow gup test
> >    [INFO]  Violation from shstk access
> >    [OK]    mprotect() test
> >    [OK]    Userfaultfd test
> >    [FAIL]  Alt shadow stack test
> 
> The selftest is looking OK on my system (Dell PowerEdge R6515 w/ EPYC
> 7713). I also just pulled a fresh 6.0 kernel and applied the series
> including the fix Nathan mentions below.
> 
> $ tools/testing/selftests/x86/test_shadow_stack_64
> [INFO]  new_ssp = 7f30cccc5ff8, *new_ssp = 7f30cccc6001
> [INFO]  changing ssp from 7f30cd4c6ff0 to 7f30cccc5ff8
> [INFO]  ssp is now 7f30cccc6000
> [OK]    Shadow stack pivot
> [OK]    Shadow stack faults
> [INFO]  Corrupting shadow stack
> [INFO]  Generated shadow stack violation successfully
> [OK]    Shadow stack violation test
> [INFO]  Gup read -> shstk access success
> [INFO]  Gup write -> shstk access success
> [INFO]  Violation from normal write
> [INFO]  Gup read -> write access success
> [INFO]  Violation from normal write
> [INFO]  Gup write -> write access success
> [INFO]  Cow gup write -> write access success
> [OK]    Shadow gup test
> [INFO]  Violation from shstk access
> [OK]    mprotect() test
> [OK]    Userfaultfd test
> [OK]    Alt shadow stack test.

Thanks for the testing. Based on the test, I wonder if this could be a
SW bug. Nathan, could I send you a tweaked test with some more debug
information?

John, are we sure AMD and Intel systems behave the same with respect to
CPUs not creating Dirty=1,Write=0 PTEs in rare situations? Or any other
CET related differences we should hash out? Otherwise I'll drop the
patch for the next version. (and assuming the issue Nathan hit doesn't
turn up anything HW related).
Nathan Chancellor Oct. 4, 2022, 8:50 p.m. UTC | #8
On Tue, Oct 04, 2022 at 08:34:54PM +0000, Edgecombe, Rick P wrote:
> On Tue, 2022-10-04 at 14:43 -0500, John Allen wrote:
> > On 10/4/22 10:47 AM, Nathan Chancellor wrote:
> > > Hi Kees,
> > > 
> > > On Mon, Oct 03, 2022 at 09:54:26PM -0700, Kees Cook wrote:
> > > > On Mon, Oct 03, 2022 at 05:09:04PM -0700, Dave Hansen wrote:
> > > > > On 10/3/22 16:57, Kees Cook wrote:
> > > > > > On Thu, Sep 29, 2022 at 03:29:30PM -0700, Rick Edgecombe
> > > > > > wrote:
> > > > > > > Shadow stack is supported on newer AMD processors, but the
> > > > > > > kernel
> > > > > > > implementation has not been tested on them. Prevent basic
> > > > > > > issues from
> > > > > > > showing up for normal users by disabling shadow stack on
> > > > > > > all CPUs except
> > > > > > > Intel until it has been tested. At which point the
> > > > > > > limitation should be
> > > > > > > removed.
> > > > > > > 
> > > > > > > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> > > > > > 
> > > > > > So running the selftests on an AMD system is sufficient to
> > > > > > drop this
> > > > > > patch?
> > > > > 
> > > > > Yes, that's enough.
> > > > > 
> > > > > I _thought_ the AMD folks provided some tested-by's at some
> > > > > point in the
> > > > > past.  But, maybe I'm confusing this for one of the other
> > > > > shared
> > > > > features.  Either way, I'm sure no tested-by's were dropped on
> > > > > purpose.
> > > > > 
> > > > > I'm sure Rick is eager to trim down his series and this would
> > > > > be a great
> > > > > patch to drop.  Does anyone want to make that easy for Rick?
> > > > > 
> > > > > <hint> <hint>
> > > > 
> > > > Hey Gustavo, Nathan, or Nick! I know y'all have some fancy AMD
> > > > testing
> > > > rigs. Got a moment to spin up this series and run the selftests?
> > > > :)
> > > 
> > > I do have access to a system with an EPYC 7513, which does have
> > > Shadow
> > > Stack support (I can see 'shstk' in the "Flags" section of lscpu
> > > with
> > > this series). As far as I understand it, AMD only added Shadow
> > > Stack
> > > with Zen 3; my regular AMD test system is Zen 2 (probably should
> > > look at
> > > procurring a Zen 3 or Zen 4 one at some point).
> > > 
> > > I applied this series on top of 6.0 and reverted this change then
> > > booted
> > > it on that system. After building the selftest (which did require
> > > 'make headers_install' and a small addition to make it build beyond
> > > that, see below), I ran it and this was the result. I am not sure
> > > if
> > > that is expected or not but the other results seem promising for
> > > dropping this patch.
> > > 
> > >    $ ./test_shadow_stack_64
> > >    [INFO]  new_ssp = 7f8a36c9fff8, *new_ssp = 7f8a36ca0001
> > >    [INFO]  changing ssp from 7f8a374a0ff0 to 7f8a36c9fff8
> > >    [INFO]  ssp is now 7f8a36ca0000
> > >    [OK]    Shadow stack pivot
> > >    [OK]    Shadow stack faults
> > >    [INFO]  Corrupting shadow stack
> > >    [INFO]  Generated shadow stack violation successfully
> > >    [OK]    Shadow stack violation test
> > >    [INFO]  Gup read -> shstk access success
> > >    [INFO]  Gup write -> shstk access success
> > >    [INFO]  Violation from normal write
> > >    [INFO]  Gup read -> write access success
> > >    [INFO]  Violation from normal write
> > >    [INFO]  Gup write -> write access success
> > >    [INFO]  Cow gup write -> write access success
> > >    [OK]    Shadow gup test
> > >    [INFO]  Violation from shstk access
> > >    [OK]    mprotect() test
> > >    [OK]    Userfaultfd test
> > >    [FAIL]  Alt shadow stack test
> > 
> > The selftest is looking OK on my system (Dell PowerEdge R6515 w/ EPYC
> > 7713). I also just pulled a fresh 6.0 kernel and applied the series
> > including the fix Nathan mentions below.
> > 
> > $ tools/testing/selftests/x86/test_shadow_stack_64
> > [INFO]  new_ssp = 7f30cccc5ff8, *new_ssp = 7f30cccc6001
> > [INFO]  changing ssp from 7f30cd4c6ff0 to 7f30cccc5ff8
> > [INFO]  ssp is now 7f30cccc6000
> > [OK]    Shadow stack pivot
> > [OK]    Shadow stack faults
> > [INFO]  Corrupting shadow stack
> > [INFO]  Generated shadow stack violation successfully
> > [OK]    Shadow stack violation test
> > [INFO]  Gup read -> shstk access success
> > [INFO]  Gup write -> shstk access success
> > [INFO]  Violation from normal write
> > [INFO]  Gup read -> write access success
> > [INFO]  Violation from normal write
> > [INFO]  Gup write -> write access success
> > [INFO]  Cow gup write -> write access success
> > [OK]    Shadow gup test
> > [INFO]  Violation from shstk access
> > [OK]    mprotect() test
> > [OK]    Userfaultfd test
> > [OK]    Alt shadow stack test.
> 
> Thanks for the testing. Based on the test, I wonder if this could be a
> SW bug. Nathan, could I send you a tweaked test with some more debug
> information?

Yes, more than happy to help you look into this further!

> John, are we sure AMD and Intel systems behave the same with respect to
> CPUs not creating Dirty=1,Write=0 PTEs in rare situations? Or any other
> CET related differences we should hash out? Otherwise I'll drop the
> patch for the next version. (and assuming the issue Nathan hit doesn't
> turn up anything HW related).
H. Peter Anvin Oct. 4, 2022, 9:17 p.m. UTC | #9
On October 4, 2022 1:50:20 PM PDT, Nathan Chancellor <nathan@kernel.org> wrote:
>On Tue, Oct 04, 2022 at 08:34:54PM +0000, Edgecombe, Rick P wrote:
>> On Tue, 2022-10-04 at 14:43 -0500, John Allen wrote:
>> > On 10/4/22 10:47 AM, Nathan Chancellor wrote:
>> > > Hi Kees,
>> > > 
>> > > On Mon, Oct 03, 2022 at 09:54:26PM -0700, Kees Cook wrote:
>> > > > On Mon, Oct 03, 2022 at 05:09:04PM -0700, Dave Hansen wrote:
>> > > > > On 10/3/22 16:57, Kees Cook wrote:
>> > > > > > On Thu, Sep 29, 2022 at 03:29:30PM -0700, Rick Edgecombe
>> > > > > > wrote:
>> > > > > > > Shadow stack is supported on newer AMD processors, but the
>> > > > > > > kernel
>> > > > > > > implementation has not been tested on them. Prevent basic
>> > > > > > > issues from
>> > > > > > > showing up for normal users by disabling shadow stack on
>> > > > > > > all CPUs except
>> > > > > > > Intel until it has been tested. At which point the
>> > > > > > > limitation should be
>> > > > > > > removed.
>> > > > > > > 
>> > > > > > > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
>> > > > > > 
>> > > > > > So running the selftests on an AMD system is sufficient to
>> > > > > > drop this
>> > > > > > patch?
>> > > > > 
>> > > > > Yes, that's enough.
>> > > > > 
>> > > > > I _thought_ the AMD folks provided some tested-by's at some
>> > > > > point in the
>> > > > > past.  But, maybe I'm confusing this for one of the other
>> > > > > shared
>> > > > > features.  Either way, I'm sure no tested-by's were dropped on
>> > > > > purpose.
>> > > > > 
>> > > > > I'm sure Rick is eager to trim down his series and this would
>> > > > > be a great
>> > > > > patch to drop.  Does anyone want to make that easy for Rick?
>> > > > > 
>> > > > > <hint> <hint>
>> > > > 
>> > > > Hey Gustavo, Nathan, or Nick! I know y'all have some fancy AMD
>> > > > testing
>> > > > rigs. Got a moment to spin up this series and run the selftests?
>> > > > :)
>> > > 
>> > > I do have access to a system with an EPYC 7513, which does have
>> > > Shadow
>> > > Stack support (I can see 'shstk' in the "Flags" section of lscpu
>> > > with
>> > > this series). As far as I understand it, AMD only added Shadow
>> > > Stack
>> > > with Zen 3; my regular AMD test system is Zen 2 (probably should
>> > > look at
>> > > procurring a Zen 3 or Zen 4 one at some point).
>> > > 
>> > > I applied this series on top of 6.0 and reverted this change then
>> > > booted
>> > > it on that system. After building the selftest (which did require
>> > > 'make headers_install' and a small addition to make it build beyond
>> > > that, see below), I ran it and this was the result. I am not sure
>> > > if
>> > > that is expected or not but the other results seem promising for
>> > > dropping this patch.
>> > > 
>> > >    $ ./test_shadow_stack_64
>> > >    [INFO]  new_ssp = 7f8a36c9fff8, *new_ssp = 7f8a36ca0001
>> > >    [INFO]  changing ssp from 7f8a374a0ff0 to 7f8a36c9fff8
>> > >    [INFO]  ssp is now 7f8a36ca0000
>> > >    [OK]    Shadow stack pivot
>> > >    [OK]    Shadow stack faults
>> > >    [INFO]  Corrupting shadow stack
>> > >    [INFO]  Generated shadow stack violation successfully
>> > >    [OK]    Shadow stack violation test
>> > >    [INFO]  Gup read -> shstk access success
>> > >    [INFO]  Gup write -> shstk access success
>> > >    [INFO]  Violation from normal write
>> > >    [INFO]  Gup read -> write access success
>> > >    [INFO]  Violation from normal write
>> > >    [INFO]  Gup write -> write access success
>> > >    [INFO]  Cow gup write -> write access success
>> > >    [OK]    Shadow gup test
>> > >    [INFO]  Violation from shstk access
>> > >    [OK]    mprotect() test
>> > >    [OK]    Userfaultfd test
>> > >    [FAIL]  Alt shadow stack test
>> > 
>> > The selftest is looking OK on my system (Dell PowerEdge R6515 w/ EPYC
>> > 7713). I also just pulled a fresh 6.0 kernel and applied the series
>> > including the fix Nathan mentions below.
>> > 
>> > $ tools/testing/selftests/x86/test_shadow_stack_64
>> > [INFO]  new_ssp = 7f30cccc5ff8, *new_ssp = 7f30cccc6001
>> > [INFO]  changing ssp from 7f30cd4c6ff0 to 7f30cccc5ff8
>> > [INFO]  ssp is now 7f30cccc6000
>> > [OK]    Shadow stack pivot
>> > [OK]    Shadow stack faults
>> > [INFO]  Corrupting shadow stack
>> > [INFO]  Generated shadow stack violation successfully
>> > [OK]    Shadow stack violation test
>> > [INFO]  Gup read -> shstk access success
>> > [INFO]  Gup write -> shstk access success
>> > [INFO]  Violation from normal write
>> > [INFO]  Gup read -> write access success
>> > [INFO]  Violation from normal write
>> > [INFO]  Gup write -> write access success
>> > [INFO]  Cow gup write -> write access success
>> > [OK]    Shadow gup test
>> > [INFO]  Violation from shstk access
>> > [OK]    mprotect() test
>> > [OK]    Userfaultfd test
>> > [OK]    Alt shadow stack test.
>> 
>> Thanks for the testing. Based on the test, I wonder if this could be a
>> SW bug. Nathan, could I send you a tweaked test with some more debug
>> information?
>
>Yes, more than happy to help you look into this further!
>
>> John, are we sure AMD and Intel systems behave the same with respect to
>> CPUs not creating Dirty=1,Write=0 PTEs in rare situations? Or any other
>> CET related differences we should hash out? Otherwise I'll drop the
>> patch for the next version. (and assuming the issue Nathan hit doesn't
>> turn up anything HW related).

I have to admit to being a bit confused here... in general, we trust CPUID bits unless they are *known* to be wrong, in which case we blacklist them.

If AMD advertises the feature but it doesn't work or they didn't validate it, that would be a (serious!) bug on their part that we can address by blacklisting, but they should also fix with a microcode/BIOS patch.

What am I missing?
Edgecombe, Rick P Oct. 4, 2022, 11:24 p.m. UTC | #10
On Tue, 2022-10-04 at 14:17 -0700, H. Peter Anvin wrote:
> On October 4, 2022 1:50:20 PM PDT, Nathan Chancellor <
> nathan@kernel.org> wrote:
> > On Tue, Oct 04, 2022 at 08:34:54PM +0000, Edgecombe, Rick P wrote:
> > > On Tue, 2022-10-04 at 14:43 -0500, John Allen wrote:
> > > > On 10/4/22 10:47 AM, Nathan Chancellor wrote:
> > > > > Hi Kees,
> > > > > 
> > > > > On Mon, Oct 03, 2022 at 09:54:26PM -0700, Kees Cook wrote:
> > > > > > On Mon, Oct 03, 2022 at 05:09:04PM -0700, Dave Hansen
> > > > > > wrote:
> > > > > > > On 10/3/22 16:57, Kees Cook wrote:
> > > > > > > > On Thu, Sep 29, 2022 at 03:29:30PM -0700, Rick
> > > > > > > > Edgecombe
> > > > > > > > wrote:
> > > > > > > > > Shadow stack is supported on newer AMD processors,
> > > > > > > > > but the
> > > > > > > > > kernel
> > > > > > > > > implementation has not been tested on them. Prevent
> > > > > > > > > basic
> > > > > > > > > issues from
> > > > > > > > > showing up for normal users by disabling shadow stack
> > > > > > > > > on
> > > > > > > > > all CPUs except
> > > > > > > > > Intel until it has been tested. At which point the
> > > > > > > > > limitation should be
> > > > > > > > > removed.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Rick Edgecombe <
> > > > > > > > > rick.p.edgecombe@intel.com>
> > > > > > > > 
> > > > > > > > So running the selftests on an AMD system is sufficient
> > > > > > > > to
> > > > > > > > drop this
> > > > > > > > patch?
> > > > > > > 
> > > > > > > Yes, that's enough.
> > > > > > > 
> > > > > > > I _thought_ the AMD folks provided some tested-by's at
> > > > > > > some
> > > > > > > point in the
> > > > > > > past.  But, maybe I'm confusing this for one of the other
> > > > > > > shared
> > > > > > > features.  Either way, I'm sure no tested-by's were
> > > > > > > dropped on
> > > > > > > purpose.
> > > > > > > 
> > > > > > > I'm sure Rick is eager to trim down his series and this
> > > > > > > would
> > > > > > > be a great
> > > > > > > patch to drop.  Does anyone want to make that easy for
> > > > > > > Rick?
> > > > > > > 
> > > > > > > <hint> <hint>
> > > > > > 
> > > > > > Hey Gustavo, Nathan, or Nick! I know y'all have some fancy
> > > > > > AMD
> > > > > > testing
> > > > > > rigs. Got a moment to spin up this series and run the
> > > > > > selftests?
> > > > > > :)
> > > > > 
> > > > > I do have access to a system with an EPYC 7513, which does
> > > > > have
> > > > > Shadow
> > > > > Stack support (I can see 'shstk' in the "Flags" section of
> > > > > lscpu
> > > > > with
> > > > > this series). As far as I understand it, AMD only added
> > > > > Shadow
> > > > > Stack
> > > > > with Zen 3; my regular AMD test system is Zen 2 (probably
> > > > > should
> > > > > look at
> > > > > procurring a Zen 3 or Zen 4 one at some point).
> > > > > 
> > > > > I applied this series on top of 6.0 and reverted this change
> > > > > then
> > > > > booted
> > > > > it on that system. After building the selftest (which did
> > > > > require
> > > > > 'make headers_install' and a small addition to make it build
> > > > > beyond
> > > > > that, see below), I ran it and this was the result. I am not
> > > > > sure
> > > > > if
> > > > > that is expected or not but the other results seem promising
> > > > > for
> > > > > dropping this patch.
> > > > > 
> > > > >    $ ./test_shadow_stack_64
> > > > >    [INFO]  new_ssp = 7f8a36c9fff8, *new_ssp = 7f8a36ca0001
> > > > >    [INFO]  changing ssp from 7f8a374a0ff0 to 7f8a36c9fff8
> > > > >    [INFO]  ssp is now 7f8a36ca0000
> > > > >    [OK]    Shadow stack pivot
> > > > >    [OK]    Shadow stack faults
> > > > >    [INFO]  Corrupting shadow stack
> > > > >    [INFO]  Generated shadow stack violation successfully
> > > > >    [OK]    Shadow stack violation test
> > > > >    [INFO]  Gup read -> shstk access success
> > > > >    [INFO]  Gup write -> shstk access success
> > > > >    [INFO]  Violation from normal write
> > > > >    [INFO]  Gup read -> write access success
> > > > >    [INFO]  Violation from normal write
> > > > >    [INFO]  Gup write -> write access success
> > > > >    [INFO]  Cow gup write -> write access success
> > > > >    [OK]    Shadow gup test
> > > > >    [INFO]  Violation from shstk access
> > > > >    [OK]    mprotect() test
> > > > >    [OK]    Userfaultfd test
> > > > >    [FAIL]  Alt shadow stack test
> > > > 
> > > > The selftest is looking OK on my system (Dell PowerEdge R6515
> > > > w/ EPYC
> > > > 7713). I also just pulled a fresh 6.0 kernel and applied the
> > > > series
> > > > including the fix Nathan mentions below.
> > > > 
> > > > $ tools/testing/selftests/x86/test_shadow_stack_64
> > > > [INFO]  new_ssp = 7f30cccc5ff8, *new_ssp = 7f30cccc6001
> > > > [INFO]  changing ssp from 7f30cd4c6ff0 to 7f30cccc5ff8
> > > > [INFO]  ssp is now 7f30cccc6000
> > > > [OK]    Shadow stack pivot
> > > > [OK]    Shadow stack faults
> > > > [INFO]  Corrupting shadow stack
> > > > [INFO]  Generated shadow stack violation successfully
> > > > [OK]    Shadow stack violation test
> > > > [INFO]  Gup read -> shstk access success
> > > > [INFO]  Gup write -> shstk access success
> > > > [INFO]  Violation from normal write
> > > > [INFO]  Gup read -> write access success
> > > > [INFO]  Violation from normal write
> > > > [INFO]  Gup write -> write access success
> > > > [INFO]  Cow gup write -> write access success
> > > > [OK]    Shadow gup test
> > > > [INFO]  Violation from shstk access
> > > > [OK]    mprotect() test
> > > > [OK]    Userfaultfd test
> > > > [OK]    Alt shadow stack test.
> > > 
> > > Thanks for the testing. Based on the test, I wonder if this could
> > > be a
> > > SW bug. Nathan, could I send you a tweaked test with some more
> > > debug
> > > information?
> > 
> > Yes, more than happy to help you look into this further!
> > 
> > > John, are we sure AMD and Intel systems behave the same with
> > > respect to
> > > CPUs not creating Dirty=1,Write=0 PTEs in rare situations? Or any
> > > other
> > > CET related differences we should hash out? Otherwise I'll drop
> > > the
> > > patch for the next version. (and assuming the issue Nathan hit
> > > doesn't
> > > turn up anything HW related).
> 
> I have to admit to being a bit confused here... in general, we trust
> CPUID bits unless they are *known* to be wrong, in which case we
> blacklist them.
> 
> If AMD advertises the feature but it doesn't work or they didn't
> validate it, that would be a (serious!) bug on their part that we can
> address by blacklisting, but they should also fix with a
> microcode/BIOS patch.
> 
> What am I missing?

I have not read anything about the AMD implementation except hearing
that it is supported. But there are some microarchitectual-like aspects
to this CET Linux implementation, around requiring CPUs to not create
Dirty=1,Write=0 PTEs in some cases, where they did in the past. In
another thread Jann asked how the IOMMU works with respect to this edge
case and I'm currently trying to chase down that answer for even Intel
HW. So I just wanted to double check that we expect that everything
should be the same. In either case we still have time to iron things
out before anything gets upstream.
Edgecombe, Rick P Oct. 20, 2022, 9:22 p.m. UTC | #11
On Tue, 2022-10-04 at 13:50 -0700, Nathan Chancellor wrote:
> On Tue, Oct 04, 2022 at 08:34:54PM +0000, Edgecombe, Rick P wrote:
> > On Tue, 2022-10-04 at 14:43 -0500, John Allen wrote:
> > > On 10/4/22 10:47 AM, Nathan Chancellor wrote:
> > > > Hi Kees,
> > > > 
> > > > On Mon, Oct 03, 2022 at 09:54:26PM -0700, Kees Cook wrote:
> > > > > On Mon, Oct 03, 2022 at 05:09:04PM -0700, Dave Hansen wrote:
> > > > > > On 10/3/22 16:57, Kees Cook wrote:
> > > > > > > On Thu, Sep 29, 2022 at 03:29:30PM -0700, Rick Edgecombe
> > > > > > > wrote:
> > > > > > > > Shadow stack is supported on newer AMD processors, but
> > > > > > > > the
> > > > > > > > kernel
> > > > > > > > implementation has not been tested on them. Prevent
> > > > > > > > basic
> > > > > > > > issues from
> > > > > > > > showing up for normal users by disabling shadow stack
> > > > > > > > on
> > > > > > > > all CPUs except
> > > > > > > > Intel until it has been tested. At which point the
> > > > > > > > limitation should be
> > > > > > > > removed.
> > > > > > > > 
> > > > > > > > Signed-off-by: Rick Edgecombe <
> > > > > > > > rick.p.edgecombe@intel.com>
> > > > > > > 
> > > > > > > So running the selftests on an AMD system is sufficient
> > > > > > > to
> > > > > > > drop this
> > > > > > > patch?
> > > > > > 
> > > > > > Yes, that's enough.
> > > > > > 
> > > > > > I _thought_ the AMD folks provided some tested-by's at some
> > > > > > point in the
> > > > > > past.  But, maybe I'm confusing this for one of the other
> > > > > > shared
> > > > > > features.  Either way, I'm sure no tested-by's were dropped
> > > > > > on
> > > > > > purpose.
> > > > > > 
> > > > > > I'm sure Rick is eager to trim down his series and this
> > > > > > would
> > > > > > be a great
> > > > > > patch to drop.  Does anyone want to make that easy for
> > > > > > Rick?
> > > > > > 
> > > > > > <hint> <hint>
> > > > > 
> > > > > Hey Gustavo, Nathan, or Nick! I know y'all have some fancy
> > > > > AMD
> > > > > testing
> > > > > rigs. Got a moment to spin up this series and run the
> > > > > selftests?
> > > > > :)
> > > > 
> > > > I do have access to a system with an EPYC 7513, which does have
> > > > Shadow
> > > > Stack support (I can see 'shstk' in the "Flags" section of
> > > > lscpu
> > > > with
> > > > this series). As far as I understand it, AMD only added Shadow
> > > > Stack
> > > > with Zen 3; my regular AMD test system is Zen 2 (probably
> > > > should
> > > > look at
> > > > procurring a Zen 3 or Zen 4 one at some point).
> > > > 
> > > > I applied this series on top of 6.0 and reverted this change
> > > > then
> > > > booted
> > > > it on that system. After building the selftest (which did
> > > > require
> > > > 'make headers_install' and a small addition to make it build
> > > > beyond
> > > > that, see below), I ran it and this was the result. I am not
> > > > sure
> > > > if
> > > > that is expected or not but the other results seem promising
> > > > for
> > > > dropping this patch.
> > > > 
> > > >     $ ./test_shadow_stack_64
> > > >     [INFO]  new_ssp = 7f8a36c9fff8, *new_ssp = 7f8a36ca0001
> > > >     [INFO]  changing ssp from 7f8a374a0ff0 to 7f8a36c9fff8
> > > >     [INFO]  ssp is now 7f8a36ca0000
> > > >     [OK]    Shadow stack pivot
> > > >     [OK]    Shadow stack faults
> > > >     [INFO]  Corrupting shadow stack
> > > >     [INFO]  Generated shadow stack violation successfully
> > > >     [OK]    Shadow stack violation test
> > > >     [INFO]  Gup read -> shstk access success
> > > >     [INFO]  Gup write -> shstk access success
> > > >     [INFO]  Violation from normal write
> > > >     [INFO]  Gup read -> write access success
> > > >     [INFO]  Violation from normal write
> > > >     [INFO]  Gup write -> write access success
> > > >     [INFO]  Cow gup write -> write access success
> > > >     [OK]    Shadow gup test
> > > >     [INFO]  Violation from shstk access
> > > >     [OK]    mprotect() test
> > > >     [OK]    Userfaultfd test
> > > >     [FAIL]  Alt shadow stack test
> > > 
> > > The selftest is looking OK on my system (Dell PowerEdge R6515 w/
> > > EPYC
> > > 7713). I also just pulled a fresh 6.0 kernel and applied the
> > > series
> > > including the fix Nathan mentions below.
> > > 
> > > $ tools/testing/selftests/x86/test_shadow_stack_64
> > > [INFO]  new_ssp = 7f30cccc5ff8, *new_ssp = 7f30cccc6001
> > > [INFO]  changing ssp from 7f30cd4c6ff0 to 7f30cccc5ff8
> > > [INFO]  ssp is now 7f30cccc6000
> > > [OK]    Shadow stack pivot
> > > [OK]    Shadow stack faults
> > > [INFO]  Corrupting shadow stack
> > > [INFO]  Generated shadow stack violation successfully
> > > [OK]    Shadow stack violation test
> > > [INFO]  Gup read -> shstk access success
> > > [INFO]  Gup write -> shstk access success
> > > [INFO]  Violation from normal write
> > > [INFO]  Gup read -> write access success
> > > [INFO]  Violation from normal write
> > > [INFO]  Gup write -> write access success
> > > [INFO]  Cow gup write -> write access success
> > > [OK]    Shadow gup test
> > > [INFO]  Violation from shstk access
> > > [OK]    mprotect() test
> > > [OK]    Userfaultfd test
> > > [OK]    Alt shadow stack test.
> > 
> > Thanks for the testing. Based on the test, I wonder if this could
> > be a
> > SW bug. Nathan, could I send you a tweaked test with some more
> > debug
> > information?
> 
> Yes, more than happy to help you look into this further!

Indeed this was a SW bug and had nothing to do with the CPU model. The
altshstk selftest was not fully initializing the stack_t struct, and
getting lucky on some compilers. Thanks to Nathan for helping me debug
it.
John Allen Nov. 3, 2022, 5:39 p.m. UTC | #12
On 10/4/22 6:24 PM, Edgecombe, Rick P wrote:
> On Tue, 2022-10-04 at 14:17 -0700, H. Peter Anvin wrote:
>> On October 4, 2022 1:50:20 PM PDT, Nathan Chancellor <
>> nathan@kernel.org> wrote:
>>> On Tue, Oct 04, 2022 at 08:34:54PM +0000, Edgecombe, Rick P wrote:
>>>> On Tue, 2022-10-04 at 14:43 -0500, John Allen wrote:
>>>>> On 10/4/22 10:47 AM, Nathan Chancellor wrote:
>>>>>> Hi Kees,
>>>>>>
>>>>>> On Mon, Oct 03, 2022 at 09:54:26PM -0700, Kees Cook wrote:
>>>>>>> On Mon, Oct 03, 2022 at 05:09:04PM -0700, Dave Hansen
>>>>>>> wrote:
>>>>>>>> On 10/3/22 16:57, Kees Cook wrote:
>>>>>>>>> On Thu, Sep 29, 2022 at 03:29:30PM -0700, Rick
>>>>>>>>> Edgecombe
>>>>>>>>> wrote:
>>>>>>>>>> Shadow stack is supported on newer AMD processors,
>>>>>>>>>> but the
>>>>>>>>>> kernel
>>>>>>>>>> implementation has not been tested on them. Prevent
>>>>>>>>>> basic
>>>>>>>>>> issues from
>>>>>>>>>> showing up for normal users by disabling shadow stack
>>>>>>>>>> on
>>>>>>>>>> all CPUs except
>>>>>>>>>> Intel until it has been tested. At which point the
>>>>>>>>>> limitation should be
>>>>>>>>>> removed.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Rick Edgecombe <
>>>>>>>>>> rick.p.edgecombe@intel.com>
>>>>>>>>>
>>>>>>>>> So running the selftests on an AMD system is sufficient
>>>>>>>>> to
>>>>>>>>> drop this
>>>>>>>>> patch?
>>>>>>>>
>>>>>>>> Yes, that's enough.
>>>>>>>>
>>>>>>>> I _thought_ the AMD folks provided some tested-by's at
>>>>>>>> some
>>>>>>>> point in the
>>>>>>>> past.  But, maybe I'm confusing this for one of the other
>>>>>>>> shared
>>>>>>>> features.  Either way, I'm sure no tested-by's were
>>>>>>>> dropped on
>>>>>>>> purpose.
>>>>>>>>
>>>>>>>> I'm sure Rick is eager to trim down his series and this
>>>>>>>> would
>>>>>>>> be a great
>>>>>>>> patch to drop.  Does anyone want to make that easy for
>>>>>>>> Rick?
>>>>>>>>
>>>>>>>> <hint> <hint>
>>>>>>>
>>>>>>> Hey Gustavo, Nathan, or Nick! I know y'all have some fancy
>>>>>>> AMD
>>>>>>> testing
>>>>>>> rigs. Got a moment to spin up this series and run the
>>>>>>> selftests?
>>>>>>> :)
>>>>>>
>>>>>> I do have access to a system with an EPYC 7513, which does
>>>>>> have
>>>>>> Shadow
>>>>>> Stack support (I can see 'shstk' in the "Flags" section of
>>>>>> lscpu
>>>>>> with
>>>>>> this series). As far as I understand it, AMD only added
>>>>>> Shadow
>>>>>> Stack
>>>>>> with Zen 3; my regular AMD test system is Zen 2 (probably
>>>>>> should
>>>>>> look at
>>>>>> procurring a Zen 3 or Zen 4 one at some point).
>>>>>>
>>>>>> I applied this series on top of 6.0 and reverted this change
>>>>>> then
>>>>>> booted
>>>>>> it on that system. After building the selftest (which did
>>>>>> require
>>>>>> 'make headers_install' and a small addition to make it build
>>>>>> beyond
>>>>>> that, see below), I ran it and this was the result. I am not
>>>>>> sure
>>>>>> if
>>>>>> that is expected or not but the other results seem promising
>>>>>> for
>>>>>> dropping this patch.
>>>>>>
>>>>>>    $ ./test_shadow_stack_64
>>>>>>    [INFO]  new_ssp = 7f8a36c9fff8, *new_ssp = 7f8a36ca0001
>>>>>>    [INFO]  changing ssp from 7f8a374a0ff0 to 7f8a36c9fff8
>>>>>>    [INFO]  ssp is now 7f8a36ca0000
>>>>>>    [OK]    Shadow stack pivot
>>>>>>    [OK]    Shadow stack faults
>>>>>>    [INFO]  Corrupting shadow stack
>>>>>>    [INFO]  Generated shadow stack violation successfully
>>>>>>    [OK]    Shadow stack violation test
>>>>>>    [INFO]  Gup read -> shstk access success
>>>>>>    [INFO]  Gup write -> shstk access success
>>>>>>    [INFO]  Violation from normal write
>>>>>>    [INFO]  Gup read -> write access success
>>>>>>    [INFO]  Violation from normal write
>>>>>>    [INFO]  Gup write -> write access success
>>>>>>    [INFO]  Cow gup write -> write access success
>>>>>>    [OK]    Shadow gup test
>>>>>>    [INFO]  Violation from shstk access
>>>>>>    [OK]    mprotect() test
>>>>>>    [OK]    Userfaultfd test
>>>>>>    [FAIL]  Alt shadow stack test
>>>>>
>>>>> The selftest is looking OK on my system (Dell PowerEdge R6515
>>>>> w/ EPYC
>>>>> 7713). I also just pulled a fresh 6.0 kernel and applied the
>>>>> series
>>>>> including the fix Nathan mentions below.
>>>>>
>>>>> $ tools/testing/selftests/x86/test_shadow_stack_64
>>>>> [INFO]  new_ssp = 7f30cccc5ff8, *new_ssp = 7f30cccc6001
>>>>> [INFO]  changing ssp from 7f30cd4c6ff0 to 7f30cccc5ff8
>>>>> [INFO]  ssp is now 7f30cccc6000
>>>>> [OK]    Shadow stack pivot
>>>>> [OK]    Shadow stack faults
>>>>> [INFO]  Corrupting shadow stack
>>>>> [INFO]  Generated shadow stack violation successfully
>>>>> [OK]    Shadow stack violation test
>>>>> [INFO]  Gup read -> shstk access success
>>>>> [INFO]  Gup write -> shstk access success
>>>>> [INFO]  Violation from normal write
>>>>> [INFO]  Gup read -> write access success
>>>>> [INFO]  Violation from normal write
>>>>> [INFO]  Gup write -> write access success
>>>>> [INFO]  Cow gup write -> write access success
>>>>> [OK]    Shadow gup test
>>>>> [INFO]  Violation from shstk access
>>>>> [OK]    mprotect() test
>>>>> [OK]    Userfaultfd test
>>>>> [OK]    Alt shadow stack test.
>>>>
>>>> Thanks for the testing. Based on the test, I wonder if this could
>>>> be a
>>>> SW bug. Nathan, could I send you a tweaked test with some more
>>>> debug
>>>> information?
>>>
>>> Yes, more than happy to help you look into this further!
>>>
>>>> John, are we sure AMD and Intel systems behave the same with
>>>> respect to
>>>> CPUs not creating Dirty=1,Write=0 PTEs in rare situations? Or any
>>>> other
>>>> CET related differences we should hash out? Otherwise I'll drop
>>>> the
>>>> patch for the next version. (and assuming the issue Nathan hit
>>>> doesn't
>>>> turn up anything HW related).
>>
>> I have to admit to being a bit confused here... in general, we trust
>> CPUID bits unless they are *known* to be wrong, in which case we
>> blacklist them.
>>
>> If AMD advertises the feature but it doesn't work or they didn't
>> validate it, that would be a (serious!) bug on their part that we can
>> address by blacklisting, but they should also fix with a
>> microcode/BIOS patch.
>>
>> What am I missing?
> 
> I have not read anything about the AMD implementation except hearing
> that it is supported. But there are some microarchitectual-like aspects
> to this CET Linux implementation, around requiring CPUs to not create
> Dirty=1,Write=0 PTEs in some cases, where they did in the past. In
> another thread Jann asked how the IOMMU works with respect to this edge
> case and I'm currently trying to chase down that answer for even Intel
> HW. So I just wanted to double check that we expect that everything
> should be the same. In either case we still have time to iron things
> out before anything gets upstream.

Hi Rick,

Sorry for the delayed reply. After asking around, I think you can safely
assume that AMD will not create Dirty=1,Write=0 PTEs in rare
circumstances and shadow stack should behave the same as Intel in that
regard.

Thanks,
John
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index d7415bb556b2..f7cacc5698d5 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -606,6 +606,14 @@  static __always_inline void setup_cet(struct cpuinfo_x86 *c)
 	if (!kernel_ibt && !user_shstk)
 		return;
 
+	/*
+	 * Shadow stack is supported on AMD processors, but has not been
+	 * tested. Only support it on Intel processors until this is done.
+	 * At which point, this vendor check should be removed.
+	 */
+	if (c->x86_vendor != X86_VENDOR_INTEL)
+		setup_clear_cpu_cap(X86_FEATURE_SHSTK);
+
 	if (kernel_ibt)
 		msr = CET_ENDBR_EN;