On Mon, Feb 19, 2018 at 08:31:21PM +0000, James Hogan wrote:
> On Mon, Feb 19, 2018 at 09:06:56PM +0300, Peter Mamonov wrote:
> > Hello,
> >
> > After upgrading the Linux kernel to the recent version I've found that the
> > Firefox browser from the Debian 8 (jessie),mipsel stopped working: it causes
> > Bus Error exception at startup. The problem is reproducible with the QEMU
> > virtual machine (qemu-system-mips64el). Thorough investigation revealed that
> > the following syscall in /lib/mipsel-linux-gnu/libpthread-2.19.so causes
> > Firefox's stack corruption at address 0x7fff5770:
> >
> > 0x77fabd28: li v0,4220
> > 0x77fabd2c: syscall
> >
> > Relevant registers contents are as follows:
> >
> > zero at v0 v1 a0 a1 a2 a3
> > R0 00000000 300004e0 0000107c 77c2e6b0 00000006 0000000e 7fff574c 7fff5770
> >
> > The stack corruption is caused by the following patch:
> >
> > commit 8c6657cb50cb037ff58b3f6a547c6569568f3527
> > Author: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> > Date: Mon Jun 26 23:51:31 2017 -0400
> >
> > Switch flock copyin/copyout primitives to copy_{from,to}_user()
> >
> > ... and lose HAVE_ARCH_...; if copy_{to,from}_user() on an
> > architecture sucks badly enough to make it a problem, we have
> > a worse problem.
> >
> > Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> >
> > Reverting the change in put_compat_flock() introduced by the patch prevents the
> > stack corruption:
> >
> > diff --git a/fs/fcntl.c b/fs/fcntl.c
> > index 0345a46b8856..c55afd836e5d 100644
> > --- a/fs/fcntl.c
> > +++ b/fs/fcntl.c
> > @@ -550,25 +550,27 @@ static int get_compat_flock64(struct flock *kfl, const struct compat_flock64 __u
> >
> > static int put_compat_flock(const struct flock *kfl, struct compat_flock __user *ufl)
> > {
> > - struct compat_flock fl;
> > -
> > - memset(&fl, 0, sizeof(struct compat_flock));
> > - copy_flock_fields(&fl, kfl);
> > - if (copy_to_user(ufl, &fl, sizeof(struct compat_flock)))
> > + if (!access_ok(VERIFY_WRITE, ufl, sizeof(*ufl)) ||
> > + __put_user(kfl->l_type, &ufl->l_type) ||
> > + __put_user(kfl->l_whence, &ufl->l_whence) ||
> > + __put_user(kfl->l_start, &ufl->l_start) ||
> > + __put_user(kfl->l_len, &ufl->l_len) ||
> > + __put_user(kfl->l_pid, &ufl->l_pid))
> > return -EFAULT;
> > return 0;
> > }
> >
> > Actually, the change introduced by the patch is ok. However, it looks like
> > there is either a mismatch of sizeof(struct compat_flock) between the kernel
> > and the user space or a mismatch of types used by the kernel and the user
> > space. Despite the fact that the user space is built for a different kernel
> > version (3.16), I believe this syscall should work fine with it, since `struct
> > compat_flock` did not change since the 3.16. So, probably, the problem is
> > caused by some discrepancies which were hidden until "Switch flock
> > copyin/copyout..." patch.
> >
> > Please, give your comments.
>
> Hmm, thanks for reporting this.
>
> The change this commit makes is to make it write the full compat_flock
> struct out, including the padding at the end, instead of only the
> specific fields, suggesting that MIPS' struct compat_flock on 64-bit
> doesn't match struct flock on 32-bit.
>
> Here's struct flock from arch/mips/include/uapi/asm/fcntl.h with offset
> annotations for 32-bit:
>
> struct flock {
> /*0*/ short l_type;
> /*2*/ short l_whence;
> /*4*/ __kernel_off_t l_start;
> /*8*/ __kernel_off_t l_len;
> /*12*/ long l_sysid;
> /*16*/ __kernel_pid_t l_pid;
> /*20*/ long pad[4];
> /*36*/
> };
>
> and here's struct compat_flock from arch/mips/include/asm/compat.h with
> offset annotations for 64-bit:
>
> struct compat_flock {
> /*0*/ short l_type;
> /*2*/ short l_whence;
> /*4*/ compat_off_t l_start;
> /*8*/ compat_off_t l_len;
> /*12*/ s32 l_sysid;
> /*16*/ compat_pid_t l_pid;
> /*20*/ short __unused;
> /*24*/ s32 pad[4];
> /*40*/
> };
>
> Clearly the existence of __unused is outright wrong here.
>
> Please can you test the following patch to see if it fixes the issue.
Yes, the patch fixes the issue.
And thanks for clarification.
Regards,
Peter
>
> Thanks again,
> James
>
> From ebcbbb431aa7cc97330793da8a30c51150963935 Mon Sep 17 00:00:00 2001
> From: James Hogan <jhogan@xxxxxxxxxx>
> Date: Mon, 19 Feb 2018 20:14:34 +0000
> Subject: [PATCH] MIPS: Drop spurious __unused in struct compat_flock
>
> MIPS' struct compat_flock doesn't match the 32-bit struct flock, as it
> has an extra short __unused before pad[4], which combined with alignment
> increases the size to 40 bytes compared with struct flock's 36 bytes.
>
> Since commit 8c6657cb50cb ("Switch flock copyin/copyout primitives to
> copy_{from,to}_user()"), put_compat_flock() writes the full compat_flock
> struct to userland, which results in corruption of the userland word
> after the struct flock when running 32-bit userlands on 64-bit kernels.
>
> This was observed to cause a bus error exception when starting Firefox
> on Debian 8 (Jessie).
>
> Reported-by: Peter Mamonov <pmamonov@xxxxxxxxx>
> Signed-off-by: James Hogan <jhogan@xxxxxxxxxx>
> Cc: Ralf Baechle <ralf@xxxxxxxxxxxxxx>
> Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> Cc: linux-mips@xxxxxxxxxxxxxx
> Cc: <stable@xxxxxxxxxxxxxxx> # 4.13+
> ---
> arch/mips/include/asm/compat.h | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/arch/mips/include/asm/compat.h b/arch/mips/include/asm/compat.h
> index 946681db8dc3..9a0fa66b81ac 100644
> --- a/arch/mips/include/asm/compat.h
> +++ b/arch/mips/include/asm/compat.h
> @@ -86,7 +86,6 @@ struct compat_flock {
> compat_off_t l_len;
> s32 l_sysid;
> compat_pid_t l_pid;
> - short __unused;
> s32 pad[4];
> };
>
> --
> 2.13.6
>
