From 7191adff2a5566efb139c79ea03eda3d0520d44a Mon Sep 17 00:00:00 2001 From: Eric W. Biederman Date: Thu, 22 Mar 2018 21:08:27 -0500 Subject: shm/security: Pass kern_ipc_perm not shmid_kernel into the shm security hooks All of the implementations of security hooks that take shmid_kernel only access shm_perm the struct kern_ipc_perm member. This means the dependencies of the shm security hooks can be simplified by passing the kern_ipc_perm member of shmid_kernel.. Making this change will allow struct shmid_kernel to become private to ipc/shm.c. Signed-off-by: "Eric W. Biederman" --- ipc/shm.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) (limited to 'ipc/shm.c') diff --git a/ipc/shm.c b/ipc/shm.c index 4643865e9171..387a786e7be1 100644 --- a/ipc/shm.c +++ b/ipc/shm.c @@ -181,7 +181,7 @@ static void shm_rcu_free(struct rcu_head *head) rcu); struct shmid_kernel *shp = container_of(ptr, struct shmid_kernel, shm_perm); - security_shm_free(shp); + security_shm_free(&shp->shm_perm); kvfree(shp); } @@ -554,7 +554,7 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params) shp->mlock_user = NULL; shp->shm_perm.security = NULL; - error = security_shm_alloc(shp); + error = security_shm_alloc(&shp->shm_perm); if (error) { kvfree(shp); return error; @@ -635,10 +635,7 @@ no_file: */ static inline int shm_security(struct kern_ipc_perm *ipcp, int shmflg) { - struct shmid_kernel *shp; - - shp = container_of(ipcp, struct shmid_kernel, shm_perm); - return security_shm_associate(shp, shmflg); + return security_shm_associate(ipcp, shmflg); } /* @@ -835,7 +832,7 @@ static int shmctl_down(struct ipc_namespace *ns, int shmid, int cmd, shp = container_of(ipcp, struct shmid_kernel, shm_perm); - err = security_shm_shmctl(shp, cmd); + err = security_shm_shmctl(&shp->shm_perm, cmd); if (err) goto out_unlock1; @@ -934,7 +931,7 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid, if (ipcperms(ns, &shp->shm_perm, S_IRUGO)) goto out_unlock; - err = security_shm_shmctl(shp, cmd); + err = security_shm_shmctl(&shp->shm_perm, cmd); if (err) goto out_unlock; @@ -978,7 +975,7 @@ static int shmctl_do_lock(struct ipc_namespace *ns, int shmid, int cmd) } audit_ipc_obj(&(shp->shm_perm)); - err = security_shm_shmctl(shp, cmd); + err = security_shm_shmctl(&shp->shm_perm, cmd); if (err) goto out_unlock1; @@ -1348,7 +1345,7 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, if (ipcperms(ns, &shp->shm_perm, acc_mode)) goto out_unlock; - err = security_shm_shmat(shp, shmaddr, shmflg); + err = security_shm_shmat(&shp->shm_perm, shmaddr, shmflg); if (err) goto out_unlock; -- cgit v1.2.3-55-g7522 From a2e102cd3cdd8b7a14e08716510707b15802073f Mon Sep 17 00:00:00 2001 From: Eric W. Biederman Date: Thu, 22 Mar 2018 21:34:44 -0500 Subject: shm: Move struct shmid_kernel into ipc/shm.c All of the users are now in ipc/shm.c so make the definition local to that file to make code maintenance easier. AKA to prevent rebuilding the entire kernel when struct shmid_kernel changes. Signed-off-by: "Eric W. Biederman" --- include/linux/shm.h | 22 ---------------------- ipc/shm.c | 22 ++++++++++++++++++++++ 2 files changed, 22 insertions(+), 22 deletions(-) (limited to 'ipc/shm.c') diff --git a/include/linux/shm.h b/include/linux/shm.h index 2bbafacfbfc9..3a8eae3ca33c 100644 --- a/include/linux/shm.h +++ b/include/linux/shm.h @@ -7,28 +7,6 @@ #include #include -struct shmid_kernel /* private to the kernel */ -{ - struct kern_ipc_perm shm_perm; - struct file *shm_file; - unsigned long shm_nattch; - unsigned long shm_segsz; - time64_t shm_atim; - time64_t shm_dtim; - time64_t shm_ctim; - pid_t shm_cprid; - pid_t shm_lprid; - struct user_struct *mlock_user; - - /* The task created the shm object. NULL if the task is dead. */ - struct task_struct *shm_creator; - struct list_head shm_clist; /* list by creator */ -} __randomize_layout; - -/* shm_mode upper byte flags */ -#define SHM_DEST 01000 /* segment will be destroyed on last detach */ -#define SHM_LOCKED 02000 /* segment will not be swapped */ - #ifdef CONFIG_SYSVIPC struct sysv_shm { struct list_head shm_clist; diff --git a/ipc/shm.c b/ipc/shm.c index 387a786e7be1..0565669ebe5c 100644 --- a/ipc/shm.c +++ b/ipc/shm.c @@ -48,6 +48,28 @@ #include "util.h" +struct shmid_kernel /* private to the kernel */ +{ + struct kern_ipc_perm shm_perm; + struct file *shm_file; + unsigned long shm_nattch; + unsigned long shm_segsz; + time64_t shm_atim; + time64_t shm_dtim; + time64_t shm_ctim; + pid_t shm_cprid; + pid_t shm_lprid; + struct user_struct *mlock_user; + + /* The task created the shm object. NULL if the task is dead. */ + struct task_struct *shm_creator; + struct list_head shm_clist; /* list by creator */ +} __randomize_layout; + +/* shm_mode upper byte flags */ +#define SHM_DEST 01000 /* segment will be destroyed on last detach */ +#define SHM_LOCKED 02000 /* segment will not be swapped */ + struct shm_file_data { int id; struct ipc_namespace *ns; -- cgit v1.2.3-55-g7522 From 98f929b1bd4d0b7c7a77d0d9776d1b924db2e454 Mon Sep 17 00:00:00 2001 From: Eric W. Biederman Date: Fri, 23 Mar 2018 00:29:57 -0500 Subject: ipc/shm: Fix shmctl(..., IPC_STAT, ...) between pid namespaces. Today shm_cpid and shm_lpid are remembered in the pid namespace of the creator and the processes that last touched a sysvipc shared memory segment. If you have processes in multiple pid namespaces that is just wrong, and I don't know how this has been over-looked for so long. As only creation and shared memory attach and shared memory detach update the pids I do not expect there to be a repeat of the issues when struct pid was attached to each af_unix skb, which in some notable cases cut the performance in half. The problem was threads of the same process updating same struct pid from different cpus causing the cache line to be highly contended and bounce between cpus. As creation, attach, and detach are expected to be rare operations for sysvipc shared memory segments I do not expect that kind of cache line ping pong to cause probems. In addition because the pid is at a fixed location in the structure instead of being dynamic on a skb, the reference count of the pid does not need to be updated on each operation if the pid is the same. This ability to simply skip the pid reference count changes if the pid is unchanging further reduces the likelihood of the a cache line holding a pid reference count ping-ponging between cpus. Fixes: b488893a390e ("pid namespaces: changes to show virtual ids to user") Reviewed-by: Nagarathnam Muthusamy Signed-off-by: "Eric W. Biederman" --- ipc/shm.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) (limited to 'ipc/shm.c') diff --git a/ipc/shm.c b/ipc/shm.c index 0565669ebe5c..932b7e411c6c 100644 --- a/ipc/shm.c +++ b/ipc/shm.c @@ -57,8 +57,8 @@ struct shmid_kernel /* private to the kernel */ time64_t shm_atim; time64_t shm_dtim; time64_t shm_ctim; - pid_t shm_cprid; - pid_t shm_lprid; + struct pid *shm_cprid; + struct pid *shm_lprid; struct user_struct *mlock_user; /* The task created the shm object. NULL if the task is dead. */ @@ -226,7 +226,7 @@ static int __shm_open(struct vm_area_struct *vma) return PTR_ERR(shp); shp->shm_atim = ktime_get_real_seconds(); - shp->shm_lprid = task_tgid_vnr(current); + ipc_update_pid(&shp->shm_lprid, task_tgid(current)); shp->shm_nattch++; shm_unlock(shp); return 0; @@ -267,6 +267,8 @@ static void shm_destroy(struct ipc_namespace *ns, struct shmid_kernel *shp) user_shm_unlock(i_size_read(file_inode(shm_file)), shp->mlock_user); fput(shm_file); + ipc_update_pid(&shp->shm_cprid, NULL); + ipc_update_pid(&shp->shm_lprid, NULL); ipc_rcu_putref(&shp->shm_perm, shm_rcu_free); } @@ -311,7 +313,7 @@ static void shm_close(struct vm_area_struct *vma) if (WARN_ON_ONCE(IS_ERR(shp))) goto done; /* no-op */ - shp->shm_lprid = task_tgid_vnr(current); + ipc_update_pid(&shp->shm_lprid, task_tgid(current)); shp->shm_dtim = ktime_get_real_seconds(); shp->shm_nattch--; if (shm_may_destroy(ns, shp)) @@ -614,8 +616,8 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params) if (IS_ERR(file)) goto no_file; - shp->shm_cprid = task_tgid_vnr(current); - shp->shm_lprid = 0; + shp->shm_cprid = get_pid(task_tgid(current)); + shp->shm_lprid = NULL; shp->shm_atim = shp->shm_dtim = 0; shp->shm_ctim = ktime_get_real_seconds(); shp->shm_segsz = size; @@ -648,6 +650,8 @@ no_id: user_shm_unlock(size, shp->mlock_user); fput(file); no_file: + ipc_update_pid(&shp->shm_cprid, NULL); + ipc_update_pid(&shp->shm_lprid, NULL); call_rcu(&shp->shm_perm.rcu, shm_rcu_free); return error; } @@ -970,8 +974,8 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid, tbuf->shm_atime = shp->shm_atim; tbuf->shm_dtime = shp->shm_dtim; tbuf->shm_ctime = shp->shm_ctim; - tbuf->shm_cpid = shp->shm_cprid; - tbuf->shm_lpid = shp->shm_lprid; + tbuf->shm_cpid = pid_vnr(shp->shm_cprid); + tbuf->shm_lpid = pid_vnr(shp->shm_lprid); tbuf->shm_nattch = shp->shm_nattch; ipc_unlock_object(&shp->shm_perm); @@ -1605,6 +1609,7 @@ SYSCALL_DEFINE1(shmdt, char __user *, shmaddr) #ifdef CONFIG_PROC_FS static int sysvipc_shm_proc_show(struct seq_file *s, void *it) { + struct pid_namespace *pid_ns = ipc_seq_pid_ns(s); struct user_namespace *user_ns = seq_user_ns(s); struct kern_ipc_perm *ipcp = it; struct shmid_kernel *shp; @@ -1627,8 +1632,8 @@ static int sysvipc_shm_proc_show(struct seq_file *s, void *it) shp->shm_perm.id, shp->shm_perm.mode, shp->shm_segsz, - shp->shm_cprid, - shp->shm_lprid, + pid_nr_ns(shp->shm_cprid, pid_ns), + pid_nr_ns(shp->shm_lprid, pid_ns), shp->shm_nattch, from_kuid_munged(user_ns, shp->shm_perm.uid), from_kgid_munged(user_ns, shp->shm_perm.gid), -- cgit v1.2.3-55-g7522 From 50ab44b1c5d1b13305ce8acb74c8e50e0dcbaedc Mon Sep 17 00:00:00 2001 From: Eric W. Biederman Date: Fri, 23 Mar 2018 23:41:55 -0500 Subject: ipc: Directly call the security hook in ipc_ops.associate After the last round of cleanups the shm, sem, and msg associate operations just became trivial wrappers around the appropriate security method. Simplify things further by just calling the security method directly. Signed-off-by: "Eric W. Biederman" --- ipc/msg.c | 10 +--------- ipc/sem.c | 10 +--------- ipc/shm.c | 10 +--------- 3 files changed, 3 insertions(+), 27 deletions(-) (limited to 'ipc/shm.c') diff --git a/ipc/msg.c b/ipc/msg.c index 825ad585a6ff..d667dd8e97ab 100644 --- a/ipc/msg.c +++ b/ipc/msg.c @@ -272,20 +272,12 @@ static void freeque(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp) ipc_rcu_putref(&msq->q_perm, msg_rcu_free); } -/* - * Called with msg_ids.rwsem and ipcp locked. - */ -static inline int msg_security(struct kern_ipc_perm *ipcp, int msgflg) -{ - return security_msg_queue_associate(ipcp, msgflg); -} - SYSCALL_DEFINE2(msgget, key_t, key, int, msgflg) { struct ipc_namespace *ns; static const struct ipc_ops msg_ops = { .getnew = newque, - .associate = msg_security, + .associate = security_msg_queue_associate, }; struct ipc_params msg_params; diff --git a/ipc/sem.c b/ipc/sem.c index 47b263960524..09d54af076a4 100644 --- a/ipc/sem.c +++ b/ipc/sem.c @@ -564,14 +564,6 @@ static int newary(struct ipc_namespace *ns, struct ipc_params *params) } -/* - * Called with sem_ids.rwsem and ipcp locked. - */ -static inline int sem_security(struct kern_ipc_perm *ipcp, int semflg) -{ - return security_sem_associate(ipcp, semflg); -} - /* * Called with sem_ids.rwsem and ipcp locked. */ @@ -592,7 +584,7 @@ SYSCALL_DEFINE3(semget, key_t, key, int, nsems, int, semflg) struct ipc_namespace *ns; static const struct ipc_ops sem_ops = { .getnew = newary, - .associate = sem_security, + .associate = security_sem_associate, .more_checks = sem_more_checks, }; struct ipc_params sem_params; diff --git a/ipc/shm.c b/ipc/shm.c index 932b7e411c6c..018db3d0e70e 100644 --- a/ipc/shm.c +++ b/ipc/shm.c @@ -656,14 +656,6 @@ no_file: return error; } -/* - * Called with shm_ids.rwsem and ipcp locked. - */ -static inline int shm_security(struct kern_ipc_perm *ipcp, int shmflg) -{ - return security_shm_associate(ipcp, shmflg); -} - /* * Called with shm_ids.rwsem and ipcp locked. */ @@ -684,7 +676,7 @@ SYSCALL_DEFINE3(shmget, key_t, key, size_t, size, int, shmflg) struct ipc_namespace *ns; static const struct ipc_ops shm_ops = { .getnew = newseg, - .associate = shm_security, + .associate = security_shm_associate, .more_checks = shm_more_checks, }; struct ipc_params shm_params; -- cgit v1.2.3-55-g7522 From 2236d4d39035b9839944603ec4b65ce71180a9ea Mon Sep 17 00:00:00 2001 From: Eric W. Biederman Date: Wed, 28 Mar 2018 13:38:55 -0500 Subject: ipc/shm: Fix pid freeing. The 0day kernel test build report reported an oops: > > IP: put_pid+0x22/0x5c > PGD 19efa067 P4D 19efa067 PUD 0 > Oops: 0000 [#1] > CPU: 0 PID: 727 Comm: trinity Not tainted 4.16.0-rc2-00010-g98f929b #1 > RIP: 0010:put_pid+0x22/0x5c > RSP: 0018:ffff986719f73e48 EFLAGS: 00010202 > RAX: 00000006d765f710 RBX: ffff98671a4fa4d0 RCX: ffff986719f73d40 > RDX: 000000006f6e6125 RSI: 0000000000000000 RDI: ffffffffa01e6d21 > RBP: ffffffffa0955fe0 R08: 0000000000000020 R09: 0000000000000000 > R10: 0000000000000078 R11: ffff986719f73e76 R12: 0000000000001000 > R13: 00000000ffffffea R14: 0000000054000fb0 R15: 0000000000000000 > FS: 00000000028c2880(0000) GS:ffffffffa06ad000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 0000000677846439 CR3: 0000000019fc1005 CR4: 00000000000606b0 > Call Trace: > ? ipc_update_pid+0x36/0x3e > ? newseg+0x34c/0x3a6 > ? ipcget+0x5d/0x528 > ? entry_SYSCALL_64_after_hwframe+0x52/0xb7 > ? SyS_shmget+0x5a/0x84 > ? do_syscall_64+0x194/0x1b3 > ? entry_SYSCALL_64_after_hwframe+0x42/0xb7 > Code: ff 05 e7 20 9b 03 58 c9 c3 48 ff 05 85 21 9b 03 48 85 ff 74 4f 8b 47 04 8b 17 48 ff 05 7c 21 9b 03 48 83 c0 03 48 c1 e0 04 ff ca <48> 8b 44 07 08 74 1f 48 ff 05 6c 21 9b 03 ff 0f 0f 94 c2 48 ff > RIP: put_pid+0x22/0x5c RSP: ffff986719f73e48 > CR2: 0000000677846439 > ---[ end trace ab8c5cb4389d37c5 ]--- > Kernel panic - not syncing: Fatal exception In newseg when changing shm_cprid and shm_lprid from pid_t to struct pid* I misread the kvmalloc as kvzalloc and thought shp was initialized to 0. As that is not the case it is not safe to for the error handling to address shm_cprid and shm_lprid before they are initialized. Therefore move the cleanup of shm_cprid and shm_lprid from the no_file error cleanup path to the no_id error cleanup path. Ensuring that an early error exit won't cause the oops above. Reported-by: kernel test robot Reviewed-by: Nagarathnam Muthusamy Signed-off-by: Eric W. Biederman --- ipc/shm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'ipc/shm.c') diff --git a/ipc/shm.c b/ipc/shm.c index 018db3d0e70e..35bdfe76d11b 100644 --- a/ipc/shm.c +++ b/ipc/shm.c @@ -646,12 +646,12 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params) return error; no_id: + ipc_update_pid(&shp->shm_cprid, NULL); + ipc_update_pid(&shp->shm_lprid, NULL); if (is_file_hugepages(file) && shp->mlock_user) user_shm_unlock(size, shp->mlock_user); fput(file); no_file: - ipc_update_pid(&shp->shm_cprid, NULL); - ipc_update_pid(&shp->shm_lprid, NULL); call_rcu(&shp->shm_perm.rcu, shm_rcu_free); return error; } -- cgit v1.2.3-55-g7522