[PATCH] selftests/x86: Update map_shadow_stack syscall nr

[PATCH] selftests/x86: Update map_shadow_stack syscall nr

[Rick Edgecombe]

Shadow stack's selftest utilizes the map_shadow_stack syscall. The
syscall is new with the feature, but the selftests cannot automatically
find the headers for the kernel source tree they are located in. This
resulted in the shadow stack test failing to build until the brand new
headers were installed.

To avoid this, a copy of the new uapi defines needed by the test were
included in the selftest (see link for discussion). When shadow stack was
merged the syscall number was changed, but the copy in the selftest was
not updated.

So update the copy of the syscall number define used when the required
headers are not installed, to have the final syscall number from the
merge.

Link: https://lore.kernel.org/lkml/Y%2FijdXoTAATt0+Ct@zn.tnic/
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
 tools/testing/selftests/x86/test_shadow_stack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/x86/test_shadow_stack.c b/tools/testing/selftests/x86/test_shadow_stack.c
index 2188968674cb..757e6527f67e 100644
--- a/tools/testing/selftests/x86/test_shadow_stack.c
+++ b/tools/testing/selftests/x86/test_shadow_stack.c
@@ -40,7 +40,7 @@
  * without building the headers.
  */
 #ifndef __NR_map_shadow_stack
-#define __NR_map_shadow_stack	452
+#define __NR_map_shadow_stack	453
 
 #define SHADOW_STACK_SET_TOKEN	(1ULL << 0)
 
-- 
2.34.1

Re: [PATCH] selftests/x86: Update map_shadow_stack syscall nr

[Sohil Mehta]

Hi Rick,

On 9/1/2023 11:16 AM, Rick Edgecombe wrote:
> Shadow stack's selftest utilizes the map_shadow_stack syscall. The
> syscall is new with the feature, but the selftests cannot automatically
> find the headers for the kernel source tree they are located in. This
> resulted in the shadow stack test failing to build until the brand new
> headers were installed.
> 

I am wondering why a definition for __NR_map_shadow_stack is missing in
include/uapi/asm-generic/unistd.h?

Wouldn't this mean that even if someone were to install the headers they
still wouldn't get the syscall number definition. Am I missing something?

> To avoid this, a copy of the new uapi defines needed by the test were
> included in the selftest (see link for discussion). When shadow stack was
> merged the syscall number was changed, but the copy in the selftest was
> not updated.
> 
> So update the copy of the syscall number define used when the required
> headers are not installed, to have the final syscall number from the
> merge.
> 

How about adding a fixes tag to make it a tiny bit easier for someone
who backports the shstk series?

Fixes: 81f30337ef4f ("selftests/x86: Add shadow stack test")

> Link: https://lore.kernel.org/lkml/Y%2FijdXoTAATt0+Ct@zn.tnic/
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> ---
>  tools/testing/selftests/x86/test_shadow_stack.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/x86/test_shadow_stack.c b/tools/testing/selftests/x86/test_shadow_stack.c
> index 2188968674cb..757e6527f67e 100644
> --- a/tools/testing/selftests/x86/test_shadow_stack.c
> +++ b/tools/testing/selftests/x86/test_shadow_stack.c
> @@ -40,7 +40,7 @@
>   * without building the headers.
>   */
>  #ifndef __NR_map_shadow_stack
> -#define __NR_map_shadow_stack	452
> +#define __NR_map_shadow_stack	453
>  
>  #define SHADOW_STACK_SET_TOKEN	(1ULL << 0)
>  

Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>

Apart from this patch, I think we also need something like commit
78252deb023c ("arch: Register fchmodat2, usually as syscall 452") to
reserve the 453 syscall number for the rest of the architectures.

Should I send one out if you don't have something prepared already?

Thanks,
Sohil

Re: [PATCH] selftests/x86: Update map_shadow_stack syscall nr

[Edgecombe, Rick P]

+Mark, regarding matching map_shadow_stack syscall numbers.

On Fri, 2023-09-01 at 12:33 -0700, Sohil Mehta wrote:
> Hi Rick,
> 
> On 9/1/2023 11:16 AM, Rick Edgecombe wrote:
> > Shadow stack's selftest utilizes the map_shadow_stack syscall. The
> > syscall is new with the feature, but the selftests cannot
> > automatically
> > find the headers for the kernel source tree they are located in.
> > This
> > resulted in the shadow stack test failing to build until the brand
> > new
> > headers were installed.
> > 
> 
> I am wondering why a definition for __NR_map_shadow_stack is missing
> in
> include/uapi/asm-generic/unistd.h?
> 
> Wouldn't this mean that even if someone were to install the headers
> they
> still wouldn't get the syscall number definition. Am I missing
> something?

There is some autogeneration that happens from the .tbl files.

> 
> > To avoid this, a copy of the new uapi defines needed by the test
> > were
> > included in the selftest (see link for discussion). When shadow
> > stack was
> > merged the syscall number was changed, but the copy in the selftest
> > was
> > not updated.
> > 
> > So update the copy of the syscall number define used when the
> > required
> > headers are not installed, to have the final syscall number from
> > the
> > merge.
> > 
> 
> How about adding a fixes tag to make it a tiny bit easier for someone
> who backports the shstk series?
> 
> Fixes: 81f30337ef4f ("selftests/x86: Add shadow stack test")

I wasn't sure if the proper tag was that commit or the merge one. If
the selftest commit is blamed, and in a backport the original commits
are grabbed plus any that blame them, then the syscall numbers would
end up mismatched.

So I think maybe?
Fixes: df57721f9a63 ("Merge tag 'x86_shstk_for_6.6-rc1' of [...]")

But maybe also this whole duplicate defines thing is questionable.

> 
> > Link: https://lore.kernel.org/lkml/Y%2FijdXoTAATt0+Ct@zn.tnic/
> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> > ---
> >  tools/testing/selftests/x86/test_shadow_stack.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/testing/selftests/x86/test_shadow_stack.c
> > b/tools/testing/selftests/x86/test_shadow_stack.c
> > index 2188968674cb..757e6527f67e 100644
> > --- a/tools/testing/selftests/x86/test_shadow_stack.c
> > +++ b/tools/testing/selftests/x86/test_shadow_stack.c
> > @@ -40,7 +40,7 @@
> >   * without building the headers.
> >   */
> >  #ifndef __NR_map_shadow_stack
> > -#define __NR_map_shadow_stack  452
> > +#define __NR_map_shadow_stack  453
> >  
> >  #define SHADOW_STACK_SET_TOKEN (1ULL << 0)
> >  
> 
> Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>

Thanks!

> 
> Apart from this patch, I think we also need something like commit
> 78252deb023c ("arch: Register fchmodat2, usually as syscall 452") to
> reserve the 453 syscall number for the rest of the architectures.
> 
> Should I send one out if you don't have something prepared already?

Originally there were no other shadow stack features, and so it was
maybe going to be an x86-only syscall. I followed in the footsteps of 
the secret mem syscall, but that one seems to have grown some similar
reservation comments since then. It probably makes more sense for that
one though, since it's sort of a generic functionality. An analogous
x86 specific syscall would maybe be modify_ldt, which doesn't really
have reservations.

But now we also have arm that plans to use it. So maybe it is worth
trying to match syscall numbers? I could imagine scenarios where it
could be useful. And I guess there is also the scenario where a generic
type syscall is added, but only implemented on non-shadow stack
architectures. So then when it makes it's way around, it can't match. I
hadn't thought about it before, so just thinking it through...

So sounds reasonable to me. I don't have anything prepped.

> 
> Thanks,
> Sohil
> 
> 

Re: [PATCH] selftests/x86: Update map_shadow_stack syscall nr

[Sohil Mehta]

On 9/1/2023 1:35 PM, Edgecombe, Rick P wrote:
> On Fri, 2023-09-01 at 12:33 -0700, Sohil Mehta wrote:

>> I am wondering why a definition for __NR_map_shadow_stack is
>> missing in include/uapi/asm-generic/unistd.h?
>> 
>> Wouldn't this mean that even if someone were to install the
>> headers they still wouldn't get the syscall number definition. Am I
>> missing something?
> 
> There is some autogeneration that happens from the .tbl files.

Hmm.. I wasn't aware of this auto-generation. The last few system calls
additions have all updated the unistd.h file manually to add the
__NR_foo defines. So I assumed that's what gets included in the header
packages and eventually gets used by userspace in some form. But maybe
x86 headers might be built differently. I need to educate myself on this.

> But maybe also this whole duplicate defines thing is questionable.
> 

Yeah, having 2 defines is a bit annoying but as mentioned in the link it
makes it easier for someone to run selftests so it's probably worth the
effort. Also since syscall numbers typically never change it is just a
one time thing and not a big maintenance burden :)

FWIW, I see these syscall nr defines all across the selftests (io_uring,
powerpc/pkeys, seccomp, etc).

> 
>>
>> Apart from this patch, I think we also need something like commit
>> 78252deb023c ("arch: Register fchmodat2, usually as syscall 452") to
>> reserve the 453 syscall number for the rest of the architectures.
>>
>> Should I send one out if you don't have something prepared already?
> 
> Originally there were no other shadow stack features, and so it was
> maybe going to be an x86-only syscall. I followed in the footsteps of 
> the secret mem syscall, but that one seems to have grown some similar
> reservation comments since then. It probably makes more sense for that
> one though, since it's sort of a generic functionality. An analogous
> x86 specific syscall would maybe be modify_ldt, which doesn't really
> have reservations.
> 
> But now we also have arm that plans to use it. So maybe it is worth
> trying to match syscall numbers? I could imagine scenarios where it
> could be useful. And I guess there is also the scenario where a generic
> type syscall is added, but only implemented on non-shadow stack
> architectures. So then when it makes it's way around, it can't match. I
> hadn't thought about it before, so just thinking it through...
> 

My suggestion is originating from the belief that at somepoint the
community decided that all *new* system call numbers would be the
consistent across architectures (except alpha). So that would mean
syscall number 453 has to be reserved on others even if it is an
x86-only syscall.

If we don't do this, and let say a generic sys_foo comes along which
uses the next available syscall number 453 on other archs, it would lead
to an inconsistency because 453 it is already used up on x86.

My memory of this is a bit hazy from my implementation of User
Interrupts more than a couple of years back. Also, I couldn't find any
handy documentation to support my belief. I'll try to dig more.

Sohil

Re: [PATCH] selftests/x86: Update map_shadow_stack syscall nr

[Edgecombe, Rick P]

On Fri, 2023-09-01 at 15:19 -0700, Sohil Mehta wrote:
> My suggestion is originating from the belief that at somepoint the
> community decided that all *new* system call numbers would be the
> consistent across architectures (except alpha). So that would mean
> syscall number 453 has to be reserved on others even if it is an
> x86-only syscall.
> 
> If we don't do this, and let say a generic sys_foo comes along which
> uses the next available syscall number 453 on other archs, it would
> lead
> to an inconsistency because 453 it is already used up on x86.
> 
> My memory of this is a bit hazy from my implementation of User
> Interrupts more than a couple of years back. Also, I couldn't find
> any
> handy documentation to support my belief. I'll try to dig more.

Putting reservations in sounds like a good idea in any case. I take it
you would like to send the patch? Otherwise let me know.

Re: [PATCH] selftests/x86: Update map_shadow_stack syscall nr

[Sohil Mehta]

On 9/1/2023 3:51 PM, Edgecombe, Rick P wrote:
> Putting reservations in sounds like a good idea in any case. I take it
> you would like to send the patch? Otherwise let me know.

Sure, I'll spin something up and send it out next week after running it
through 0day.

[tip: x86/urgent] selftests/x86: Update map_shadow_stack syscall nr

[tip-bot2 for Rick Edgecombe]

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

Commit-ID:     6ea7bb00c1ba180f8bf8320b8d59b532501c5271
Gitweb:        https://git.kernel.org/tip/6ea7bb00c1ba180f8bf8320b8d59b532501c5271
Author:        Rick Edgecombe <rick.p.edgecombe@intel.com>
AuthorDate:    Fri, 01 Sep 2023 11:16:52 -07:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Fri, 01 Sep 2023 23:34:13 +02:00

selftests/x86: Update map_shadow_stack syscall nr

Shadow stack's selftest utilizes the map_shadow_stack syscall. The
syscall is new with the feature, but the selftests cannot automatically
find the headers for the kernel source tree they are located in. This
resulted in the shadow stack test failing to build until the brand new
headers were installed.

To avoid this, a copy of the new uapi defines needed by the test were
included in the selftest (see link for discussion). When shadow stack was
merged the syscall number was changed, but the copy in the selftest was
not updated.

So update the copy of the syscall number define used when the required
headers are not installed, to have the final syscall number from the
merge.

Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Link: https://lore.kernel.org/lkml/Y%2FijdXoTAATt0+Ct@zn.tnic/
Link: https://lore.kernel.org/r/20230901181652.2583861-1-rick.p.edgecombe@intel.com
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Fixes: df57721f9a63 ("Merge tag 'x86_shstk_for_6.6-rc1' of [...]")
---
 tools/testing/selftests/x86/test_shadow_stack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/x86/test_shadow_stack.c b/tools/testing/selftests/x86/test_shadow_stack.c
index 2188968..757e652 100644
--- a/tools/testing/selftests/x86/test_shadow_stack.c
+++ b/tools/testing/selftests/x86/test_shadow_stack.c
@@ -40,7 +40,7 @@
  * without building the headers.
  */
 #ifndef __NR_map_shadow_stack
-#define __NR_map_shadow_stack	452
+#define __NR_map_shadow_stack	453
 
 #define SHADOW_STACK_SET_TOKEN	(1ULL << 0)