linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] ipc/shm: Fix order of parameters when calling copy_compat_shmid_to_user
@ 2017-09-18 16:47 Will Deacon
  2017-09-20  9:41 ` Will Deacon
  2017-09-20 11:01 ` Al Viro
  0 siblings, 2 replies; 3+ messages in thread
From: Will Deacon @ 2017-09-18 16:47 UTC (permalink / raw)
  To: viro; +Cc: linux-kernel, Will Deacon, Andrew Morton

Commit 553f770ef71b ("ipc: move compat shmctl to native") moved the
compat IPC syscall handling into ipc/shm.c and refactored the struct
accessors in the process. Unfortunately, the call to
copy_compat_shmid_to_user when handling a compat {IPC,SHM}_STAT command
gets the arguments the wrong way round, passing a kernel stack address
as the user buffer (destination) and the user buffer as the kernel stack
address (source).

This patch fixes the parameter ordering so the buffers are accessed
correctly.

Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 ipc/shm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipc/shm.c b/ipc/shm.c
index 1b3adfe3c60e..1e2b1692ba2c 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -1237,7 +1237,7 @@ COMPAT_SYSCALL_DEFINE3(shmctl, int, shmid, int, cmd, void __user *, uptr)
 		err = shmctl_stat(ns, shmid, cmd, &sem64);
 		if (err < 0)
 			return err;
-		if (copy_compat_shmid_to_user(&sem64, uptr, version))
+		if (copy_compat_shmid_to_user(uptr, &sem64, version))
 			err = -EFAULT;
 		return err;
 
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] ipc/shm: Fix order of parameters when calling copy_compat_shmid_to_user
  2017-09-18 16:47 [PATCH] ipc/shm: Fix order of parameters when calling copy_compat_shmid_to_user Will Deacon
@ 2017-09-20  9:41 ` Will Deacon
  2017-09-20 11:01 ` Al Viro
  1 sibling, 0 replies; 3+ messages in thread
From: Will Deacon @ 2017-09-20  9:41 UTC (permalink / raw)
  To: viro; +Cc: linux-kernel, Andrew Morton

On Mon, Sep 18, 2017 at 05:47:38PM +0100, Will Deacon wrote:
> Commit 553f770ef71b ("ipc: move compat shmctl to native") moved the
> compat IPC syscall handling into ipc/shm.c and refactored the struct
> accessors in the process. Unfortunately, the call to
> copy_compat_shmid_to_user when handling a compat {IPC,SHM}_STAT command
> gets the arguments the wrong way round, passing a kernel stack address
> as the user buffer (destination) and the user buffer as the kernel stack
> address (source).
> 
> This patch fixes the parameter ordering so the buffers are accessed
> correctly.
> 
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  ipc/shm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ipc/shm.c b/ipc/shm.c
> index 1b3adfe3c60e..1e2b1692ba2c 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -1237,7 +1237,7 @@ COMPAT_SYSCALL_DEFINE3(shmctl, int, shmid, int, cmd, void __user *, uptr)
>  		err = shmctl_stat(ns, shmid, cmd, &sem64);
>  		if (err < 0)
>  			return err;
> -		if (copy_compat_shmid_to_user(&sem64, uptr, version))
> +		if (copy_compat_shmid_to_user(uptr, &sem64, version))
>  			err = -EFAULT;
>  		return err;

FWIW, this can easily Oops an arm64 kernel with a 32-bit userspace. LTP's
hugeshmctl02 test tickles the problem by doing:

	[...]
	shmget(0x710e4ddf, 134217728, IPC_CREAT|IPC_EXCL|SHM_HUGETLB|0600) = 32769
	[...]
	shmctl(32769, IPC_64|IPC_STAT, 0xffffffff)

which causes:

[  345.655736] Unable to handle kernel paging request at virtual address ffffffff
[  345.677173] Mem abort info:
[  345.685444]   Exception class = DABT (current EL), IL = 32 bits
[  345.702981]   SET = 0, FnV = 0
[  345.712016]   EA = 0, S1PTW = 0
[  345.721308] Data abort info:
[  345.729838]   ISV = 0, ISS = 0x00000006
[  345.741189]   CM = 0, WnR = 0
[  345.749966] user pgtable: 4k pages, 48-bit VAs, pgd = ffff800975801000
[  345.769306] [00000000ffffffff] *pgd=00000009f5397003, *pud=00000009f4c16003, *pmd=0000000000000000
[  345.795873] Internal error: Oops: 96000006 [#1] PREEMPT SMP
[  345.812370] Modules linked in:
[  345.821400] CPU: 2 PID: 2586 Comm: hugeshmctl02 Not tainted 4.14.0-rc1 #1
[  345.841506] Hardware name: ARM Juno development board (r2) (DT)
[  345.795873] Internal error: Oops: 96000006 [#1] PREEMPT SMP
[  345.894072] task: ffff8009758bf000 task.stack: ffff00000dc70000
[  345.911607] PC is at to_compat_ipc64_perm+0x0/0x40
[  345.925789] LR is at copy_compat_shmid_to_user+0xbc/0x120
[  345.941770] pc : [<ffff000008362350>] lr : [<ffff000008368a2c>] pstate: 60000145
[  345.963678] sp : ffff00000dc73d60
[  345.973475] x29: ffff00000dc73d60 x28: ffff8009758bf000 
[  345.989203] x27: ffff0000089d4000 x26: 0000000000000134 
[  346.004930] x25: 000000000000018e x24: ffff000008f52eb0 
[  346.020656] x23: 00000000ffffffff x22: ffff8009758bf000 
[  346.036383] x21: 0000000000000100 x20: ffff00000dc73e50 
[  346.052109] x19: 00000000ffffffff x18: 0000000000000000 
[  346.067836] x17: 0000000000000000 x16: ffff000008369cd8 
[  346.083562] x15: 0000000000000000 x14: 00000000f7a2872f 
[  346.099289] x13: 00000000ffdc9334 x12: 0000000000000134 
[  346.115015] x11: 000000000002eeb4 x10: 0000000000000040 
[  346.130742] x9 : ffff000008f530a8 x8 : ffff800976e29240 
[  346.146468] x7 : ffff800976e29268 x6 : ffff000008f3fa48 
[  346.162195] x5 : 0000000000000001 x4 : 0000000000000000 
[  346.177921] x3 : ffff000008f3fa48 x2 : 0000000000000100 
[  346.193648] x1 : 00000000ffffffff x0 : ffff00000dc73d88 
[  346.209375] Process hugeshmctl02 (pid: 2586, stack limit = 0xffff00000dc70000)

Will

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] ipc/shm: Fix order of parameters when calling copy_compat_shmid_to_user
  2017-09-18 16:47 [PATCH] ipc/shm: Fix order of parameters when calling copy_compat_shmid_to_user Will Deacon
  2017-09-20  9:41 ` Will Deacon
@ 2017-09-20 11:01 ` Al Viro
  1 sibling, 0 replies; 3+ messages in thread
From: Al Viro @ 2017-09-20 11:01 UTC (permalink / raw)
  To: Will Deacon; +Cc: linux-kernel, Andrew Morton

On Mon, Sep 18, 2017 at 05:47:38PM +0100, Will Deacon wrote:
> Commit 553f770ef71b ("ipc: move compat shmctl to native") moved the
> compat IPC syscall handling into ipc/shm.c and refactored the struct
> accessors in the process. Unfortunately, the call to
> copy_compat_shmid_to_user when handling a compat {IPC,SHM}_STAT command
> gets the arguments the wrong way round, passing a kernel stack address
> as the user buffer (destination) and the user buffer as the kernel stack
> address (source).
> 
> This patch fixes the parameter ordering so the buffers are accessed
> correctly.

ACK, will push to Linus tonight...

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2017-09-20 11:01 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-18 16:47 [PATCH] ipc/shm: Fix order of parameters when calling copy_compat_shmid_to_user Will Deacon
2017-09-20  9:41 ` Will Deacon
2017-09-20 11:01 ` Al Viro

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).