Skip to contentSkip to conversation

ipc/shm: Fix order of parameters when calling copy_compat_shmid_to_user

PATCH In review Will Deacon 2 participants 3 messages Sep 18, 2017 – Sep 20, 2017

lore
Share & tools
lore

Will Deacon

author
#0 lore raw

PATCH 1/1 ipc/shm: Fix order of parameters when calling copy_compat_shmid_to_user

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 <[email protected]>
Cc Andrew Morton <[email protected]>
Signed-off-by Will Deacon <[email protected]>
---
 ipc/shm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
ipc/shm.c +1 -1
index 1b3adfe3c60e..1e2b1692ba2c 100644
@@ -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;
Signature · 1 line
2.1.4
author
#1 lore raw
Quoted 23 lines · Will Deacon

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 <[email protected]>
Cc Andrew Morton <[email protected]>
Signed-off-by Will Deacon <[email protected]>
---
 ipc/shm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
ipc/shm.c +0 -0
index 1b3adfe3c60e..1e2b1692ba2c 100644

ipc/shm.c COMPAT_SYSCALL_DEFINE3(shmctl, int, shmid, int, cmd, void __user *, uptr) · line 1242

Quoted 7 lines
ipc/shm.c +1 -1
@@ -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

#2 lore raw
In reply to Will Deacon
Quoted 10 lines · Will Deacon

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...