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@xxxxxxxxxxxxxxxxxx> > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
