Re: fcntl64 syscall causes user program stack corruption

Re: fcntl64 syscall causes user program stack corruption

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 5619 bytes --]

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.

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


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: fcntl64 syscall causes user program stack corruption

[Peter Mamonov]

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
>