diff mbox series

[16/20] arm64: signal32: move ilp32 and aarch32 common code to separated file

Message ID 20170604120009.342-17-ynorov@caviumnetworks.com
State Superseded
Headers show
Series [01/20] compat ABI: use non-compat openat and open_by_handle_at variants | expand

Commit Message

Yury Norov June 4, 2017, noon UTC 
Signed-off-by: Yury Norov <ynorov@caviumnetworks.com>

---
 arch/arm64/include/asm/signal32.h        |   3 +
 arch/arm64/include/asm/signal32_common.h |  27 +++++++
 arch/arm64/kernel/Makefile               |   2 +-
 arch/arm64/kernel/signal32.c             | 107 ------------------------
 arch/arm64/kernel/signal32_common.c      | 135 +++++++++++++++++++++++++++++++
 5 files changed, 166 insertions(+), 108 deletions(-)
 create mode 100644 arch/arm64/include/asm/signal32_common.h
 create mode 100644 arch/arm64/kernel/signal32_common.c

-- 
2.11.0

Comments

James Morse June 19, 2017, 4:16 p.m. UTC | #1 
Hi Yury,

On 04/06/17 13:00, Yury Norov wrote:
> Signed-off-by: Yury Norov <ynorov@caviumnetworks.com>


Can I offer a body for the commit message:
ILP32 needs to mix 32bit struct siginfo and 64bit sigframe for its signal
handlers. Move the existing compat code for copying siginfo to user space and
manipulating signal masks into signal32_common.c so it can be used to deliver
aarch32 and ilp32 signals.


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

> index e68fcce538e1..1c4ede717bd2 100644

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

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

> @@ -13,6 +13,9 @@

>   * You should have received a copy of the GNU General Public License

>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.

>   */

> +

> +#include <asm/signal32_common.h>

> +

>  #ifndef __ASM_SIGNAL32_H

>  #define __ASM_SIGNAL32_H


Nit: This should go inside the guard.


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

> new file mode 100644

> index 000000000000..5bddc25dca12

> --- /dev/null

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

> @@ -0,0 +1,135 @@

[...]
> +#include <linux/compat.h>

> +#include <linux/signal.h>

> +#include <linux/ratelimit.h>


What do you need ratelimit.h for?


> +#include <linux/uaccess.h>

> +

> +#include <asm/esr.h>


I can't see anything using these ESR_ macros in here...


> +#include <asm/fpsimd.h>


This was for the VFP save/restore code, which you didn't move...


> +#include <asm/signal32_common.h>

> +#include <asm/unistd.h>


[...]


> +int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)

[...]
> +	case __SI_FAULT:

> +		err |= __put_user((compat_uptr_t)(unsigned long)from->si_addr,

> +				  &to->si_addr);


This looks tricky. si_addr comes from FAR_EL1 when user-space touches something
it shouldn't. This could be a 64bit value as ilp32 processes can still branch to
64bit addresses in registers and generate loads that cross the invisible 4GB
boundary. Here you truncate the 64bit address.
Obviously this can't happen at all with aarch32, and for C programs its into
undefined-behaviour territory, but it doesn't feel right to pass an address to
user-space that we know is wrong... but we don't have an alternative.

This looks like a class of problem particular to ilp32/x32: 'accessed an address
you can't encode with a signal'. After a quick dig in x86's x32 code, it looks
like they only pass the first 32bits of si_addr too.

One option is to mint a new si_code to go with SIGBUS meaning something like
'address overflowed si_addr'. Alternatively we could just kill tasks that do this.


Thanks,

James
Yury Norov June 20, 2017, 2:16 p.m. UTC | #2 
On Mon, Jun 19, 2017 at 05:16:42PM +0100, James Morse wrote:
> Hi Yury,

> 

> On 04/06/17 13:00, Yury Norov wrote:

> > Signed-off-by: Yury Norov <ynorov@caviumnetworks.com>

> 

> Can I offer a body for the commit message:

> ILP32 needs to mix 32bit struct siginfo and 64bit sigframe for its signal

> handlers. Move the existing compat code for copying siginfo to user space and

> manipulating signal masks into signal32_common.c so it can be used to deliver

> aarch32 and ilp32 signals.


Ok

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

> > index e68fcce538e1..1c4ede717bd2 100644

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

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

> > @@ -13,6 +13,9 @@

> >   * You should have received a copy of the GNU General Public License

> >   * along with this program.  If not, see <http://www.gnu.org/licenses/>.

> >   */

> > +

> > +#include <asm/signal32_common.h>

> > +

> >  #ifndef __ASM_SIGNAL32_H

> >  #define __ASM_SIGNAL32_H

> 

> Nit: This should go inside the guard.

 
Ok, thanks. Will fix this and all below
 
> > diff --git a/arch/arm64/kernel/signal32_common.c b/arch/arm64/kernel/signal32_common.c

> > new file mode 100644

> > index 000000000000..5bddc25dca12

> > --- /dev/null

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

> > @@ -0,0 +1,135 @@

> [...]

> > +#include <linux/compat.h>

> > +#include <linux/signal.h>

> > +#include <linux/ratelimit.h>

> 

> What do you need ratelimit.h for?

> 

> 

> > +#include <linux/uaccess.h>

> > +

> > +#include <asm/esr.h>

> 

> I can't see anything using these ESR_ macros in here...

> 

> 

> > +#include <asm/fpsimd.h>

> 

> This was for the VFP save/restore code, which you didn't move...

> 

> 

> > +#include <asm/signal32_common.h>

> > +#include <asm/unistd.h>

> 

> [...]

> 

> 

> > +int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)

> [...]

> > +	case __SI_FAULT:

> > +		err |= __put_user((compat_uptr_t)(unsigned long)from->si_addr,

> > +				  &to->si_addr);

> 

> This looks tricky. si_addr comes from FAR_EL1 when user-space touches something

> it shouldn't. This could be a 64bit value as ilp32 processes can still branch to

> 64bit addresses in registers and generate loads that cross the invisible 4GB

> boundary. Here you truncate the 64bit address.

> Obviously this can't happen at all with aarch32, and for C programs its into

> undefined-behaviour territory, but it doesn't feel right to pass an address to

> user-space that we know is wrong... but we don't have an alternative.

> 

> This looks like a class of problem particular to ilp32/x32: 'accessed an address

> you can't encode with a signal'. After a quick dig in x86's x32 code, it looks

> like they only pass the first 32bits of si_addr too.

> 

> One option is to mint a new si_code to go with SIGBUS meaning something like

> 'address overflowed si_addr'. Alternatively we could just kill tasks that do this.


New SIGBUS sounds reasonable at the first glance, but I think it should be
discussed widely at first, and the patch that implements it should touch
all arches that may be affected.

Yury
diff mbox series

Patch

diff --git a/arch/arm64/include/asm/signal32.h b/arch/arm64/include/asm/signal32.h
index e68fcce538e1..1c4ede717bd2 100644
--- a/arch/arm64/include/asm/signal32.h
+++ b/arch/arm64/include/asm/signal32.h
@@ -13,6 +13,9 @@ 
  * You should have received a copy of the GNU General Public License
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
+
+#include <asm/signal32_common.h>
+
 #ifndef __ASM_SIGNAL32_H
 #define __ASM_SIGNAL32_H
 
diff --git a/arch/arm64/include/asm/signal32_common.h b/arch/arm64/include/asm/signal32_common.h
new file mode 100644
index 000000000000..36c1ebc07a97
--- /dev/null
+++ b/arch/arm64/include/asm/signal32_common.h
@@ -0,0 +1,27 @@ 
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#ifndef __ASM_SIGNAL32_COMMON_H
+#define __ASM_SIGNAL32_COMMON_H
+
+#ifdef CONFIG_COMPAT
+
+int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from);
+int copy_siginfo_from_user32(siginfo_t *to, compat_siginfo_t __user *from);
+
+int put_sigset_t(compat_sigset_t __user *uset, sigset_t *set);
+int get_sigset_t(sigset_t *set, const compat_sigset_t __user *uset);
+
+#endif /* CONFIG_COMPAT*/
+
+#endif /* __ASM_SIGNAL32_COMMON_H */
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 7e0c48f858f1..1dd659907f34 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -30,7 +30,7 @@  $(obj)/%.stub.o: $(obj)/%.o FORCE
 arm64-obj-$(CONFIG_AARCH32_EL0)		+= sys32.o kuser32.o signal32.o 	\
 					   sys_compat.o entry32.o binfmt_elf32.o
 arm64-obj-$(CONFIG_ARM64_ILP32)		+= binfmt_ilp32.o sys_ilp32.o
-arm64-obj-$(CONFIG_COMPAT)		+= entry32_common.o
+arm64-obj-$(CONFIG_COMPAT)		+= entry32_common.o signal32_common.o
 arm64-obj-$(CONFIG_FUNCTION_TRACER)	+= ftrace.o entry-ftrace.o
 arm64-obj-$(CONFIG_MODULES)		+= arm64ksyms.o module.o
 arm64-obj-$(CONFIG_ARM64_MODULE_PLTS)	+= module-plts.o
diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c
index c747a0fc5d7d..181cc3012bda 100644
--- a/arch/arm64/kernel/signal32.c
+++ b/arch/arm64/kernel/signal32.c
@@ -103,113 +103,6 @@  struct compat_rt_sigframe {
 
 #define _BLOCKABLE (~(sigmask(SIGKILL) | sigmask(SIGSTOP)))
 
-static inline int put_sigset_t(compat_sigset_t __user *uset, sigset_t *set)
-{
-	compat_sigset_t	cset;
-
-	cset.sig[0] = set->sig[0] & 0xffffffffull;
-	cset.sig[1] = set->sig[0] >> 32;
-
-	return copy_to_user(uset, &cset, sizeof(*uset));
-}
-
-static inline int get_sigset_t(sigset_t *set,
-			       const compat_sigset_t __user *uset)
-{
-	compat_sigset_t s32;
-
-	if (copy_from_user(&s32, uset, sizeof(*uset)))
-		return -EFAULT;
-
-	set->sig[0] = s32.sig[0] | (((long)s32.sig[1]) << 32);
-	return 0;
-}
-
-int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
-{
-	int err;
-
-	if (!access_ok(VERIFY_WRITE, to, sizeof(*to)))
-		return -EFAULT;
-
-	/* If you change siginfo_t structure, please be sure
-	 * this code is fixed accordingly.
-	 * It should never copy any pad contained in the structure
-	 * to avoid security leaks, but must copy the generic
-	 * 3 ints plus the relevant union member.
-	 * This routine must convert siginfo from 64bit to 32bit as well
-	 * at the same time.
-	 */
-	err = __put_user(from->si_signo, &to->si_signo);
-	err |= __put_user(from->si_errno, &to->si_errno);
-	err |= __put_user((short)from->si_code, &to->si_code);
-	if (from->si_code < 0)
-		err |= __copy_to_user(&to->_sifields._pad, &from->_sifields._pad,
-				      SI_PAD_SIZE);
-	else switch (from->si_code & __SI_MASK) {
-	case __SI_KILL:
-		err |= __put_user(from->si_pid, &to->si_pid);
-		err |= __put_user(from->si_uid, &to->si_uid);
-		break;
-	case __SI_TIMER:
-		 err |= __put_user(from->si_tid, &to->si_tid);
-		 err |= __put_user(from->si_overrun, &to->si_overrun);
-		 err |= __put_user(from->si_int, &to->si_int);
-		break;
-	case __SI_POLL:
-		err |= __put_user(from->si_band, &to->si_band);
-		err |= __put_user(from->si_fd, &to->si_fd);
-		break;
-	case __SI_FAULT:
-		err |= __put_user((compat_uptr_t)(unsigned long)from->si_addr,
-				  &to->si_addr);
-#ifdef BUS_MCEERR_AO
-		/*
-		 * Other callers might not initialize the si_lsb field,
-		 * so check explicitly for the right codes here.
-		 */
-		if (from->si_signo == SIGBUS &&
-		    (from->si_code == BUS_MCEERR_AR || from->si_code == BUS_MCEERR_AO))
-			err |= __put_user(from->si_addr_lsb, &to->si_addr_lsb);
-#endif
-		break;
-	case __SI_CHLD:
-		err |= __put_user(from->si_pid, &to->si_pid);
-		err |= __put_user(from->si_uid, &to->si_uid);
-		err |= __put_user(from->si_status, &to->si_status);
-		err |= __put_user(from->si_utime, &to->si_utime);
-		err |= __put_user(from->si_stime, &to->si_stime);
-		break;
-	case __SI_RT: /* This is not generated by the kernel as of now. */
-	case __SI_MESGQ: /* But this is */
-		err |= __put_user(from->si_pid, &to->si_pid);
-		err |= __put_user(from->si_uid, &to->si_uid);
-		err |= __put_user(from->si_int, &to->si_int);
-		break;
-	case __SI_SYS:
-		err |= __put_user((compat_uptr_t)(unsigned long)
-				from->si_call_addr, &to->si_call_addr);
-		err |= __put_user(from->si_syscall, &to->si_syscall);
-		err |= __put_user(from->si_arch, &to->si_arch);
-		break;
-	default: /* this is just in case for now ... */
-		err |= __put_user(from->si_pid, &to->si_pid);
-		err |= __put_user(from->si_uid, &to->si_uid);
-		break;
-	}
-	return err;
-}
-
-int copy_siginfo_from_user32(siginfo_t *to, compat_siginfo_t __user *from)
-{
-	if (copy_from_user(to, from, __ARCH_SI_PREAMBLE_SIZE) ||
-	    copy_from_user(to->_sifields._pad,
-			   from->_sifields._pad, SI_PAD_SIZE))
-		return -EFAULT;
-
-	return 0;
-}
-
 /*
  * VFP save/restore code.
  *
diff --git a/arch/arm64/kernel/signal32_common.c b/arch/arm64/kernel/signal32_common.c
new file mode 100644
index 000000000000..5bddc25dca12
--- /dev/null
+++ b/arch/arm64/kernel/signal32_common.c
@@ -0,0 +1,135 @@ 
+/*
+ * Based on arch/arm/kernel/signal.c
+ *
+ * Copyright (C) 1995-2009 Russell King
+ * Copyright (C) 2012 ARM Ltd.
+ * Modified by Will Deacon <will.deacon@arm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/compat.h>
+#include <linux/signal.h>
+#include <linux/ratelimit.h>
+#include <linux/uaccess.h>
+
+#include <asm/esr.h>
+#include <asm/fpsimd.h>
+#include <asm/signal32_common.h>
+#include <asm/unistd.h>
+
+int put_sigset_t(compat_sigset_t __user *uset, sigset_t *set)
+{
+	compat_sigset_t	cset;
+
+	cset.sig[0] = set->sig[0] & 0xffffffffull;
+	cset.sig[1] = set->sig[0] >> 32;
+
+	return copy_to_user(uset, &cset, sizeof(*uset));
+}
+
+int get_sigset_t(sigset_t *set, const compat_sigset_t __user *uset)
+{
+	compat_sigset_t s32;
+
+	if (copy_from_user(&s32, uset, sizeof(*uset)))
+		return -EFAULT;
+
+	set->sig[0] = s32.sig[0] | (((long)s32.sig[1]) << 32);
+	return 0;
+}
+
+int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
+{
+	int err;
+
+	if (!access_ok(VERIFY_WRITE, to, sizeof(*to)))
+		return -EFAULT;
+
+	/* If you change siginfo_t structure, please be sure
+	 * this code is fixed accordingly.
+	 * It should never copy any pad contained in the structure
+	 * to avoid security leaks, but must copy the generic
+	 * 3 ints plus the relevant union member.
+	 * This routine must convert siginfo from 64bit to 32bit as well
+	 * at the same time.
+	 */
+	err = __put_user(from->si_signo, &to->si_signo);
+	err |= __put_user(from->si_errno, &to->si_errno);
+	err |= __put_user((short)from->si_code, &to->si_code);
+	if (from->si_code < 0)
+		err |= __copy_to_user(&to->_sifields._pad, &from->_sifields._pad,
+				      SI_PAD_SIZE);
+	else switch (from->si_code & __SI_MASK) {
+	case __SI_KILL:
+		err |= __put_user(from->si_pid, &to->si_pid);
+		err |= __put_user(from->si_uid, &to->si_uid);
+		break;
+	case __SI_TIMER:
+		 err |= __put_user(from->si_tid, &to->si_tid);
+		 err |= __put_user(from->si_overrun, &to->si_overrun);
+		 err |= __put_user(from->si_int, &to->si_int);
+		break;
+	case __SI_POLL:
+		err |= __put_user(from->si_band, &to->si_band);
+		err |= __put_user(from->si_fd, &to->si_fd);
+		break;
+	case __SI_FAULT:
+		err |= __put_user((compat_uptr_t)(unsigned long)from->si_addr,
+				  &to->si_addr);
+#ifdef BUS_MCEERR_AO
+		/*
+		 * Other callers might not initialize the si_lsb field,
+		 * so check explicitly for the right codes here.
+		 */
+		if (from->si_signo == SIGBUS &&
+		    (from->si_code == BUS_MCEERR_AR || from->si_code == BUS_MCEERR_AO))
+			err |= __put_user(from->si_addr_lsb, &to->si_addr_lsb);
+#endif
+		break;
+	case __SI_CHLD:
+		err |= __put_user(from->si_pid, &to->si_pid);
+		err |= __put_user(from->si_uid, &to->si_uid);
+		err |= __put_user(from->si_status, &to->si_status);
+		err |= __put_user(from->si_utime, &to->si_utime);
+		err |= __put_user(from->si_stime, &to->si_stime);
+		break;
+	case __SI_RT: /* This is not generated by the kernel as of now. */
+	case __SI_MESGQ: /* But this is */
+		err |= __put_user(from->si_pid, &to->si_pid);
+		err |= __put_user(from->si_uid, &to->si_uid);
+		err |= __put_user(from->si_int, &to->si_int);
+		break;
+	case __SI_SYS:
+		err |= __put_user((compat_uptr_t)(unsigned long)
+				from->si_call_addr, &to->si_call_addr);
+		err |= __put_user(from->si_syscall, &to->si_syscall);
+		err |= __put_user(from->si_arch, &to->si_arch);
+		break;
+	default: /* this is just in case for now ... */
+		err |= __put_user(from->si_pid, &to->si_pid);
+		err |= __put_user(from->si_uid, &to->si_uid);
+		break;
+	}
+	return err;
+}
+
+int copy_siginfo_from_user32(siginfo_t *to, compat_siginfo_t __user *from)
+{
+	if (copy_from_user(to, from, __ARCH_SI_PREAMBLE_SIZE) ||
+	    copy_from_user(to->_sifields._pad,
+			   from->_sifields._pad, SI_PAD_SIZE))
+		return -EFAULT;
+
+	return 0;
+}