Prior to support for almost all ps activity via sysctl, ps used procfs,

and so special-casing was introduced to provide extra procfs privilege
to the kmem group.  With the advent of non-setgid kmem ps, this code
is no longer required, and in fact, can is potentially harmful as it
allocates privilege to a gid that is increasingly less meaningful.
Knowledge of specific gid's in kernel is also generally bad precedent,
as the kernel security policy doesn't distinguish gid's specifically,
only uid 0.

This commit removes reference to kmem in procfs, both in terms of
access control decisions, and the applying of gid kmem to the
/proc/*/mem file, simplifying the associated code considerably.
Processes are still permitted to access the mem file based on
the debugging policy, so ps -e still works fine for normal
processes and use.

Reviewed by:	tmm
Obtained from:	TrustedBSD Project

3 files changed, 9 insertions, 42 deletions

diff --git a/sys/fs/procfs/procfs.h b/sys/fs/procfs/procfs.h
index 1c8e5e1..1882be6 100644
--- a/sys/fs/procfs/procfs.h
+++ b/sys/fs/procfs/procfs.h
@@ -88,8 +88,6 @@ struct pfsnode {
 	 ((cnp)->cn_namelen == (len) && \
 	  (bcmp((s), (cnp)->cn_nameptr, (len)) == 0))
 
-#define KMEM_GROUP 2
-
 #define PROCFS_FILENO(pid, type) \
 	(((type) < Pproc) ? \
 			((type) + 2) : \
diff --git a/sys/fs/procfs/procfs_mem.c b/sys/fs/procfs/procfs_mem.c
index dcba9b0..29773a0 100644
--- a/sys/fs/procfs/procfs_mem.c
+++ b/sys/fs/procfs/procfs_mem.c
@@ -247,26 +247,14 @@ procfs_domem(curp, p, pfs, uio)
 	struct pfsnode *pfs;
 	struct uio *uio;
 {
+	int error;
 
 	if (uio->uio_resid == 0)
 		return (0);
 
- 	/*
- 	 * XXX
- 	 * We need to check for KMEM_GROUP because ps is sgid kmem;
- 	 * not allowing it here causes ps to not work properly.  Arguably,
- 	 * this is a bug with what ps does.  We only need to do this
- 	 * for Pmem nodes, and only if it's reading.  This is still not
- 	 * good, as it may still be possible to grab illicit data if
- 	 * a process somehow gets to be KMEM_GROUP.  Note that this also
- 	 * means that KMEM_GROUP can't change without editing procfs.h!
- 	 * All in all, quite yucky.
- 	 */
- 
- 	if (p_candebug(curp, p) &&
-	    !(uio->uio_rw == UIO_READ &&
-	      procfs_kmemaccess(curp)))
- 		return EPERM;
+	error = p_candebug(curp, p);
+	if (error)
+		return (error);
 
 	return (procfs_rwmem(curp, p, uio));
 }
@@ -303,21 +291,3 @@ procfs_findtextvp(p)
 
 	return (p->p_textvp);
 }
-
-int procfs_kmemaccess(curp)
-	struct proc *curp;
-{
-	int i;
-	struct ucred *cred;
-
-	cred = curp->p_ucred;
-	if (suser(curp))
-		return 1;
-
-	/* XXX: Why isn't this done with file-perms ??? */
-	for (i = 0; i < cred->cr_ngroups; i++)
-		if (cred->cr_groups[i] == KMEM_GROUP)	
-			return 1;
-	
-	return 0;
-}
diff --git a/sys/fs/procfs/procfs_vnops.c b/sys/fs/procfs/procfs_vnops.c
index bc280c78..9efe6d66 100644
--- a/sys/fs/procfs/procfs_vnops.c
+++ b/sys/fs/procfs/procfs_vnops.c
@@ -157,10 +157,9 @@ procfs_open(ap)
 		}
 
 		p1 = ap->a_p;
-		if (p_candebug(p1, p2) &&
-		    !procfs_kmemaccess(p1)) {
-			error = EPERM;
-		}
+		error = p_candebug(p1, p2);
+		if (error)
+			return (error);
 
 		if (ap->a_mode & FWRITE)
 			pfs->pfs_flags = ap->a_mode & (FWRITE|O_EXCL);
@@ -456,7 +455,6 @@ procfs_getattr(ap)
 					  ((VREAD|VWRITE)>>6));
 		break;
 	case Pmem:
-		/* Retain group kmem readablity. */
 		PROC_LOCK(procp);
 		if (procp->p_flag & P_SUGID)
 			vap->va_mode &= ~(VREAD|VWRITE);
@@ -528,6 +526,8 @@ procfs_getattr(ap)
 		 * If we denied owner access earlier, then we have to
 		 * change the owner to root - otherwise 'ps' and friends
 		 * will break even though they are setgid kmem. *SIGH*
+		 * XXX: ps and friends are no longer setgid kmem, why
+		 * is this needed?
 		 */
 		PROC_LOCK(procp);
 		if (procp->p_flag & P_SUGID)
@@ -535,7 +535,6 @@ procfs_getattr(ap)
 		else
 			vap->va_uid = procp->p_ucred->cr_uid;
 		PROC_UNLOCK(procp);
-		vap->va_gid = KMEM_GROUP;
 		break;
 
 	case Pregs: