Subject: Re: core file name format: diffs
To: None <tech-kern@netbsd.org>
From: Manuel Bouyer <bouyer@antioche.lip6.fr>
List: tech-kern
Date: 09/24/1999 00:36:05
--ReaqsoxgOBHFXBhH
Content-Type: text/plain; charset=us-ascii
On Wed, Sep 22, 1999 at 06:12:48PM -0400, der Mouse wrote:
> > + if (lim->pl_corename == defcorename) {
> > + newlim->pl_corename = lim->pl_corename;
> > + } else {
> > + newlim->pl_corename = malloc(strlen(lim->pl_corename)+1,
> > + M_TEMP, M_WAITOK);
> > + strcpy(newlim->pl_corename, lim->pl_corename);
> > + }
>
> I believe there is a race condition here.
>
> 1) Process A sets its corename.
> 2) Process A forks process B.
> 3) Process A changes a limit, causing limcopy() to be called.
> Postulate a resource shortage such that malloc() goes to sleep.
> Note (see dosetrlimit()) that p_refcnt is decremented first.
> 4) A and B still share the same p_limit.
> 5) Process B runs and changes its corename format to a longer string.
> Because p_refcnt is 1, this simply frees the old string and
> allocates a new one. (By this point the resource shortage is gone.)
> 6) Process A wakes up, finishes the malloc(), and strcpy()s
> lim->pl_corename - which has changed since it did the strlen() and
> went to sleep - into the space it allocated. This overruns the
> malloc()ed space. Not Good.
>
> No, it wouldn't happen often. But it would be a hell of a tough bug to
> find the once in a blue moon it did strike.
Hum, this can occur with a limit change as well, as limcopy() may sleep on the
plimit_pool pool too, and limcopy() will copy changed rlimits when it wakes up.
Is is less bad than copying overruning a malloc'd area, but still ...
We should decerase the ref_count *after* limcopy() as returned, once
we drop our reference to the rlimit (and this is good programing practice
in general: don't decrease a refcont while you still have a reference to
the shared struct, you may use it after :)
So I propose to move all (p->p_limit->p_refcnt--) to after the limcopy()
call, and change it to:
if (--p->p_limit->p_refcnt == 0)
pool_put(&plimit_pool, p->p_limit);
New diff appened below (only sys/kern, I didn't change anything else).
It compiles but I didn't boot the resulting kernel (it's late here :)
I'll test tomorow, or saturday. I'll commit this on monday if all problems
are sovled since then :)
Thanks again for looking at this, it's usefull :)
--
Manuel Bouyer, LIP6, Universite Paris VI. Manuel.Bouyer@lip6.fr
--
--ReaqsoxgOBHFXBhH
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="sysctl.diff4"
Index: init_main.c
===================================================================
RCS file: /cvsroot/syssrc/sys/kern/init_main.c,v
retrieving revision 1.156
diff -u -r1.156 init_main.c
--- init_main.c 1999/09/17 20:11:56 1.156
+++ init_main.c 1999/09/23 22:35:12
@@ -182,6 +182,7 @@
#if defined(NFSSERVER) || defined(NFS)
extern void nfs_init __P((void));
#endif
+ extern char defcorename[];
/*
* Initialize the current process pointer (curproc) before
@@ -286,6 +287,7 @@
limit0.pl_rlimit[RLIMIT_RSS].rlim_max = i;
limit0.pl_rlimit[RLIMIT_MEMLOCK].rlim_max = i;
limit0.pl_rlimit[RLIMIT_MEMLOCK].rlim_cur = i / 3;
+ limit0.pl_corename = defcorename;
limit0.p_refcnt = 1;
/*
Index: kern_exec.c
===================================================================
RCS file: /cvsroot/syssrc/sys/kern/kern_exec.c,v
retrieving revision 1.102
diff -u -r1.102 kern_exec.c
--- kern_exec.c 1999/08/09 02:42:20 1.102
+++ kern_exec.c 1999/09/23 22:35:12
@@ -457,7 +457,7 @@
p->p_ucred->cr_uid = attr.va_uid;
if (attr.va_mode & S_ISGID)
p->p_ucred->cr_gid = attr.va_gid;
- p->p_flag |= P_SUGID;
+ p_sugid(p);
} else
p->p_flag &= ~P_SUGID;
p->p_cred->p_svuid = p->p_ucred->cr_uid;
Index: kern_proc.c
===================================================================
RCS file: /cvsroot/syssrc/sys/kern/kern_proc.c,v
retrieving revision 1.34
diff -u -r1.34 kern_proc.c
--- kern_proc.c 1999/07/25 06:30:34 1.34
+++ kern_proc.c 1999/09/23 22:35:12
@@ -519,6 +519,29 @@
}
}
+/* mark process as suid/sgid, reset some values do defaults */
+void
+p_sugid(p)
+ struct proc *p;
+{
+ extern char defcorename[];
+
+ p->p_flag |= P_SUGID;
+ /* reset what needs to be reset in plimit */
+ if (p->p_limit->pl_corename != defcorename) {
+ if (p->p_limit->p_refcnt > 1 &&
+ (p->p_limit->p_lflags & PL_SHAREMOD) == 0) {
+ p->p_limit = limcopy(p->p_limit);
+ if (--p->p_limit->p_refcnt == 0)
+ pool_put(&plimit_pool, p->p_limit);
+ } else {
+ free(p->p_limit->pl_corename, M_TEMP);
+ }
+ p->p_limit->pl_corename = defcorename;
+ }
+}
+
+
#ifdef DEBUG
void
pgrpdump()
Index: kern_prot.c
===================================================================
RCS file: /cvsroot/syssrc/sys/kern/kern_prot.c,v
retrieving revision 1.54
diff -u -r1.54 kern_prot.c
--- kern_prot.c 1999/04/30 05:30:32 1.54
+++ kern_prot.c 1999/09/23 22:35:12
@@ -335,7 +335,7 @@
pc->pc_ucred->cr_uid = uid;
pc->p_ruid = uid;
pc->p_svuid = uid;
- p->p_flag |= P_SUGID;
+ p_sugid(p);
return (0);
}
@@ -363,7 +363,7 @@
*/
pc->pc_ucred = crcopy(pc->pc_ucred);
pc->pc_ucred->cr_uid = euid;
- p->p_flag |= P_SUGID;
+ p_sugid(p);
return (0);
}
@@ -408,7 +408,7 @@
}
if (euid != (uid_t)-1 && ruid != (uid_t)-1)
- p->p_flag |= P_SUGID;
+ p_sugid(p);
return (0);
}
@@ -434,7 +434,7 @@
pc->pc_ucred->cr_gid = gid;
pc->p_rgid = gid;
pc->p_svgid = gid;
- p->p_flag |= P_SUGID;
+ p_sugid(p);
return (0);
}
@@ -458,7 +458,7 @@
return (error);
pc->pc_ucred = crcopy(pc->pc_ucred);
pc->pc_ucred->cr_gid = egid;
- p->p_flag |= P_SUGID;
+ p_sugid(p);
return (0);
}
@@ -501,7 +501,7 @@
}
if (egid != (gid_t)-1 && rgid != (gid_t)-1)
- p->p_flag |= P_SUGID;
+ p_sugid(p);
return (0);
}
@@ -531,7 +531,7 @@
if (error)
return (error);
pc->pc_ucred->cr_ngroups = ngrp;
- p->p_flag |= P_SUGID;
+ p_sugid(p);
return (0);
}
Index: kern_resource.c
===================================================================
RCS file: /cvsroot/syssrc/sys/kern/kern_resource.c,v
retrieving revision 1.52
diff -u -r1.52 kern_resource.c
--- kern_resource.c 1999/07/25 06:30:34 1.52
+++ kern_resource.c 1999/09/23 22:35:12
@@ -225,17 +225,19 @@
error = copyin(SCARG(uap, rlp), &alim, sizeof(struct rlimit));
if (error)
return (error);
- return (dosetrlimit(p, which, &alim));
+ return (dosetrlimit(p, p->p_cred, which, &alim));
}
int
-dosetrlimit(p, which, limp)
+dosetrlimit(p, cred, which, limp)
struct proc *p;
+ struct pcred *cred;
int which;
struct rlimit *limp;
{
register struct rlimit *alimp;
extern unsigned maxdmap, maxsmap;
+ struct plimit *newplim;
int error;
if ((u_int)which >= RLIM_NLIMITS)
@@ -245,16 +247,23 @@
return (EINVAL);
alimp = &p->p_rlimit[which];
+ /* if we don't change the value, no need to limcopy() */
+ if (limp->rlim_cur == alimp->rlim_cur &&
+ limp->rlim_max == alimp->rlim_max)
+ return 0;
+
if (limp->rlim_cur > alimp->rlim_max ||
limp->rlim_max > alimp->rlim_max)
- if ((error = suser(p->p_ucred, &p->p_acflag)) != 0)
+ if ((error = suser(cred->pc_ucred, &p->p_acflag)) != 0)
return (error);
if (limp->rlim_cur > limp->rlim_max)
limp->rlim_cur = limp->rlim_max;
if (p->p_limit->p_refcnt > 1 &&
(p->p_limit->p_lflags & PL_SHAREMOD) == 0) {
- p->p_limit->p_refcnt--;
- p->p_limit = limcopy(p->p_limit);
+ newplim = limcopy(p->p_limit);
+ if (--p->p_limit->p_refcnt == 0)
+ pool_put(&plimit_pool, p->p_limit);
+ p->p_limit = newplim;
alimp = &p->p_rlimit[which];
}
@@ -454,10 +463,18 @@
struct plimit *lim;
{
register struct plimit *newlim;
+ extern char *defcorename;
newlim = pool_get(&plimit_pool, PR_WAITOK);
memcpy(newlim->pl_rlimit, lim->pl_rlimit,
sizeof(struct rlimit) * RLIM_NLIMITS);
+ if (lim->pl_corename == defcorename) {
+ newlim->pl_corename = lim->pl_corename;
+ } else {
+ newlim->pl_corename = malloc(strlen(lim->pl_corename)+1,
+ M_TEMP, M_WAITOK);
+ strcpy(newlim->pl_corename, lim->pl_corename);
+ }
newlim->p_lflags = 0;
newlim->p_refcnt = 1;
return (newlim);
Index: kern_sig.c
===================================================================
RCS file: /cvsroot/syssrc/sys/kern/kern_sig.c,v
retrieving revision 1.93
diff -u -r1.93 kern_sig.c
--- kern_sig.c 1999/08/31 12:30:35 1.93
+++ kern_sig.c 1999/09/23 22:35:12
@@ -79,6 +79,7 @@
void stop __P((struct proc *p));
void killproc __P((struct proc *, char *));
+static int build_corename __P((char *));
sigset_t contsigmask, stopsigmask, sigcantmask;
@@ -1264,9 +1265,8 @@
struct nameidata nd;
struct vattr vattr;
int error, error1;
- char name[MAXCOMLEN+6]; /* progname.core */
+ char name[MAXPATHLEN];
struct core core;
- extern int shortcorename;
/*
* Make sure the process has not set-id, to prevent data leaks.
@@ -1293,11 +1293,10 @@
(vp->v_mount->mnt_flag & MNT_NOCOREDUMP) != 0)
return (EPERM);
- if (shortcorename)
- sprintf(name, "core");
- else
- sprintf(name, "%s.core", p->p_comm);
-
+ error = build_corename(name);
+ if (error)
+ return error;
+
NDINIT(&nd, LOOKUP, NOFOLLOW, UIO_SYSSPACE, name, p);
error = vn_open(&nd, O_CREAT | FWRITE | FNOSYMLINK, S_IRUSR | S_IWUSR);
if (error)
@@ -1393,4 +1392,52 @@
psignal(p, SIGSYS);
return (ENOSYS);
+}
+
+static int
+build_corename(dst)
+ char *dst;
+{
+ const char *s;
+ char *d;
+ int len, i;
+
+ for (s = curproc->p_limit->pl_corename, len = 0, d = dst;
+ *s != '\0'; s++) {
+ if (*s == '%') {
+ switch (*(s+1)) {
+ case 'n':
+ i = snprintf(d,MAXPATHLEN - 1 - len, "%s",
+ curproc->p_comm);
+ break;
+ case 'p':
+ i = snprintf(d, MAXPATHLEN - 1 - len, "%d",
+ curproc->p_pid);
+ break;
+ case 'u':
+ i = snprintf(d, MAXPATHLEN - 1 - len, "%s",
+ curproc->p_pgrp->pg_session->s_login);
+ break;
+ case 't':
+ i = snprintf(d, MAXPATHLEN - 1 - len, "%ld",
+ curproc->p_stats->p_start.tv_sec);
+ break;
+ default:
+ goto copy;
+ }
+ if (i >= MAXPATHLEN - 1 - len)
+ return ENAMETOOLONG;
+ len += i;
+ d += i;
+ s++;
+ } else {
+copy: *d = *s;
+ d++;
+ len++;
+ if (len >= MAXPATHLEN - 1)
+ return ENAMETOOLONG;
+ }
+ }
+ *d = '\0';
+ return 0;
}
Index: kern_sysctl.c
===================================================================
RCS file: /cvsroot/syssrc/sys/kern/kern_sysctl.c,v
retrieving revision 1.50
diff -u -r1.50 kern_sysctl.c
--- kern_sysctl.c 1999/07/25 06:30:35 1.50
+++ kern_sysctl.c 1999/09/23 22:35:13
@@ -44,13 +44,14 @@
#include "opt_ddb.h"
#include "opt_insecure.h"
-#include "opt_shortcorename.h"
+#include "opt_defcorename.h"
#include "opt_sysv.h"
#include <sys/param.h>
#include <sys/systm.h>
#include <sys/kernel.h>
#include <sys/malloc.h>
+#include <sys/pool.h>
#include <sys/proc.h>
#include <sys/file.h>
#include <sys/vnode.h>
@@ -68,6 +69,8 @@
#include <sys/mount.h>
#include <sys/syscallargs.h>
+#include <sys/resource.h>
+#include <sys/resourcevar.h>
#if defined(DDB)
@@ -102,9 +105,6 @@
sysctlfn *fn;
int name[CTL_MAXNAME];
- if (SCARG(uap, new) != NULL &&
- (error = suser(p->p_ucred, &p->p_acflag)))
- return (error);
/*
* all top-level sysctl names are non-terminal
*/
@@ -115,6 +115,15 @@
if (error)
return (error);
+ /*
+ * For all but CTL_PROC, must be root to change a value.
+ * For CTL_PROC, must be root, or owner of the proc (and not suid),
+ * this is checked in proc_sysctl() (once we know the targer proc).
+ */
+ if (SCARG(uap, new) != NULL && name[0] != CTL_PROC &&
+ (error = suser(p->p_ucred, &p->p_acflag)))
+ return error;
+
switch (name[0]) {
case CTL_KERN:
fn = kern_sysctl;
@@ -146,6 +155,9 @@
fn = ddb_sysctl;
break;
#endif
+ case CTL_PROC:
+ fn = proc_sysctl;
+ break;
default:
return (EOPNOTSUPP);
}
@@ -210,10 +222,12 @@
#else
int securelevel = 0;
#endif
-#ifdef SHORTCORENAME
-int shortcorename = 1;
+#ifdef DEFCORENAME
+char defcorename[MAXPATHLEN] = DEFCORENAME;
+int defcorenamelen = sizeof(DEFCORENAME);
#else
-int shortcorename = 0;
+char defcorename[MAXPATHLEN] = "%n.core";
+int defcorenamelen = sizeof("%n.core");
#endif
/*
@@ -232,7 +246,6 @@
int error, level, inthostid;
int old_autonicetime;
int old_vnodes;
- int old_shortcorename;
extern char ostype[], osrelease[], version[];
/* All sysctl names at this level, except for a few, are terminal. */
@@ -304,7 +317,7 @@
case KERN_VNODE:
return (sysctl_vnode(oldp, oldlenp, p));
case KERN_PROC:
- return (sysctl_doproc(name + 1, namelen - 1, oldp, oldlenp));
+ return (sysctl_doeproc(name + 1, namelen - 1, oldp, oldlenp));
case KERN_FILE:
return (sysctl_file(oldp, oldlenp));
#ifdef GPROF
@@ -380,16 +393,14 @@
#else
return (sysctl_rdint(oldp, oldlenp, newp, 0));
#endif
- case KERN_SHORTCORENAME:
- /* Only allow values of zero or one. */
- old_shortcorename = shortcorename;
- error = sysctl_int(oldp, oldlenp, newp, newlen,
- &shortcorename);
- if (shortcorename != 0 && shortcorename != 1) {
- shortcorename = old_shortcorename;
- return (EINVAL);
- }
- return (error);
+ case KERN_DEFCORENAME:
+ if (newp && newlen < 1)
+ return (EINVAL);
+ error = sysctl_string(oldp, oldlenp, newp, newlen,
+ defcorename, sizeof(defcorename));
+ if (newp && !error)
+ defcorenamelen = newlen;
+ return (error);
case KERN_SYNCHRONIZED_IO:
return (sysctl_rdint(oldp, oldlenp, newp, 1));
case KERN_IOV_MAX:
@@ -498,6 +509,153 @@
}
#endif /* DEBUG */
+int
+proc_sysctl(name, namelen, oldp, oldlenp, newp, newlen, p)
+ int *name;
+ u_int namelen;
+ void *oldp;
+ size_t *oldlenp;
+ void *newp;
+ size_t newlen;
+ struct proc *p;
+{
+ struct proc *ptmp;
+ const struct proclist_desc *pd;
+ int error = 0;
+ struct rlimit alim;
+ struct plimit *newplim;
+ char *tmps = NULL;
+ int i, curlen, len;
+
+ if (namelen < 2)
+ return EINVAL;
+
+ if (name[0] == PROC_CURPROC) {
+ ptmp = p;
+ } else {
+ proclist_lock_read();
+ for (pd = proclists; pd->pd_list != NULL; pd++) {
+ for (ptmp = LIST_FIRST(pd->pd_list); ptmp != NULL;
+ ptmp = LIST_NEXT(ptmp, p_list)) {
+ /* Skip embryonic processes. */
+ if (ptmp->p_stat == SIDL)
+ continue;
+ if (ptmp->p_pid == (pid_t)name[0])
+ break;
+ }
+ if (ptmp != NULL)
+ break;
+ }
+ proclist_unlock_read();
+ if (ptmp == NULL)
+ return(ESRCH);
+ if (p->p_ucred->cr_uid != 0) {
+ if(p->p_cred->p_ruid != ptmp->p_cred->p_ruid ||
+ p->p_cred->p_ruid != ptmp->p_cred->p_svuid)
+ return EPERM;
+ if (ptmp->p_cred->p_rgid != ptmp->p_cred->p_svgid)
+ return EPERM; /* sgid proc */
+ for (i = 0; i < p->p_ucred->cr_ngroups; i++) {
+ if (p->p_ucred->cr_groups[i] ==
+ ptmp->p_cred->p_rgid)
+ break;
+ }
+ if (i == p->p_ucred->cr_ngroups)
+ return EPERM;
+ }
+ }
+ if (name[1] == PROC_PID_CORENAME) {
+ if (namelen != 2)
+ return EINVAL;
+ /*
+ * Can't use sysctl_string() here because we may malloc a new
+ * area during the process, so we have to do it by hand.
+ */
+ curlen = strlen(ptmp->p_limit->pl_corename) + 1;
+ if (oldp && *oldlenp < curlen)
+ return (ENOMEM);
+ if (newp) {
+ if (securelevel > 2)
+ return EPERM;
+ if (newlen > MAXPATHLEN)
+ return ENAMETOOLONG;
+ tmps = malloc(newlen + 1, M_TEMP, M_WAITOK);
+ if (tmps == NULL)
+ return ENOMEM;
+ error = copyin(newp, tmps, newlen + 1);
+ tmps[newlen] = '\0';
+ if (error)
+ goto cleanup;
+ /* Enforce to be either 'core' for end with '.core' */
+ if (newlen < 4) { /* c.o.r.e */
+ error = EINVAL;
+ goto cleanup;
+ }
+ len = newlen - 4;
+ if (len > 0) {
+ if (tmps[len - 1] != '.' &&
+ tmps[len - 1] != '/') {
+ error = EINVAL;
+ goto cleanup;
+ }
+ }
+ if (strcmp(&tmps[len], "core") != 0) {
+ error = EINVAL;
+ goto cleanup;
+ }
+ }
+ if (oldp) {
+ *oldlenp = curlen;
+ error = copyout(ptmp->p_limit->pl_corename, oldp,
+ curlen);
+ }
+ if (newp && error == 0) {
+ /* if the 2 strings are identical, don't limcopy() */
+ if (strcmp(tmps, ptmp->p_limit->pl_corename) == 0) {
+ error = 0;
+ goto cleanup;
+ }
+ if (ptmp->p_limit->p_refcnt > 1 &&
+ (ptmp->p_limit->p_lflags & PL_SHAREMOD) == 0) {
+ newplim = limcopy(ptmp->p_limit);
+ if (--p->p_limit->p_refcnt == 0)
+ pool_put(&plimit_pool, p->p_limit);
+ p->p_limit = newplim;
+ } else if (ptmp->p_limit->pl_corename != defcorename) {
+ free(ptmp->p_limit->pl_corename, M_TEMP);
+ }
+ ptmp->p_limit->pl_corename = tmps;
+ return (0);
+ }
+cleanup:
+ if (tmps)
+ free(tmps, M_TEMP);
+ return (error);
+ }
+ if (name[1] == PROC_PID_LIMIT) {
+ if (namelen != 4 || name[2] >= PROC_PID_LIMIT_MAXID)
+ return EINVAL;
+ memcpy(&alim, &ptmp->p_rlimit[name[2] - 1], sizeof(alim));
+ if (name[3] == PROC_PID_LIMIT_TYPE_HARD)
+ error = sysctl_quad(oldp, oldlenp, newp, newlen,
+ &alim.rlim_max);
+ else if (name[3] == PROC_PID_LIMIT_TYPE_SOFT)
+ error = sysctl_quad(oldp, oldlenp, newp, newlen,
+ &alim.rlim_cur);
+ else
+ error = EINVAL;
+
+ if (error)
+ return error;
+
+ if (newp)
+ error = dosetrlimit(ptmp, p->p_cred,
+ name[2] - 1, &alim);
+ return error;
+ }
+ return (EINVAL);
+}
+
/*
* Validate parameters and get old / set new parameters
* for an integer-valued sysctl function.
@@ -548,6 +706,55 @@
/*
* Validate parameters and get old / set new parameters
+ * for an quad-valued sysctl function.
+ */
+int
+sysctl_quad(oldp, oldlenp, newp, newlen, valp)
+ void *oldp;
+ size_t *oldlenp;
+ void *newp;
+ size_t newlen;
+ quad_t *valp;
+{
+ int error = 0;
+
+ if (oldp && *oldlenp < sizeof(quad_t))
+ return (ENOMEM);
+ if (newp && newlen != sizeof(quad_t))
+ return (EINVAL);
+ *oldlenp = sizeof(quad_t);
+ if (oldp)
+ error = copyout(valp, oldp, sizeof(quad_t));
+ if (error == 0 && newp)
+ error = copyin(newp, valp, sizeof(quad_t));
+ return (error);
+}
+
+/*
+ * As above, but read-only.
+ */
+int
+sysctl_rdquad(oldp, oldlenp, newp, val)
+ void *oldp;
+ size_t *oldlenp;
+ void *newp;
+ quad_t val;
+{
+ int error = 0;
+
+ if (oldp && *oldlenp < sizeof(quad_t))
+ return (ENOMEM);
+ if (newp)
+ return (EPERM);
+ *oldlenp = sizeof(quad_t);
+ if (oldp)
+ error = copyout((caddr_t)&val, oldp, sizeof(quad_t));
+ return (error);
+}
+
+
+/*
+ * Validate parameters and get old / set new parameters
* for a string-valued sysctl function.
*/
int
@@ -709,7 +916,7 @@
#define KERN_PROCSLOP (5 * sizeof(struct kinfo_proc))
int
-sysctl_doproc(name, namelen, where, sizep)
+sysctl_doeproc(name, namelen, where, sizep)
int *name;
u_int namelen;
char *where;
--ReaqsoxgOBHFXBhH--