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.
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
Attachment:
signature.asc
Description: Digital signature
