diff mbox series

[1/3] arm64: compat: Avoid sending SIGILL for unallocated syscall numbers

Message ID 1546539240-20647-2-git-send-email-will.deacon@arm.com
State New
Headers show
Series [1/3] arm64: compat: Avoid sending SIGILL for unallocated syscall numbers | expand

Commit Message

Will Deacon Jan. 3, 2019, 6:13 p.m. UTC 
The ARM Linux kernel handles the EABI syscall numbers as follows:

  0           - NR_SYSCALLS-1	: Invoke syscall via syscall table
  NR_SYSCALLS - 0xeffff		: -ENOSYS (to be allocated in future)
  0xf0000     - 0xf07ff		: Private syscall or -ENOSYS if not allocated
  > 0xf07ff			: SIGILL


Our compat code gets this wrong and ends up sending SIGILL in response
to all syscalls greater than NR_SYSCALLS which have a value greater
than 0x7ff in the bottom 16 bits.

Fix this by defining the end of the ARM private syscall region and
checking the syscall number against that directly. Update the comment
while we're at it.

Cc: <stable@vger.kernel.org>
Cc: Dave Martin <Dave.Martin@arm.com>
Reported-by: Pi-Hsun Shih <pihsun@chromium.org>
Signed-off-by: Will Deacon <will.deacon@arm.com>

---
 arch/arm64/include/asm/unistd.h | 5 +++--
 arch/arm64/kernel/sys_compat.c  | 4 ++--
 2 files changed, 5 insertions(+), 4 deletions(-)

-- 
2.1.4

Comments

Dave Martin Jan. 4, 2019, 12:54 p.m. UTC | #1 
On Thu, Jan 03, 2019 at 06:13:58PM +0000, Will Deacon wrote:
> The ARM Linux kernel handles the EABI syscall numbers as follows:

> 

>   0           - NR_SYSCALLS-1	: Invoke syscall via syscall table

>   NR_SYSCALLS - 0xeffff		: -ENOSYS (to be allocated in future)

>   0xf0000     - 0xf07ff		: Private syscall or -ENOSYS if not allocated

>   > 0xf07ff			: SIGILL

> 

> Our compat code gets this wrong and ends up sending SIGILL in response

> to all syscalls greater than NR_SYSCALLS which have a value greater

> than 0x7ff in the bottom 16 bits.

> 

> Fix this by defining the end of the ARM private syscall region and

> checking the syscall number against that directly. Update the comment

> while we're at it.

> 

> Cc: <stable@vger.kernel.org>

> Cc: Dave Martin <Dave.Martin@arm.com>

> Reported-by: Pi-Hsun Shih <pihsun@chromium.org>

> Signed-off-by: Will Deacon <will.deacon@arm.com>

> ---

>  arch/arm64/include/asm/unistd.h | 5 +++--

>  arch/arm64/kernel/sys_compat.c  | 4 ++--

>  2 files changed, 5 insertions(+), 4 deletions(-)

> 

> diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h

> index b13ca091f833..be66a54ee3a1 100644

> --- a/arch/arm64/include/asm/unistd.h

> +++ b/arch/arm64/include/asm/unistd.h

> @@ -40,8 +40,9 @@

>   * The following SVCs are ARM private.

>   */

>  #define __ARM_NR_COMPAT_BASE		0x0f0000

> -#define __ARM_NR_compat_cacheflush	(__ARM_NR_COMPAT_BASE+2)

> -#define __ARM_NR_compat_set_tls		(__ARM_NR_COMPAT_BASE+5)

> +#define __ARM_NR_compat_cacheflush	(__ARM_NR_COMPAT_BASE + 2)

> +#define __ARM_NR_compat_set_tls		(__ARM_NR_COMPAT_BASE + 5)

> +#define __ARM_NR_compat_syscall_end	(__ARM_NR_COMPAT_BASE + 0x800)


Nit: there is no compat_syscall_end().  Can we make this #define upper
case, like __ARM_NR_COMPAT_BASE, since a symbolic bound, not a syscall
number?

It would be nice to have this for arch/arm too rather than the current
magic numbers.

>  #define __NR_compat_syscalls		399

>  #endif

> diff --git a/arch/arm64/kernel/sys_compat.c b/arch/arm64/kernel/sys_compat.c

> index 32653d156747..5972b7533fa0 100644

> --- a/arch/arm64/kernel/sys_compat.c

> +++ b/arch/arm64/kernel/sys_compat.c

> @@ -102,12 +102,12 @@ long compat_arm_syscall(struct pt_regs *regs)

>  

>  	default:

>  		/*

> -		 * Calls 9f00xx..9f07ff are defined to return -ENOSYS

> +		 * Calls 0xf0xxx..0xf07ff are defined to return -ENOSYS

>  		 * if not implemented, rather than raising SIGILL. This

>  		 * way the calling program can gracefully determine whether

>  		 * a feature is supported.

>  		 */

> -		if ((no & 0xffff) <= 0x7ff)

> +		if (no < __ARM_NR_compat_syscall_end)


arm_syscall() in arch/arm is not responsible for handling syscalls less
__ARM_NR_BASE; the code in entry-common.S redirects those directly to
sys_ni_syscall() instead.

Given how fiddly this is I think it's preferable if we keep to the
arch/arm code structure as much as possible, and call this function only
when no >= __ARM_NR_COMPAT_BASE?

Regular (possibly unallocated) Linux syscalls can be more naturally
handled in do_ni_syscall() instead IMHO, mirroring the entry-common.S
code that it replaces.

Cheers
---Dave
Will Deacon Jan. 4, 2019, 1:47 p.m. UTC | #2 
On Fri, Jan 04, 2019 at 12:54:26PM +0000, Dave Martin wrote:
> On Thu, Jan 03, 2019 at 06:13:58PM +0000, Will Deacon wrote:

> > The ARM Linux kernel handles the EABI syscall numbers as follows:

> > 

> >   0           - NR_SYSCALLS-1	: Invoke syscall via syscall table

> >   NR_SYSCALLS - 0xeffff		: -ENOSYS (to be allocated in future)

> >   0xf0000     - 0xf07ff		: Private syscall or -ENOSYS if not allocated

> >   > 0xf07ff			: SIGILL

> > 

> > Our compat code gets this wrong and ends up sending SIGILL in response

> > to all syscalls greater than NR_SYSCALLS which have a value greater

> > than 0x7ff in the bottom 16 bits.

> > 

> > Fix this by defining the end of the ARM private syscall region and

> > checking the syscall number against that directly. Update the comment

> > while we're at it.

> > 

> > Cc: <stable@vger.kernel.org>

> > Cc: Dave Martin <Dave.Martin@arm.com>

> > Reported-by: Pi-Hsun Shih <pihsun@chromium.org>

> > Signed-off-by: Will Deacon <will.deacon@arm.com>

> > ---

> >  arch/arm64/include/asm/unistd.h | 5 +++--

> >  arch/arm64/kernel/sys_compat.c  | 4 ++--

> >  2 files changed, 5 insertions(+), 4 deletions(-)

> > 

> > diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h

> > index b13ca091f833..be66a54ee3a1 100644

> > --- a/arch/arm64/include/asm/unistd.h

> > +++ b/arch/arm64/include/asm/unistd.h

> > @@ -40,8 +40,9 @@

> >   * The following SVCs are ARM private.

> >   */

> >  #define __ARM_NR_COMPAT_BASE		0x0f0000

> > -#define __ARM_NR_compat_cacheflush	(__ARM_NR_COMPAT_BASE+2)

> > -#define __ARM_NR_compat_set_tls		(__ARM_NR_COMPAT_BASE+5)

> > +#define __ARM_NR_compat_cacheflush	(__ARM_NR_COMPAT_BASE + 2)

> > +#define __ARM_NR_compat_set_tls		(__ARM_NR_COMPAT_BASE + 5)

> > +#define __ARM_NR_compat_syscall_end	(__ARM_NR_COMPAT_BASE + 0x800)

> 

> Nit: there is no compat_syscall_end().  Can we make this #define upper

> case, like __ARM_NR_COMPAT_BASE, since a symbolic bound, not a syscall

> number?


That's fair; I'll rename it to __ARM_NR_COMPAT_END.

> >  #define __NR_compat_syscalls		399

> >  #endif

> > diff --git a/arch/arm64/kernel/sys_compat.c b/arch/arm64/kernel/sys_compat.c

> > index 32653d156747..5972b7533fa0 100644

> > --- a/arch/arm64/kernel/sys_compat.c

> > +++ b/arch/arm64/kernel/sys_compat.c

> > @@ -102,12 +102,12 @@ long compat_arm_syscall(struct pt_regs *regs)

> >  

> >  	default:

> >  		/*

> > -		 * Calls 9f00xx..9f07ff are defined to return -ENOSYS

> > +		 * Calls 0xf0xxx..0xf07ff are defined to return -ENOSYS

> >  		 * if not implemented, rather than raising SIGILL. This

> >  		 * way the calling program can gracefully determine whether

> >  		 * a feature is supported.

> >  		 */

> > -		if ((no & 0xffff) <= 0x7ff)

> > +		if (no < __ARM_NR_compat_syscall_end)

> 

> arm_syscall() in arch/arm is not responsible for handling syscalls less

> __ARM_NR_BASE; the code in entry-common.S redirects those directly to

> sys_ni_syscall() instead.

> 

> Given how fiddly this is I think it's preferable if we keep to the

> arch/arm code structure as much as possible, and call this function only

> when no >= __ARM_NR_COMPAT_BASE?


I don't think we should be using the arm code structure as a template here.
Although we're providing the same interface, we don't have to worry about
things like OABI or ARM vs THUMB entry code. We're also in the process of
moving more of this out of assembly and into C, so I'd rather keep the
compat code as self-contained as possible.

Even if we did modify the caller so that we only call compat_arm_syscall()
for numbers >= __ARM_NR_COMPAT_BASE, we'd still need the check above to
handle numbers >= __ARM_NR_COMPAT_END, so I don't think we gain anything.

Will
Dave Martin Jan. 4, 2019, 2:15 p.m. UTC | #3 
On Fri, Jan 04, 2019 at 01:47:49PM +0000, Will Deacon wrote:
> On Fri, Jan 04, 2019 at 12:54:26PM +0000, Dave Martin wrote:

> > On Thu, Jan 03, 2019 at 06:13:58PM +0000, Will Deacon wrote:

> > > The ARM Linux kernel handles the EABI syscall numbers as follows:

> > > 

> > >   0           - NR_SYSCALLS-1	: Invoke syscall via syscall table

> > >   NR_SYSCALLS - 0xeffff		: -ENOSYS (to be allocated in future)

> > >   0xf0000     - 0xf07ff		: Private syscall or -ENOSYS if not allocated

> > >   > 0xf07ff			: SIGILL

> > > 

> > > Our compat code gets this wrong and ends up sending SIGILL in response

> > > to all syscalls greater than NR_SYSCALLS which have a value greater

> > > than 0x7ff in the bottom 16 bits.

> > > 

> > > Fix this by defining the end of the ARM private syscall region and

> > > checking the syscall number against that directly. Update the comment

> > > while we're at it.

> > > 

> > > Cc: <stable@vger.kernel.org>

> > > Cc: Dave Martin <Dave.Martin@arm.com>

> > > Reported-by: Pi-Hsun Shih <pihsun@chromium.org>

> > > Signed-off-by: Will Deacon <will.deacon@arm.com>

> > > ---

> > >  arch/arm64/include/asm/unistd.h | 5 +++--

> > >  arch/arm64/kernel/sys_compat.c  | 4 ++--

> > >  2 files changed, 5 insertions(+), 4 deletions(-)

> > > 

> > > diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h

> > > index b13ca091f833..be66a54ee3a1 100644

> > > --- a/arch/arm64/include/asm/unistd.h

> > > +++ b/arch/arm64/include/asm/unistd.h

> > > @@ -40,8 +40,9 @@

> > >   * The following SVCs are ARM private.

> > >   */

> > >  #define __ARM_NR_COMPAT_BASE		0x0f0000

> > > -#define __ARM_NR_compat_cacheflush	(__ARM_NR_COMPAT_BASE+2)

> > > -#define __ARM_NR_compat_set_tls		(__ARM_NR_COMPAT_BASE+5)

> > > +#define __ARM_NR_compat_cacheflush	(__ARM_NR_COMPAT_BASE + 2)

> > > +#define __ARM_NR_compat_set_tls		(__ARM_NR_COMPAT_BASE + 5)

> > > +#define __ARM_NR_compat_syscall_end	(__ARM_NR_COMPAT_BASE + 0x800)

> > 

> > Nit: there is no compat_syscall_end().  Can we make this #define upper

> > case, like __ARM_NR_COMPAT_BASE, since a symbolic bound, not a syscall

> > number?

> 

> That's fair; I'll rename it to __ARM_NR_COMPAT_END.


Thanks

> 

> > >  #define __NR_compat_syscalls		399

> > >  #endif

> > > diff --git a/arch/arm64/kernel/sys_compat.c b/arch/arm64/kernel/sys_compat.c

> > > index 32653d156747..5972b7533fa0 100644

> > > --- a/arch/arm64/kernel/sys_compat.c

> > > +++ b/arch/arm64/kernel/sys_compat.c

> > > @@ -102,12 +102,12 @@ long compat_arm_syscall(struct pt_regs *regs)

> > >  

> > >  	default:

> > >  		/*

> > > -		 * Calls 9f00xx..9f07ff are defined to return -ENOSYS

> > > +		 * Calls 0xf0xxx..0xf07ff are defined to return -ENOSYS

> > >  		 * if not implemented, rather than raising SIGILL. This

> > >  		 * way the calling program can gracefully determine whether

> > >  		 * a feature is supported.

> > >  		 */

> > > -		if ((no & 0xffff) <= 0x7ff)

> > > +		if (no < __ARM_NR_compat_syscall_end)

> > 

> > arm_syscall() in arch/arm is not responsible for handling syscalls less

> > __ARM_NR_BASE; the code in entry-common.S redirects those directly to

> > sys_ni_syscall() instead.

> > 

> > Given how fiddly this is I think it's preferable if we keep to the

> > arch/arm code structure as much as possible, and call this function only

> > when no >= __ARM_NR_COMPAT_BASE?

> 

> I don't think we should be using the arm code structure as a template here.

> Although we're providing the same interface, we don't have to worry about

> things like OABI or ARM vs THUMB entry code. We're also in the process of

> moving more of this out of assembly and into C, so I'd rather keep the

> compat code as self-contained as possible.

> 

> Even if we did modify the caller so that we only call compat_arm_syscall()

> for numbers >= __ARM_NR_COMPAT_BASE, we'd still need the check above to

> handle numbers >= __ARM_NR_COMPAT_END, so I don't think we gain anything.


Well, I can live with it either way.

Cross-referencing the arm64 and arm trees for this functionality is
challenging, and will tend to get harder as the code diverges...  but
hopefully it's gone about as far as it's going to go by this point.

Cheers
---Dave
diff mbox series

Patch

diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h
index b13ca091f833..be66a54ee3a1 100644
--- a/arch/arm64/include/asm/unistd.h
+++ b/arch/arm64/include/asm/unistd.h
@@ -40,8 +40,9 @@ 
  * The following SVCs are ARM private.
  */
 #define __ARM_NR_COMPAT_BASE		0x0f0000
-#define __ARM_NR_compat_cacheflush	(__ARM_NR_COMPAT_BASE+2)
-#define __ARM_NR_compat_set_tls		(__ARM_NR_COMPAT_BASE+5)
+#define __ARM_NR_compat_cacheflush	(__ARM_NR_COMPAT_BASE + 2)
+#define __ARM_NR_compat_set_tls		(__ARM_NR_COMPAT_BASE + 5)
+#define __ARM_NR_compat_syscall_end	(__ARM_NR_COMPAT_BASE + 0x800)
 
 #define __NR_compat_syscalls		399
 #endif
diff --git a/arch/arm64/kernel/sys_compat.c b/arch/arm64/kernel/sys_compat.c
index 32653d156747..5972b7533fa0 100644
--- a/arch/arm64/kernel/sys_compat.c
+++ b/arch/arm64/kernel/sys_compat.c
@@ -102,12 +102,12 @@  long compat_arm_syscall(struct pt_regs *regs)
 
 	default:
 		/*
-		 * Calls 9f00xx..9f07ff are defined to return -ENOSYS
+		 * Calls 0xf0xxx..0xf07ff are defined to return -ENOSYS
 		 * if not implemented, rather than raising SIGILL. This
 		 * way the calling program can gracefully determine whether
 		 * a feature is supported.
 		 */
-		if ((no & 0xffff) <= 0x7ff)
+		if (no < __ARM_NR_compat_syscall_end)
 			return -ENOSYS;
 		break;
 	}