ipc/shm: introduce shmctl(SHM_STAT_ANY)

Patch series "sysvipc: introduce STAT_ANY commands", v2.

The following patches adds the discussed (see [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 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.

This patch (of 3):

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.

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

Link: http://lkml.kernel.org/r/20180215162458.10059-2-dave@stgolabs.net
Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
Acked-by: Michal Hocko <mhocko@suse.com>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>
Cc: Manfred Spraul <manfred@colorfullife.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Robert Kettler <robert.kettler@outlook.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

diff --git a/include/uapi/linux/shm.h b/include/uapi/linux/shm.h
index 4de12a39b07506e04fff9d658144b2feee136d50..dde1344f047cf04c541a21f4d983c9eb3c606ef5 100644 (file)
--- 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 acefe44fefefa187c838c3830d944b5d3a4c047d..1a28b6a9644949ee7cc8b2886bfb092c1f56292c 100644 (file)
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -947,14 +947,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);
@@ -962,9 +962,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->shm_perm, cmd);
        if (err)
@@ -1104,6 +1115,7 @@ long ksys_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)
@@ -1282,6 +1294,7 @@ long compat_ksys_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 1eeb70e439d7999646d3fab9c38da5ea60cf1b60..1287013f747d89ecb228e6410ce91402aef60005 100644 (file)
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6157,6 +6157,7 @@ static int selinux_shm_shmctl(struct kern_ipc_perm *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 73549007bf9e63d75920376b71671d5bd55ca3f0..0daab3019023489a4122cb1254eb388f80b15f58 100644 (file)
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3046,6 +3046,7 @@ static int smack_shm_shmctl(struct kern_ipc_perm *isp, int cmd)
        switch (cmd) {
        case IPC_STAT:
        case SHM_STAT:
+       case SHM_STAT_ANY:
                may = MAY_READ;
                break;
        case IPC_SET: