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@zeniv.linux.org.uk 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@zeniv.linux.org.uk

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@kernel.org 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@gmail.com Signed-off-by: James Hogan jhogan@kernel.org Cc: Ralf Baechle ralf@linux-mips.org Cc: Al Viro viro@zeniv.linux.org.uk Cc: linux-mips@linux-mips.org Cc: stable@vger.kernel.org # 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