[PATCH -next 0/3] sysvipc: introduce STAT_ALL commands

[PATCH -next 0/3] sysvipc: introduce STAT_ALL commands

[Davidlohr Bueso]

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_ALL 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_ALL " (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_ALL
 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_ALL
 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

Davidlohr Bueso (3):
  ipc/shm: introduce shmctl(SHM_STAT_ALL)
  ipc/sem: introduce shmctl(SEM_STAT_ALL)
  ipc/msg: introduce shmctl(MSG_STAT_ALL)

 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 +++
 7 files changed, 50 insertions(+), 17 deletions(-)

-- 
2.13.6

[PATCH 1/3] ipc/shm: introduce shmctl(SHM_STAT_ALL)

[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_ALL 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 +
 3 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/include/uapi/linux/shm.h b/include/uapi/linux/shm.h
index 4de12a39b075..017284fca194 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_ALL    15
 
 /* Obsolete, used only for backwards compatibility */
 struct	shminfo {
diff --git a/ipc/shm.c b/ipc/shm.c
index 4643865e9171..a84b45b4f50f 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_ALL) {
 		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_ALL 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_ALL)
+		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_ALL:
 	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_ALL:
 	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..dc2a9a0f6ddf 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_ALL:
 		perms = SHM__GETATTR | SHM__ASSOCIATE;
 		break;
 	case IPC_SET:
-- 
2.13.6

[PATCH 2/3] ipc/sem: introduce shmctl(SEM_STAT_ALL)

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

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 include/uapi/linux/sem.h |  1 +
 ipc/sem.c                | 17 ++++++++++++-----
 security/selinux/hooks.c |  1 +
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/include/uapi/linux/sem.h b/include/uapi/linux/sem.h
index 9c3e745b0656..fde93f635660 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_ALL 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..cb2737c7f1d6 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_ALL) {
 		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_ALL */
+	if (cmd == SEM_STAT_ALL)
+		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_ALL:
 		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_ALL:
 		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 dc2a9a0f6ddf..aa965742bba9 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_ALL:
 		perms = SEM__GETATTR | SEM__ASSOCIATE;
 		break;
 	default:
-- 
2.13.6

[PATCH 2/3] ipc/sem: introduce shmctl(SEM_STAT_ALL)

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

Signed-off-by: Davidlohr Bueso <dbueso-l3A5Bk7waGM@public.gmane.org>
---
 include/uapi/linux/sem.h |  1 +
 ipc/sem.c                | 17 ++++++++++++-----
 security/selinux/hooks.c |  1 +
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/include/uapi/linux/sem.h b/include/uapi/linux/sem.h
index 9c3e745b0656..fde93f635660 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_ALL 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..cb2737c7f1d6 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_ALL) {
 		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_ALL */
+	if (cmd == SEM_STAT_ALL)
+		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_ALL:
 		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_ALL:
 		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 dc2a9a0f6ddf..aa965742bba9 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_ALL:
 		perms = SEM__GETATTR | SEM__ASSOCIATE;
 		break;
 	default:
-- 
2.13.6

[PATCH 3/3] ipc/msg: introduce shmctl(MSG_STAT_ALL)

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

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 include/uapi/linux/msg.h |  1 +
 ipc/msg.c                | 17 ++++++++++++-----
 security/selinux/hooks.c |  1 +
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/include/uapi/linux/msg.h b/include/uapi/linux/msg.h
index 5d5ab81dc9be..fe9bdfb25a2f 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_ALL 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..533c9ac2fb13 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_ALL) {
 		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_ALL */
+	if (cmd == MSG_STAT_ALL)
+		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_ALL:
 	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_ALL:
 		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 aa965742bba9..457e80f6b4e7 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_ALL:
 		perms = MSGQ__GETATTR | MSGQ__ASSOCIATE;
 		break;
 	case IPC_SET:
-- 
2.13.6

Re: [PATCH -next 0/3] sysvipc: introduce STAT_ALL commands

[Eric W. Biederman]

Davidlohr Bueso <dave@stgolabs.net> writes:

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

I am just going to kibitz for a moment.

Could we name this _STAT_ANY or _STAT_NOPERM instead of _STAT_ALL.

I keep thinking a name with _ALL in it should affect all ipc opbjects of
a given type, not simply work any ipc object regardless of permissions.

Eric

> 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_ALL 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_ALL " (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_ALL
>  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_ALL
>  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
>
> Davidlohr Bueso (3):
>   ipc/shm: introduce shmctl(SHM_STAT_ALL)
>   ipc/sem: introduce shmctl(SEM_STAT_ALL)
>   ipc/msg: introduce shmctl(MSG_STAT_ALL)
>
>  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 +++
>  7 files changed, 50 insertions(+), 17 deletions(-)

Re: [PATCH -next 0/3] sysvipc: introduce STAT_ALL commands

[Davidlohr Bueso]

On Tue, 13 Feb 2018, Eric W. Biederman wrote:

>Davidlohr Bueso <dave@stgolabs.net> writes:
>
>> 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:
>
>I am just going to kibitz for a moment.

Nice word that, kibitz.

>
>Could we name this _STAT_ANY or _STAT_NOPERM instead of _STAT_ALL.
>
>I keep thinking a name with _ALL in it should affect all ipc opbjects of
>a given type, not simply work any ipc object regardless of permissions.

Yeah, and _ALL similarly makes the difference of IPC_STAT and SHM_STAT wrt
the passed shmid that more adhoc.

I'm going to go with _STAT_ANY.

Thanks,
Davidlohr

Re: [PATCH -next 0/3] sysvipc: introduce STAT_ALL commands

[Davidlohr Bueso]

On Tue, 13 Feb 2018, Eric W. Biederman wrote:

>Davidlohr Bueso <dave-h16yJtLeMjHk1uMJSBkQmQ@public.gmane.org> writes:
>
>> 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:
>
>I am just going to kibitz for a moment.

Nice word that, kibitz.

>
>Could we name this _STAT_ANY or _STAT_NOPERM instead of _STAT_ALL.
>
>I keep thinking a name with _ALL in it should affect all ipc opbjects of
>a given type, not simply work any ipc object regardless of permissions.

Yeah, and _ALL similarly makes the difference of IPC_STAT and SHM_STAT wrt
the passed shmid that more adhoc.

I'm going to go with _STAT_ANY.

Thanks,
Davidlohr

Re: [PATCH -next 0/3] sysvipc: introduce STAT_ALL commands

[Andrew Morton]

On Tue, 13 Feb 2018 09:41:33 -0800 Davidlohr Bueso <dave@stgolabs.net> wrote:

> 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:
> 
> ...
> 
> [1] https://lkml.org/lkml/2017/12/19/220

It would be nice to summarize the above discussion right here, rather
than merely linking to an email thread.

A reported-by:mhocko would be appropriate.

Really, the only reason for this patchset is to speed up the userspace
interface, yes?  But the changelog is awfull skimpy on the details
here.  Who cares about it and why and which apps can be changed (ipcs?)
and why do we care and how much better does it get, etc.


Dumb question: an admin can do `chmod 0400 /proc/sysvipc/shm' and then
this data is hidden from unprivileged users.  So doesn't this change
represent a security hole for such users?

Re: [PATCH -next 0/3] sysvipc: introduce STAT_ALL commands

[Davidlohr Bueso]

On Tue, 13 Feb 2018, Andrew Morton wrote:

>On Tue, 13 Feb 2018 09:41:33 -0800 Davidlohr Bueso <dave@stgolabs.net> wrote:
>
>> 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:
>>
>> ...
>>
>> [1] https://lkml.org/lkml/2017/12/19/220
>
>It would be nice to summarize the above discussion right here, rather
>than merely linking to an email thread.

I would have thought (1) and (2) above sumarize the situation. Patches
themselves also have a thorough changelog.

>
>A reported-by:mhocko would be appropriate.

This was really reported by a customer, but sure.

>
>Really, the only reason for this patchset is to speed up the userspace
>interface, yes?  But the changelog is awfull skimpy on the details
>here.  Who cares about it and why and which apps can be changed (ipcs?)
>and why do we care and how much better does it get, etc.

Yes, better performance is the goal here. The customer application is
a large and well known database vendor. When consulted as to why this
functionality was needed (and why the process couldn't be pimped to
CAP_IPC_OWNER):

""
The databaselayer saves its rowstore for some reason inside shared memory
(so large amounts of shared memory). The DB instances communicate with each
other about how much memory(ANON, but not shared) each instance is using.

Due to the nature of shared memory one instance might remove a segment,
but it will never know when it is realy gone unless we ask the kernel
to give us the correct information. When this happens we would be fine
to tell others, but to speed things up we currenty allow each instance
to calculate the shared memory size on their own.
""

As such these very specific workloads that deal with large (+10k) number
of ipc objects (shared memory segments in this case).

>
>
>Dumb question: an admin can do `chmod 0400 /proc/sysvipc/shm' and then
>this data is hidden from unprivileged users.  So doesn't this change
>represent a security hole for such users?

Sure, but you'd even hide data that the user can legitimately have
available (the same data that SHM_STAT would return). This deals with
permissions at an ipc object granularity, not file.

In any case, doing a 0400 on the file would cause 'ipcs -m' would not
show anything for other users.

Thanks,
Davidlohr

Re: [PATCH -next 0/3] sysvipc: introduce STAT_ALL commands

[Davidlohr Bueso]

On Tue, 13 Feb 2018, Andrew Morton wrote:

>On Tue, 13 Feb 2018 09:41:33 -0800 Davidlohr Bueso <dave-h16yJtLeMjHk1uMJSBkQmQ@public.gmane.org> wrote:
>
>> 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:
>>
>> ...
>>
>> [1] https://lkml.org/lkml/2017/12/19/220
>
>It would be nice to summarize the above discussion right here, rather
>than merely linking to an email thread.

I would have thought (1) and (2) above sumarize the situation. Patches
themselves also have a thorough changelog.

>
>A reported-by:mhocko would be appropriate.

This was really reported by a customer, but sure.

>
>Really, the only reason for this patchset is to speed up the userspace
>interface, yes?  But the changelog is awfull skimpy on the details
>here.  Who cares about it and why and which apps can be changed (ipcs?)
>and why do we care and how much better does it get, etc.

Yes, better performance is the goal here. The customer application is
a large and well known database vendor. When consulted as to why this
functionality was needed (and why the process couldn't be pimped to
CAP_IPC_OWNER):

""
The databaselayer saves its rowstore for some reason inside shared memory
(so large amounts of shared memory). The DB instances communicate with each
other about how much memory(ANON, but not shared) each instance is using.

Due to the nature of shared memory one instance might remove a segment,
but it will never know when it is realy gone unless we ask the kernel
to give us the correct information. When this happens we would be fine
to tell others, but to speed things up we currenty allow each instance
to calculate the shared memory size on their own.
""

As such these very specific workloads that deal with large (+10k) number
of ipc objects (shared memory segments in this case).

>
>
>Dumb question: an admin can do `chmod 0400 /proc/sysvipc/shm' and then
>this data is hidden from unprivileged users.  So doesn't this change
>represent a security hole for such users?

Sure, but you'd even hide data that the user can legitimately have
available (the same data that SHM_STAT would return). This deals with
permissions at an ipc object granularity, not file.

In any case, doing a 0400 on the file would cause 'ipcs -m' would not
show anything for other users.

Thanks,
Davidlohr