linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH -next v2 0/3] sysvipc: introduce STAT_ANY commands
@ 2018-02-15 16:24 Davidlohr Bueso
  2018-02-15 16:24 ` [PATCH 1/3] ipc/shm: introduce shmctl(SHM_STAT_ANY) Davidlohr Bueso
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Davidlohr Bueso @ 2018-02-15 16:24 UTC (permalink / raw)
  To: akpm
  Cc: mhocko, mtk.manpages, robert.kettler, manfred, ebiederm,
	keescook, linux-api, linux-kernel, dave

Changes from v1 (https://lwn.net/Articles/747225/):
- Renamed cmds to STAT_ANY, instead of STAT_ALL (per Eric).
- Added Robert's reported-by tag.
- Added changes for smack lsm.

Hi,

The following patches adds the discussed[1] new command for shm
as well as for sems and msq as they are subject to the same discrepancies
for ipc object permission checks between the syscall and via procfs.
These new commands are justified in that (1) we are stuck with this
semantics as changing syscall and procfs can break userland; and (2) some
users can benefit from performance (for large amounts of shm segments,
for example) from not having to parse the procfs interface.

Once (if) merged, I will submit the necesary manpage updates. But I'm
thinking something like:

diff --git a/man2/shmctl.2 b/man2/shmctl.2
index 7bb503999941..bb00bbe21a57 100644
--- a/man2/shmctl.2
+++ b/man2/shmctl.2
@@ -41,6 +41,7 @@
 .\" 2005-04-25, mtk -- noted aberrant Linux behavior w.r.t. new
 .\"	attaches to a segment that has already been marked for deletion.
 .\" 2005-08-02, mtk: Added IPC_INFO, SHM_INFO, SHM_STAT descriptions.
+.\" 2018-02-13, dbueso: Added SHM_STAT_ANY description.
 .\"
 .TH SHMCTL 2 2017-09-15 "Linux" "Linux Programmer's Manual"
 .SH NAME
@@ -242,6 +243,18 @@ However, the
 argument is not a segment identifier, but instead an index into
 the kernel's internal array that maintains information about
 all shared memory segments on the system.
+.TP
+.BR SHM_STAT_ANY " (Linux-specific)"
+Return a
+.I shmid_ds
+structure as for
+.BR SHM_STAT .
+However, the
+.I shm_perm.mode
+is not checked for read access for
+.IR shmid ,
+resembing the behaviour of
+/proc/sysvipc/shm.
 .PP
 The caller can prevent or allow swapping of a shared
 memory segment with the following \fIcmd\fP values:
@@ -287,7 +300,7 @@ operation returns the index of the highest used entry in the
 kernel's internal array recording information about all
 shared memory segments.
 (This information can be used with repeated
-.B SHM_STAT
+.B SHM_STAT/SHM_STAT_ANY
 operations to obtain information about all shared memory segments
 on the system.)
 A successful
@@ -328,7 +341,7 @@ isn't accessible.
 \fIshmid\fP is not a valid identifier, or \fIcmd\fP
 is not a valid command.
 Or: for a
-.B SHM_STAT
+.B SHM_STAT/SHM_STAT_ANY
 operation, the index value specified in
 .I shmid
 referred to an array slot that is currently unused.


Thanks!

[1] https://lkml.org/lkml/2017/12/19/220

*** BLURB HERE ***

Davidlohr Bueso (3):
  ipc/shm: introduce shmctl(SHM_STAT_ANY)
  ipc/sem: introduce semctl(SEM_STAT_ANY)
  ipc/msg: introduce msgctl(MSG_STAT_ANY)

 include/uapi/linux/msg.h   |  1 +
 include/uapi/linux/sem.h   |  1 +
 include/uapi/linux/shm.h   |  5 +++--
 ipc/msg.c                  | 17 ++++++++++++-----
 ipc/sem.c                  | 17 ++++++++++++-----
 ipc/shm.c                  | 23 ++++++++++++++++++-----
 security/selinux/hooks.c   |  3 +++
 security/smack/smack_lsm.c |  3 +++
 8 files changed, 53 insertions(+), 17 deletions(-)

-- 
2.13.6


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

* [PATCH 1/3] ipc/shm: introduce shmctl(SHM_STAT_ANY)
  2018-02-15 16:24 [PATCH -next v2 0/3] sysvipc: introduce STAT_ANY commands Davidlohr Bueso
@ 2018-02-15 16:24 ` Davidlohr Bueso
  2018-02-20 10:13   ` Michal Hocko
  2018-02-15 16:24 ` [PATCH 2/3] ipc/sem: introduce semctl(SEM_STAT_ANY) Davidlohr Bueso
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 7+ messages in thread
From: Davidlohr Bueso @ 2018-02-15 16:24 UTC (permalink / raw)
  To: akpm
  Cc: mhocko, mtk.manpages, robert.kettler, manfred, ebiederm,
	keescook, linux-api, linux-kernel, dave, Davidlohr Bueso

There is a permission discrepancy when consulting shm ipc
object metadata between /proc/sysvipc/shm (0444) and the
SHM_STAT shmctl command. The later does permission checks
for the object vs S_IRUGO. As such there can be cases where
EACCESS is returned via syscall but the info is displayed
anyways in the procfs files.

While this might have security implications via info leaking
(albeit no writing to the shm metadata), this behavior goes
way back and showing all the objects regardless of the
permissions was most likely an overlook - so we are stuck
with it. Furthermore, modifying either the syscall or the
procfs file can cause userspace programs to break (ie ipcs).
Some applications require getting the procfs info (without
root privileges) and can be rather slow in comparison with
a syscall -- up to 500x in some reported cases.

This patch introduces a new SHM_STAT_ANY command such that
the shm ipc object permissions are ignored, and only audited
instead. In addition, I've left the lsm security hook checks
in place, as if some policy can block the call, then the user
has no other choice than just parsing the procfs file.

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 include/uapi/linux/shm.h   |  5 +++--
 ipc/shm.c                  | 23 ++++++++++++++++++-----
 security/selinux/hooks.c   |  1 +
 security/smack/smack_lsm.c |  1 +
 4 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/include/uapi/linux/shm.h b/include/uapi/linux/shm.h
index 4de12a39b075..dde1344f047c 100644
--- a/include/uapi/linux/shm.h
+++ b/include/uapi/linux/shm.h
@@ -83,8 +83,9 @@ struct shmid_ds {
 #define SHM_UNLOCK 	12
 
 /* ipcs ctl commands */
-#define SHM_STAT 	13
-#define SHM_INFO 	14
+#define SHM_STAT	13
+#define SHM_INFO	14
+#define SHM_STAT_ANY    15
 
 /* Obsolete, used only for backwards compatibility */
 struct	shminfo {
diff --git a/ipc/shm.c b/ipc/shm.c
index 4643865e9171..60827d9c3716 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -915,14 +915,14 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid,
 	memset(tbuf, 0, sizeof(*tbuf));
 
 	rcu_read_lock();
-	if (cmd == SHM_STAT) {
+	if (cmd == SHM_STAT || cmd == SHM_STAT_ANY) {
 		shp = shm_obtain_object(ns, shmid);
 		if (IS_ERR(shp)) {
 			err = PTR_ERR(shp);
 			goto out_unlock;
 		}
 		id = shp->shm_perm.id;
-	} else {
+	} else { /* IPC_STAT */
 		shp = shm_obtain_object_check(ns, shmid);
 		if (IS_ERR(shp)) {
 			err = PTR_ERR(shp);
@@ -930,9 +930,20 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid,
 		}
 	}
 
-	err = -EACCES;
-	if (ipcperms(ns, &shp->shm_perm, S_IRUGO))
-		goto out_unlock;
+	/*
+	 * Semantically SHM_STAT_ANY ought to be identical to
+	 * that functionality provided by the /proc/sysvipc/
+	 * interface. As such, only audit these calls and
+	 * do not do traditional S_IRUGO permission checks on
+	 * the ipc object.
+	 */
+	if (cmd == SHM_STAT_ANY)
+		audit_ipc_obj(&shp->shm_perm);
+	else {
+		err = -EACCES;
+		if (ipcperms(ns, &shp->shm_perm, S_IRUGO))
+			goto out_unlock;
+	}
 
 	err = security_shm_shmctl(shp, cmd);
 	if (err)
@@ -1072,6 +1083,7 @@ SYSCALL_DEFINE3(shmctl, int, shmid, int, cmd, struct shmid_ds __user *, buf)
 		return err;
 	}
 	case SHM_STAT:
+	case SHM_STAT_ANY:
 	case IPC_STAT: {
 		err = shmctl_stat(ns, shmid, cmd, &sem64);
 		if (err < 0)
@@ -1245,6 +1257,7 @@ COMPAT_SYSCALL_DEFINE3(shmctl, int, shmid, int, cmd, void __user *, uptr)
 		return err;
 	}
 	case IPC_STAT:
+	case SHM_STAT_ANY:
 	case SHM_STAT:
 		err = shmctl_stat(ns, shmid, cmd, &sem64);
 		if (err < 0)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 35ef1e9045e8..373dceede50d 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5734,6 +5734,7 @@ static int selinux_shm_shmctl(struct shmid_kernel *shp, int cmd)
 				    SECCLASS_SYSTEM, SYSTEM__IPC_INFO, NULL);
 	case IPC_STAT:
 	case SHM_STAT:
+	case SHM_STAT_ANY:
 		perms = SHM__GETATTR | SHM__ASSOCIATE;
 		break;
 	case IPC_SET:
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 03fdecba93bb..51d22b03b0ae 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3034,6 +3034,7 @@ static int smack_shm_shmctl(struct shmid_kernel *shp, int cmd)
 	switch (cmd) {
 	case IPC_STAT:
 	case SHM_STAT:
+	case SHM_STAT_ANY:
 		may = MAY_READ;
 		break;
 	case IPC_SET:
-- 
2.13.6


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

* [PATCH 2/3] ipc/sem: introduce semctl(SEM_STAT_ANY)
  2018-02-15 16:24 [PATCH -next v2 0/3] sysvipc: introduce STAT_ANY commands Davidlohr Bueso
  2018-02-15 16:24 ` [PATCH 1/3] ipc/shm: introduce shmctl(SHM_STAT_ANY) Davidlohr Bueso
@ 2018-02-15 16:24 ` Davidlohr Bueso
  2018-02-15 16:24 ` [PATCH 3/3] ipc/msg: introduce msgctl(MSG_STAT_ANY) Davidlohr Bueso
  2018-03-20 18:55 ` [PATCH -next v2 0/3] sysvipc: introduce STAT_ANY commands Davidlohr Bueso
  3 siblings, 0 replies; 7+ messages in thread
From: Davidlohr Bueso @ 2018-02-15 16:24 UTC (permalink / raw)
  To: akpm
  Cc: mhocko, mtk.manpages, robert.kettler, manfred, ebiederm,
	keescook, linux-api, linux-kernel, dave, Davidlohr Bueso

There is a permission discrepancy when consulting shm ipc
object metadata between /proc/sysvipc/sem (0444) and the
SEM_STAT semctl command. The later does permission checks
for the object vs S_IRUGO. As such there can be cases where
EACCESS is returned via syscall but the info is displayed
anyways in the procfs files.

While this might have security implications via info leaking
(albeit no writing to the sma metadata), this behavior goes
way back and showing all the objects regardless of the
permissions was most likely an overlook - so we are stuck
with it. Furthermore, modifying either the syscall or the
procfs file can cause userspace programs to break (ie ipcs).
Some applications require getting the procfs info (without
root privileges) and can be rather slow in comparison with
a syscall -- up to 500x in some reported cases for shm.

This patch introduces a new SEM_STAT_ANY command such that
the sem ipc object permissions are ignored, and only audited
instead. In addition, I've left the lsm security hook checks
in place, as if some policy can block the call, then the user
has no other choice than just parsing the procfs file.

Reported-by: Robert Kettler <robert.kettler@outlook.com>
Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 include/uapi/linux/sem.h   |  1 +
 ipc/sem.c                  | 17 ++++++++++++-----
 security/selinux/hooks.c   |  1 +
 security/smack/smack_lsm.c |  1 +
 4 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/include/uapi/linux/sem.h b/include/uapi/linux/sem.h
index 9c3e745b0656..39a1876f039e 100644
--- a/include/uapi/linux/sem.h
+++ b/include/uapi/linux/sem.h
@@ -19,6 +19,7 @@
 /* ipcs ctl cmds */
 #define SEM_STAT 18
 #define SEM_INFO 19
+#define SEM_STAT_ANY 20
 
 /* Obsolete, used only for backwards compatibility and libc5 compiles */
 struct semid_ds {
diff --git a/ipc/sem.c b/ipc/sem.c
index a4af04979fd2..79acad0e0aa1 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1190,14 +1190,14 @@ static int semctl_stat(struct ipc_namespace *ns, int semid,
 	memset(semid64, 0, sizeof(*semid64));
 
 	rcu_read_lock();
-	if (cmd == SEM_STAT) {
+	if (cmd == SEM_STAT || cmd == SEM_STAT_ANY) {
 		sma = sem_obtain_object(ns, semid);
 		if (IS_ERR(sma)) {
 			err = PTR_ERR(sma);
 			goto out_unlock;
 		}
 		id = sma->sem_perm.id;
-	} else {
+	} else { /* IPC_STAT */
 		sma = sem_obtain_object_check(ns, semid);
 		if (IS_ERR(sma)) {
 			err = PTR_ERR(sma);
@@ -1205,9 +1205,14 @@ static int semctl_stat(struct ipc_namespace *ns, int semid,
 		}
 	}
 
-	err = -EACCES;
-	if (ipcperms(ns, &sma->sem_perm, S_IRUGO))
-		goto out_unlock;
+	/* see comment for SHM_STAT_ANY */
+	if (cmd == SEM_STAT_ANY)
+		audit_ipc_obj(&sma->sem_perm);
+	else {
+		err = -EACCES;
+		if (ipcperms(ns, &sma->sem_perm, S_IRUGO))
+			goto out_unlock;
+	}
 
 	err = security_sem_semctl(sma, cmd);
 	if (err)
@@ -1596,6 +1601,7 @@ SYSCALL_DEFINE4(semctl, int, semid, int, semnum, int, cmd, unsigned long, arg)
 		return semctl_info(ns, semid, cmd, p);
 	case IPC_STAT:
 	case SEM_STAT:
+	case SEM_STAT_ANY:
 		err = semctl_stat(ns, semid, cmd, &semid64);
 		if (err < 0)
 			return err;
@@ -1697,6 +1703,7 @@ COMPAT_SYSCALL_DEFINE4(semctl, int, semid, int, semnum, int, cmd, int, arg)
 		return semctl_info(ns, semid, cmd, p);
 	case IPC_STAT:
 	case SEM_STAT:
+	case SEM_STAT_ANY:
 		err = semctl_stat(ns, semid, cmd, &semid64);
 		if (err < 0)
 			return err;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 373dceede50d..38f71d12206a 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5847,6 +5847,7 @@ static int selinux_sem_semctl(struct sem_array *sma, int cmd)
 		break;
 	case IPC_STAT:
 	case SEM_STAT:
+	case SEM_STAT_ANY:
 		perms = SEM__GETATTR | SEM__ASSOCIATE;
 		break;
 	default:
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 51d22b03b0ae..d478c0940b00 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3167,6 +3167,7 @@ static int smack_sem_semctl(struct sem_array *sma, int cmd)
 	case GETALL:
 	case IPC_STAT:
 	case SEM_STAT:
+	case SEM_STAT_ANY:
 		may = MAY_READ;
 		break;
 	case SETVAL:
-- 
2.13.6


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

* [PATCH 3/3] ipc/msg: introduce msgctl(MSG_STAT_ANY)
  2018-02-15 16:24 [PATCH -next v2 0/3] sysvipc: introduce STAT_ANY commands Davidlohr Bueso
  2018-02-15 16:24 ` [PATCH 1/3] ipc/shm: introduce shmctl(SHM_STAT_ANY) Davidlohr Bueso
  2018-02-15 16:24 ` [PATCH 2/3] ipc/sem: introduce semctl(SEM_STAT_ANY) Davidlohr Bueso
@ 2018-02-15 16:24 ` Davidlohr Bueso
  2018-03-20 18:55 ` [PATCH -next v2 0/3] sysvipc: introduce STAT_ANY commands Davidlohr Bueso
  3 siblings, 0 replies; 7+ messages in thread
From: Davidlohr Bueso @ 2018-02-15 16:24 UTC (permalink / raw)
  To: akpm
  Cc: mhocko, mtk.manpages, robert.kettler, manfred, ebiederm,
	keescook, linux-api, linux-kernel, dave, Davidlohr Bueso

There is a permission discrepancy when consulting msq ipc
object metadata between /proc/sysvipc/msg (0444) and the
MSG_STAT shmctl command. The later does permission checks
for the object vs S_IRUGO. As such there can be cases where
EACCESS is returned via syscall but the info is displayed
anyways in the procfs files.

While this might have security implications via info leaking
(albeit no writing to the msq metadata), this behavior goes
way back and showing all the objects regardless of the
permissions was most likely an overlook - so we are stuck
with it. Furthermore, modifying either the syscall or the
procfs file can cause userspace programs to break (ie ipcs).
Some applications require getting the procfs info (without
root privileges) and can be rather slow in comparison with
a syscall -- up to 500x in some reported cases for shm.

This patch introduces a new MSG_STAT_ANY command such that
the msq ipc object permissions are ignored, and only audited
instead. In addition, I've left the lsm security hook checks
in place, as if some policy can block the call, then the user
has no other choice than just parsing the procfs file.

Reported-by: Robert Kettler <robert.kettler@outlook.com>
Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 include/uapi/linux/msg.h   |  1 +
 ipc/msg.c                  | 17 ++++++++++++-----
 security/selinux/hooks.c   |  1 +
 security/smack/smack_lsm.c |  1 +
 4 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/include/uapi/linux/msg.h b/include/uapi/linux/msg.h
index 5d5ab81dc9be..e4a0d9a9a9e8 100644
--- a/include/uapi/linux/msg.h
+++ b/include/uapi/linux/msg.h
@@ -7,6 +7,7 @@
 /* ipcs ctl commands */
 #define MSG_STAT 11
 #define MSG_INFO 12
+#define MSG_STAT_ANY 13
 
 /* msgrcv options */
 #define MSG_NOERROR     010000  /* no error if message is too big */
diff --git a/ipc/msg.c b/ipc/msg.c
index 0dcc6699dc53..644032335921 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -483,14 +483,14 @@ static int msgctl_stat(struct ipc_namespace *ns, int msqid,
 	memset(p, 0, sizeof(*p));
 
 	rcu_read_lock();
-	if (cmd == MSG_STAT) {
+	if (cmd == MSG_STAT || cmd == MSG_STAT_ANY) {
 		msq = msq_obtain_object(ns, msqid);
 		if (IS_ERR(msq)) {
 			err = PTR_ERR(msq);
 			goto out_unlock;
 		}
 		id = msq->q_perm.id;
-	} else {
+	} else { /* IPC_STAT */
 		msq = msq_obtain_object_check(ns, msqid);
 		if (IS_ERR(msq)) {
 			err = PTR_ERR(msq);
@@ -498,9 +498,14 @@ static int msgctl_stat(struct ipc_namespace *ns, int msqid,
 		}
 	}
 
-	err = -EACCES;
-	if (ipcperms(ns, &msq->q_perm, S_IRUGO))
-		goto out_unlock;
+	/* see comment for SHM_STAT_ANY */
+	if (cmd == MSG_STAT_ANY)
+		audit_ipc_obj(&msq->q_perm);
+	else {
+		err = -EACCES;
+		if (ipcperms(ns, &msq->q_perm, S_IRUGO))
+			goto out_unlock;
+	}
 
 	err = security_msg_queue_msgctl(msq, cmd);
 	if (err)
@@ -558,6 +563,7 @@ SYSCALL_DEFINE3(msgctl, int, msqid, int, cmd, struct msqid_ds __user *, buf)
 		return err;
 	}
 	case MSG_STAT:	/* msqid is an index rather than a msg queue id */
+	case MSG_STAT_ANY:
 	case IPC_STAT:
 		err = msgctl_stat(ns, msqid, cmd, &msqid64);
 		if (err < 0)
@@ -671,6 +677,7 @@ COMPAT_SYSCALL_DEFINE3(msgctl, int, msqid, int, cmd, void __user *, uptr)
 	}
 	case IPC_STAT:
 	case MSG_STAT:
+	case MSG_STAT_ANY:
 		err = msgctl_stat(ns, msqid, cmd, &msqid64);
 		if (err < 0)
 			return err;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 38f71d12206a..f87f538c83a9 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5591,6 +5591,7 @@ static int selinux_msg_queue_msgctl(struct msg_queue *msq, int cmd)
 				    SECCLASS_SYSTEM, SYSTEM__IPC_INFO, NULL);
 	case IPC_STAT:
 	case MSG_STAT:
+	case MSG_STAT_ANY:
 		perms = MSGQ__GETATTR | MSGQ__ASSOCIATE;
 		break;
 	case IPC_SET:
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index d478c0940b00..193159d5acf8 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3296,6 +3296,7 @@ static int smack_msg_queue_msgctl(struct msg_queue *msq, int cmd)
 	switch (cmd) {
 	case IPC_STAT:
 	case MSG_STAT:
+	case MSG_STAT_ANY:
 		may = MAY_READ;
 		break;
 	case IPC_SET:
-- 
2.13.6


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

* Re: [PATCH 1/3] ipc/shm: introduce shmctl(SHM_STAT_ANY)
  2018-02-15 16:24 ` [PATCH 1/3] ipc/shm: introduce shmctl(SHM_STAT_ANY) Davidlohr Bueso
@ 2018-02-20 10:13   ` Michal Hocko
  0 siblings, 0 replies; 7+ messages in thread
From: Michal Hocko @ 2018-02-20 10:13 UTC (permalink / raw)
  To: Davidlohr Bueso
  Cc: akpm, mtk.manpages, robert.kettler, manfred, ebiederm, keescook,
	linux-api, linux-kernel, Davidlohr Bueso

On Thu 15-02-18 08:24:56, Davidlohr Bueso wrote:
> There is a permission discrepancy when consulting shm ipc
> object metadata between /proc/sysvipc/shm (0444) and the
> SHM_STAT shmctl command. The later does permission checks
> for the object vs S_IRUGO. As such there can be cases where
> EACCESS is returned via syscall but the info is displayed
> anyways in the procfs files.
> 
> While this might have security implications via info leaking
> (albeit no writing to the shm metadata), this behavior goes
> way back and showing all the objects regardless of the
> permissions was most likely an overlook - so we are stuck
> with it. Furthermore, modifying either the syscall or the
> procfs file can cause userspace programs to break (ie ipcs).
> Some applications require getting the procfs info (without
> root privileges) and can be rather slow in comparison with
> a syscall -- up to 500x in some reported cases.
> 
> This patch introduces a new SHM_STAT_ANY command such that
> the shm ipc object permissions are ignored, and only audited
> instead. In addition, I've left the lsm security hook checks
> in place, as if some policy can block the call, then the user
> has no other choice than just parsing the procfs file.
> 
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>

Thanks for taking this over Davidlohr! The patch looks reasonable to me.
I am not very much familiar with the security modules (audit) part to
be honest but it matches my expectations.

Feel free to add my:
Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  include/uapi/linux/shm.h   |  5 +++--
>  ipc/shm.c                  | 23 ++++++++++++++++++-----
>  security/selinux/hooks.c   |  1 +
>  security/smack/smack_lsm.c |  1 +
>  4 files changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/include/uapi/linux/shm.h b/include/uapi/linux/shm.h
> index 4de12a39b075..dde1344f047c 100644
> --- a/include/uapi/linux/shm.h
> +++ b/include/uapi/linux/shm.h
> @@ -83,8 +83,9 @@ struct shmid_ds {
>  #define SHM_UNLOCK 	12
>  
>  /* ipcs ctl commands */
> -#define SHM_STAT 	13
> -#define SHM_INFO 	14
> +#define SHM_STAT	13
> +#define SHM_INFO	14
> +#define SHM_STAT_ANY    15
>  
>  /* Obsolete, used only for backwards compatibility */
>  struct	shminfo {
> diff --git a/ipc/shm.c b/ipc/shm.c
> index 4643865e9171..60827d9c3716 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -915,14 +915,14 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid,
>  	memset(tbuf, 0, sizeof(*tbuf));
>  
>  	rcu_read_lock();
> -	if (cmd == SHM_STAT) {
> +	if (cmd == SHM_STAT || cmd == SHM_STAT_ANY) {
>  		shp = shm_obtain_object(ns, shmid);
>  		if (IS_ERR(shp)) {
>  			err = PTR_ERR(shp);
>  			goto out_unlock;
>  		}
>  		id = shp->shm_perm.id;
> -	} else {
> +	} else { /* IPC_STAT */
>  		shp = shm_obtain_object_check(ns, shmid);
>  		if (IS_ERR(shp)) {
>  			err = PTR_ERR(shp);
> @@ -930,9 +930,20 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid,
>  		}
>  	}
>  
> -	err = -EACCES;
> -	if (ipcperms(ns, &shp->shm_perm, S_IRUGO))
> -		goto out_unlock;
> +	/*
> +	 * Semantically SHM_STAT_ANY ought to be identical to
> +	 * that functionality provided by the /proc/sysvipc/
> +	 * interface. As such, only audit these calls and
> +	 * do not do traditional S_IRUGO permission checks on
> +	 * the ipc object.
> +	 */
> +	if (cmd == SHM_STAT_ANY)
> +		audit_ipc_obj(&shp->shm_perm);
> +	else {
> +		err = -EACCES;
> +		if (ipcperms(ns, &shp->shm_perm, S_IRUGO))
> +			goto out_unlock;
> +	}
>  
>  	err = security_shm_shmctl(shp, cmd);
>  	if (err)
> @@ -1072,6 +1083,7 @@ SYSCALL_DEFINE3(shmctl, int, shmid, int, cmd, struct shmid_ds __user *, buf)
>  		return err;
>  	}
>  	case SHM_STAT:
> +	case SHM_STAT_ANY:
>  	case IPC_STAT: {
>  		err = shmctl_stat(ns, shmid, cmd, &sem64);
>  		if (err < 0)
> @@ -1245,6 +1257,7 @@ COMPAT_SYSCALL_DEFINE3(shmctl, int, shmid, int, cmd, void __user *, uptr)
>  		return err;
>  	}
>  	case IPC_STAT:
> +	case SHM_STAT_ANY:
>  	case SHM_STAT:
>  		err = shmctl_stat(ns, shmid, cmd, &sem64);
>  		if (err < 0)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 35ef1e9045e8..373dceede50d 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -5734,6 +5734,7 @@ static int selinux_shm_shmctl(struct shmid_kernel *shp, int cmd)
>  				    SECCLASS_SYSTEM, SYSTEM__IPC_INFO, NULL);
>  	case IPC_STAT:
>  	case SHM_STAT:
> +	case SHM_STAT_ANY:
>  		perms = SHM__GETATTR | SHM__ASSOCIATE;
>  		break;
>  	case IPC_SET:
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 03fdecba93bb..51d22b03b0ae 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -3034,6 +3034,7 @@ static int smack_shm_shmctl(struct shmid_kernel *shp, int cmd)
>  	switch (cmd) {
>  	case IPC_STAT:
>  	case SHM_STAT:
> +	case SHM_STAT_ANY:
>  		may = MAY_READ;
>  		break;
>  	case IPC_SET:
> -- 
> 2.13.6

-- 
Michal Hocko
SUSE Labs

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

* Re: [PATCH -next v2 0/3] sysvipc: introduce STAT_ANY commands
  2018-02-15 16:24 [PATCH -next v2 0/3] sysvipc: introduce STAT_ANY commands Davidlohr Bueso
                   ` (2 preceding siblings ...)
  2018-02-15 16:24 ` [PATCH 3/3] ipc/msg: introduce msgctl(MSG_STAT_ANY) Davidlohr Bueso
@ 2018-03-20 18:55 ` Davidlohr Bueso
  2018-11-04 16:29   ` Michael Kerrisk (man-pages)
  3 siblings, 1 reply; 7+ messages in thread
From: Davidlohr Bueso @ 2018-03-20 18:55 UTC (permalink / raw)
  To: akpm
  Cc: mhocko, mtk.manpages, robert.kettler, manfred, ebiederm,
	keescook, linux-api, linux-kernel

On Thu, 15 Feb 2018, Davidlohr Bueso wrote:

>Once (if) merged, I will submit the necesary manpage updates. But I'm
>thinking something like:

Hi Michael, here are the updated manpage entries. As always, please feel
free to modify the descriptions as you see fit.

Thanks,
Davidlohr


------------8<----------------------------------------------------------
[PATCH] sysvipc: add *_STAT_ANY command descriptions

The *ctl syscall descriptions have been updated to reflect the
new commands in msg queues, semaphores and shmem.

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 man2/msgctl.2 | 20 +++++++++++++++++---
 man2/semctl.2 | 19 ++++++++++++++++++-
 man2/shmctl.2 | 17 +++++++++++++++--
 3 files changed, 50 insertions(+), 6 deletions(-)

diff --git a/man2/msgctl.2 b/man2/msgctl.2
index df74c750457a..31b1a1f12fb0 100644
--- a/man2/msgctl.2
+++ b/man2/msgctl.2
@@ -33,6 +33,7 @@
 .\"	Language and formatting clean-ups
 .\"	Added msqid_ds and ipc_perm structure definitions
 .\" 2005-08-02, mtk: Added IPC_INFO, MSG_INFO, MSG_STAT descriptions
+.\" 2018-03-20, dbueso: Added MSG_STAT_ANY description.
 .\"
 .TH MSGCTL 2 2017-09-15 "Linux" "Linux Programmer's Manual"
 .SH NAME
@@ -222,10 +223,23 @@ Return a
 structure as for
 .BR IPC_STAT .
 However, the
-.I msqid
+.I msgid
 argument is not a queue identifier, but instead an index into
 the kernel's internal array that maintains information about
 all message queues on the system.
+.TP
+.BR MSG_STAT_ANY " (Linux-specific)"
+Return a
+.I msqid_ds
+structure as for
+.BR MSG_STAT .
+However, the
+.I msg_perm.mode
+is not checked for read access for
+.IR msqid ,
+resembing the behaviour of
+/proc/sysvipc/msg.
+.PP
 .SH RETURN VALUE
 On success,
 .BR IPC_STAT ,
@@ -241,10 +255,10 @@ operation returns the index of the highest used entry in the
 kernel's internal array recording information about all
 message queues.
 (This information can be used with repeated
-.B MSG_STAT
+.B MSG_STAT or MSG_STAT_ANY
 operations to obtain information about all queues on the system.)
 A successful
-.B MSG_STAT
+.B MSG_STAT or MSG_STAT_ANY
 operation returns the identifier of the queue whose index was given in
 .IR msqid .
 .PP
diff --git a/man2/semctl.2 b/man2/semctl.2
index 02930d2c74ae..0ebe0434c05f 100644
--- a/man2/semctl.2
+++ b/man2/semctl.2
@@ -37,6 +37,7 @@
 .\"	Rewrote semun text
 .\"	Added semid_ds and ipc_perm structure definitions
 .\" 2005-08-02, mtk: Added IPC_INFO, SEM_INFO, SEM_STAT descriptions.
+.\" 2018-03-20, dbueso: Added SEM_STAT_ANY description.
 .\"
 .TH SEMCTL 2 2017-09-15 "Linux" "Linux Programmer's Manual"
 .SH NAME
@@ -240,6 +241,17 @@ argument is not a semaphore identifier, but instead an index into
 the kernel's internal array that maintains information about
 all semaphore sets on the system.
 .TP
+.BR SEM_STAT_ANY " (Linux-specific)"
+Return a
+.I seminfo
+structure containing the same information as for
+.BR SEM_STAT .
+However, the
+.I sem_perm.mode
+is not checked for read access for
+.IR semid ,
+resembing the behaviour of
+/proc/sysvipc/sem.
 .B GETALL
 Return
 .B semval
@@ -367,7 +379,7 @@ the index of the highest used entry in the
 kernel's internal array recording information about all
 semaphore sets.
 (This information can be used with repeated
-.B SEM_STAT
+.B SEM_STAT or SEM_STAT_ANY
 operations to obtain information about all semaphore sets on the system.)
 .TP
 .B SEM_INFO
@@ -377,6 +389,10 @@ as for
 .B SEM_STAT
 the identifier of the semaphore set whose index was given in
 .IR semid .
+.TP
+.B SEM_STAT_ANY
+as for
+.BR SEM_STAT .
 .PP
 All other
 .I cmd
@@ -397,6 +413,7 @@ has one of the values
 .BR GETZCNT ,
 .BR IPC_STAT ,
 .BR SEM_STAT ,
+.BR SEM_STAT_ANY ,
 .BR SETALL ,
 or
 .B SETVAL
diff --git a/man2/shmctl.2 b/man2/shmctl.2
index 7bb503999941..42c47d9f4350 100644
--- a/man2/shmctl.2
+++ b/man2/shmctl.2
@@ -41,6 +41,7 @@
 .\" 2005-04-25, mtk -- noted aberrant Linux behavior w.r.t. new
 .\"	attaches to a segment that has already been marked for deletion.
 .\" 2005-08-02, mtk: Added IPC_INFO, SHM_INFO, SHM_STAT descriptions.
+.\" 2018-03-20, dbueso: Added SHM_STAT_ANY description.
 .\"
 .TH SHMCTL 2 2017-09-15 "Linux" "Linux Programmer's Manual"
 .SH NAME
@@ -242,6 +243,18 @@ However, the
 argument is not a segment identifier, but instead an index into
 the kernel's internal array that maintains information about
 all shared memory segments on the system.
+.TP
+.BR SHM_STAT_ANY " (Linux-specific)"
+Return a
+.I shmid_ds
+structure as for
+.BR SHM_STAT .
+However, the
+.I shm_perm.mode
+is not checked for read access for
+.IR shmid ,
+resembing the behaviour of
+/proc/sysvipc/shm.
 .PP
 The caller can prevent or allow swapping of a shared
 memory segment with the following \fIcmd\fP values:
@@ -287,7 +300,7 @@ operation returns the index of the highest used entry in the
 kernel's internal array recording information about all
 shared memory segments.
 (This information can be used with repeated
-.B SHM_STAT
+.B SHM_STAT or SHM_STAT_ANY
 operations to obtain information about all shared memory segments
 on the system.)
 A successful
@@ -328,7 +341,7 @@ isn't accessible.
 \fIshmid\fP is not a valid identifier, or \fIcmd\fP
 is not a valid command.
 Or: for a
-.B SHM_STAT
+.B SHM_STAT or SHM_STAT_ANY
 operation, the index value specified in
 .I shmid
 referred to an array slot that is currently unused.
-- 
2.13.6


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

* Re: [PATCH -next v2 0/3] sysvipc: introduce STAT_ANY commands
  2018-03-20 18:55 ` [PATCH -next v2 0/3] sysvipc: introduce STAT_ANY commands Davidlohr Bueso
@ 2018-11-04 16:29   ` Michael Kerrisk (man-pages)
  0 siblings, 0 replies; 7+ messages in thread
From: Michael Kerrisk (man-pages) @ 2018-11-04 16:29 UTC (permalink / raw)
  To: Davidlohr Bueso, akpm
  Cc: mtk.manpages, mhocko, robert.kettler, manfred, ebiederm,
	keescook, linux-api, linux-kernel, Joe Lawrence

Hello Davidlohr,

On 3/20/18 7:55 PM, Davidlohr Bueso wrote:
> On Thu, 15 Feb 2018, Davidlohr Bueso wrote:
> 
>> Once (if) merged, I will submit the necesary manpage updates. But I'm
>> thinking something like:
> 
> Hi Michael, here are the updated manpage entries. As always, please feel
> free to modify the descriptions as you see fit.

Thanks. Patch applied. Sorry for the delay...

Cheers,

Michael

> ------------8<----------------------------------------------------------
> [PATCH] sysvipc: add *_STAT_ANY command descriptions
> 
> The *ctl syscall descriptions have been updated to reflect the
> new commands in msg queues, semaphores and shmem.
> 
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> ---
>  man2/msgctl.2 | 20 +++++++++++++++++---
>  man2/semctl.2 | 19 ++++++++++++++++++-
>  man2/shmctl.2 | 17 +++++++++++++++--
>  3 files changed, 50 insertions(+), 6 deletions(-)
> 
> diff --git a/man2/msgctl.2 b/man2/msgctl.2
> index df74c750457a..31b1a1f12fb0 100644
> --- a/man2/msgctl.2
> +++ b/man2/msgctl.2
> @@ -33,6 +33,7 @@
>  .\"	Language and formatting clean-ups
>  .\"	Added msqid_ds and ipc_perm structure definitions
>  .\" 2005-08-02, mtk: Added IPC_INFO, MSG_INFO, MSG_STAT descriptions
> +.\" 2018-03-20, dbueso: Added MSG_STAT_ANY description.
>  .\"
>  .TH MSGCTL 2 2017-09-15 "Linux" "Linux Programmer's Manual"
>  .SH NAME
> @@ -222,10 +223,23 @@ Return a
>  structure as for
>  .BR IPC_STAT .
>  However, the
> -.I msqid
> +.I msgid
>  argument is not a queue identifier, but instead an index into
>  the kernel's internal array that maintains information about
>  all message queues on the system.
> +.TP
> +.BR MSG_STAT_ANY " (Linux-specific)"
> +Return a
> +.I msqid_ds
> +structure as for
> +.BR MSG_STAT .
> +However, the
> +.I msg_perm.mode
> +is not checked for read access for
> +.IR msqid ,
> +resembing the behaviour of
> +/proc/sysvipc/msg.
> +.PP
>  .SH RETURN VALUE
>  On success,
>  .BR IPC_STAT ,
> @@ -241,10 +255,10 @@ operation returns the index of the highest used entry in the
>  kernel's internal array recording information about all
>  message queues.
>  (This information can be used with repeated
> -.B MSG_STAT
> +.B MSG_STAT or MSG_STAT_ANY
>  operations to obtain information about all queues on the system.)
>  A successful
> -.B MSG_STAT
> +.B MSG_STAT or MSG_STAT_ANY
>  operation returns the identifier of the queue whose index was given in
>  .IR msqid .
>  .PP
> diff --git a/man2/semctl.2 b/man2/semctl.2
> index 02930d2c74ae..0ebe0434c05f 100644
> --- a/man2/semctl.2
> +++ b/man2/semctl.2
> @@ -37,6 +37,7 @@
>  .\"	Rewrote semun text
>  .\"	Added semid_ds and ipc_perm structure definitions
>  .\" 2005-08-02, mtk: Added IPC_INFO, SEM_INFO, SEM_STAT descriptions.
> +.\" 2018-03-20, dbueso: Added SEM_STAT_ANY description.
>  .\"
>  .TH SEMCTL 2 2017-09-15 "Linux" "Linux Programmer's Manual"
>  .SH NAME
> @@ -240,6 +241,17 @@ argument is not a semaphore identifier, but instead an index into
>  the kernel's internal array that maintains information about
>  all semaphore sets on the system.
>  .TP
> +.BR SEM_STAT_ANY " (Linux-specific)"
> +Return a
> +.I seminfo
> +structure containing the same information as for
> +.BR SEM_STAT .
> +However, the
> +.I sem_perm.mode
> +is not checked for read access for
> +.IR semid ,
> +resembing the behaviour of
> +/proc/sysvipc/sem.
>  .B GETALL
>  Return
>  .B semval
> @@ -367,7 +379,7 @@ the index of the highest used entry in the
>  kernel's internal array recording information about all
>  semaphore sets.
>  (This information can be used with repeated
> -.B SEM_STAT
> +.B SEM_STAT or SEM_STAT_ANY
>  operations to obtain information about all semaphore sets on the system.)
>  .TP
>  .B SEM_INFO
> @@ -377,6 +389,10 @@ as for
>  .B SEM_STAT
>  the identifier of the semaphore set whose index was given in
>  .IR semid .
> +.TP
> +.B SEM_STAT_ANY
> +as for
> +.BR SEM_STAT .
>  .PP
>  All other
>  .I cmd
> @@ -397,6 +413,7 @@ has one of the values
>  .BR GETZCNT ,
>  .BR IPC_STAT ,
>  .BR SEM_STAT ,
> +.BR SEM_STAT_ANY ,
>  .BR SETALL ,
>  or
>  .B SETVAL
> diff --git a/man2/shmctl.2 b/man2/shmctl.2
> index 7bb503999941..42c47d9f4350 100644
> --- a/man2/shmctl.2
> +++ b/man2/shmctl.2
> @@ -41,6 +41,7 @@
>  .\" 2005-04-25, mtk -- noted aberrant Linux behavior w.r.t. new
>  .\"	attaches to a segment that has already been marked for deletion.
>  .\" 2005-08-02, mtk: Added IPC_INFO, SHM_INFO, SHM_STAT descriptions.
> +.\" 2018-03-20, dbueso: Added SHM_STAT_ANY description.
>  .\"
>  .TH SHMCTL 2 2017-09-15 "Linux" "Linux Programmer's Manual"
>  .SH NAME
> @@ -242,6 +243,18 @@ However, the
>  argument is not a segment identifier, but instead an index into
>  the kernel's internal array that maintains information about
>  all shared memory segments on the system.
> +.TP
> +.BR SHM_STAT_ANY " (Linux-specific)"
> +Return a
> +.I shmid_ds
> +structure as for
> +.BR SHM_STAT .
> +However, the
> +.I shm_perm.mode
> +is not checked for read access for
> +.IR shmid ,
> +resembing the behaviour of
> +/proc/sysvipc/shm.
>  .PP
>  The caller can prevent or allow swapping of a shared
>  memory segment with the following \fIcmd\fP values:
> @@ -287,7 +300,7 @@ operation returns the index of the highest used entry in the
>  kernel's internal array recording information about all
>  shared memory segments.
>  (This information can be used with repeated
> -.B SHM_STAT
> +.B SHM_STAT or SHM_STAT_ANY
>  operations to obtain information about all shared memory segments
>  on the system.)
>  A successful
> @@ -328,7 +341,7 @@ isn't accessible.
>  \fIshmid\fP is not a valid identifier, or \fIcmd\fP
>  is not a valid command.
>  Or: for a
> -.B SHM_STAT
> +.B SHM_STAT or SHM_STAT_ANY
>  operation, the index value specified in
>  .I shmid
>  referred to an array slot that is currently unused.
> 


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

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

end of thread, other threads:[~2018-11-04 16:29 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-15 16:24 [PATCH -next v2 0/3] sysvipc: introduce STAT_ANY commands Davidlohr Bueso
2018-02-15 16:24 ` [PATCH 1/3] ipc/shm: introduce shmctl(SHM_STAT_ANY) Davidlohr Bueso
2018-02-20 10:13   ` Michal Hocko
2018-02-15 16:24 ` [PATCH 2/3] ipc/sem: introduce semctl(SEM_STAT_ANY) Davidlohr Bueso
2018-02-15 16:24 ` [PATCH 3/3] ipc/msg: introduce msgctl(MSG_STAT_ANY) Davidlohr Bueso
2018-03-20 18:55 ` [PATCH -next v2 0/3] sysvipc: introduce STAT_ANY commands Davidlohr Bueso
2018-11-04 16:29   ` Michael Kerrisk (man-pages)

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