Hi,
The following diff tries to simplify a bit the entanglement of namei
data and execve(2) syscall.
Currently, when called, sys_execve() might doing recursive calls of
check_exec() with a shared `struct nameidata' (`ep_ndp' inside
`struct exec_package').
I would like to disassociate them, and make check_exec() to "own" the
`struct nameidata' data. execve(2) is complex enough, no needs to adds
namei() complexity inside.
check_exec() will initialize nameidata, call namei() (as now), extract
useful information for the caller (only the vnode and the
command-name, returned via exec_package struct), and free other namei
ressources.
To proceed, it only needs to know the wanted path (to properly init
`nd' with NDINIT).
As the call of check_exec() could be recursive (when scripts are
involved), the path could come from:
- directly from sys_execve() via SCARG(uap, path) (from userspace)
- from exec_script_makecmds() via the shellname (from systemspace)
In `struct exec_package', I opted to reuse `ep_name' for passing the
path: it is what sys_execve() is already doing at first call. Later it
is only used for construct the fake-args list in
exec_script_makecmds(), and it could being overrided after that (and
restored on error). I reordered them a bit to make it fit.
Eventually I could also introduce a new struct field for the wanted
path.
`ep_segflg' is introduced to mark if ep_name comes from UIO_USERSPACE
or UIO_SYSSPACE. it will be used by NDINIT() in check_exec().
`ep_comm' is the other information (with ep_vp) wanted in result of
check_exec() call. it will be copied to `ps_comm'.
Comments or OK ?
--
Sebastien Marie
diff 8de782433999755fc9356c8ed8dc7b327e532351 /home/semarie/repos/openbsd/src
blob - a31fa5a3269a3b568450066a1bd001317d710354
file + sys/kern/exec_script.c
--- sys/kern/exec_script.c
+++ sys/kern/exec_script.c
@@ -64,19 +64,24 @@ exec_script_makecmds(struct proc *p, struct exec_packa
{
int error, hdrlinelen, shellnamelen, shellarglen;
char *hdrstr = epp->ep_hdr;
- char *cp, *shellname, *shellarg, *oldpnbuf;
+ char *cp, *shellname, *shellarg;
char **shellargp = NULL, **tmpsap;
struct vnode *scriptvp;
+ char old_comm[MAXCOMLEN+1];
+ char *old_name;
+ enum uio_seg old_segflg;
uid_t script_uid = -1;
gid_t script_gid = -1;
u_short script_sbits;
/*
- * remember the old vp and pnbuf for later, so we can restore
+ * remember the old epp values for later, so we can restore
* them if check_exec() fails.
*/
+ old_segflg = epp->ep_segflg;
+ old_name = epp->ep_name;
scriptvp = epp->ep_vp;
- oldpnbuf = epp->ep_ndp->ni_cnd.cn_pnbuf;
+ strlcpy(old_comm, epp->ep_comm, sizeof(old_comm));
/*
* if the magic isn't that of a shell script, or we've already
@@ -186,13 +191,7 @@ check_shell:
FRELE(fp, p);
}
- /* set up the parameters for the recursive check_exec() call */
- epp->ep_ndp->ni_dirfd = AT_FDCWD;
- epp->ep_ndp->ni_dirp = shellname;
- epp->ep_ndp->ni_segflg = UIO_SYSSPACE;
- epp->ep_flags |= EXEC_INDIR;
-
- /* and set up the fake args list, for later */
+ /* set up the fake args list, for later */
shellargp = mallocarray(4, sizeof(char *), M_EXEC, M_WAITOK);
tmpsap = shellargp;
*tmpsap = malloc(shellnamelen + 1, M_EXEC, M_WAITOK);
@@ -214,6 +213,11 @@ check_shell:
tmpsap++;
*tmpsap = NULL;
+ /* set up the parameters for the recursive check_exec() call */
+ epp->ep_segflg = UIO_SYSSPACE;
+ epp->ep_name = shellname;
+ epp->ep_flags |= EXEC_INDIR;
+
/*
* mark the header we have as invalid; check_exec will read
* the header from the new executable
@@ -233,9 +237,6 @@ check_shell:
if ((epp->ep_flags & EXEC_HASFD) == 0)
vn_close(scriptvp, FREAD, p->p_ucred, p);
- /* free the old pathname buffer */
- pool_put(&namei_pool, oldpnbuf);
-
epp->ep_flags |= (EXEC_HASARGL | EXEC_SKIPARG);
epp->ep_fa = shellargp;
/*
@@ -250,8 +251,13 @@ check_shell:
return (0);
}
- /* XXX oldpnbuf not set for "goto fail" path */
- epp->ep_ndp->ni_cnd.cn_pnbuf = oldpnbuf;
+ /*
+ * no need to restore these ep_* in 'fail' as there were not
+ * changed at this point
+ */
+ epp->ep_segflg = old_segflg;
+ epp->ep_name = old_name;
+ strlcpy(epp->ep_comm, old_comm, sizeof(epp->ep_comm));
fail:
/* note that we've clobbered the header */
epp->ep_flags |= EXEC_DESTR;
@@ -265,8 +271,6 @@ fail:
} else
vn_close(scriptvp, FREAD, p->p_ucred, p);
- pool_put(&namei_pool, epp->ep_ndp->ni_cnd.cn_pnbuf);
-
/* free the fake arg list, because we're not returning it */
if (shellargp != NULL) {
free(shellargp[0], M_EXEC, shellnamelen + 1);
blob - 674c62a53ed99e4beb9afc311f3bc01a0046c8fc
file + sys/kern/kern_exec.c
--- sys/kern/kern_exec.c
+++ sys/kern/kern_exec.c
@@ -121,19 +121,28 @@ check_exec(struct proc *p, struct exec_package *epp)
{
int error, i;
struct vnode *vp;
- struct nameidata *ndp;
- size_t resid;
+ struct nameidata nd;
+ size_t resid, len;
- ndp = epp->ep_ndp;
- ndp->ni_cnd.cn_nameiop = LOOKUP;
- ndp->ni_cnd.cn_flags = FOLLOW | LOCKLEAF | SAVENAME;
+ NDINIT(&nd, LOOKUP, FOLLOW | LOCKLEAF | SAVENAME,
+ epp->ep_segflg, epp->ep_name, p);
+ nd.ni_pledge = PLEDGE_EXEC;
+ nd.ni_unveil = UNVEIL_EXEC;
+
if (epp->ep_flags & EXEC_INDIR)
- ndp->ni_cnd.cn_flags |= BYPASSUNVEIL;
+ nd.ni_cnd.cn_flags |= BYPASSUNVEIL;
+
/* first get the vnode */
- if ((error = namei(ndp)) != 0)
+ if ((error = namei(&nd)) != 0)
return (error);
- epp->ep_vp = vp = ndp->ni_vp;
+ epp->ep_vp = vp = nd.ni_vp;
+ /* copy the executable name and free namei buffer */
+ memset(epp->ep_comm, 0, sizeof(epp->ep_comm));
+ len = min(nd.ni_cnd.cn_namelen, MAXCOMLEN);
+ memcpy(epp->ep_comm, nd.ni_cnd.cn_nameptr, len);
+ pool_put(&namei_pool, nd.ni_cnd.cn_pnbuf);
+
/* check for regular file */
if (vp->v_type != VREG) {
error = EACCES;
@@ -222,18 +231,15 @@ check_exec(struct proc *p, struct exec_package *epp)
bad2:
/*
- * close the vnode, free the pathname buf, and punt.
+ * close the vnode, and punt.
*/
vn_close(vp, FREAD, p->p_ucred, p);
- pool_put(&namei_pool, ndp->ni_cnd.cn_pnbuf);
return (error);
bad1:
/*
- * free the namei pathname buffer, and put the vnode
- * (which we don't yet have open).
+ * put the vnode (which we don't yet have open).
*/
- pool_put(&namei_pool, ndp->ni_cnd.cn_pnbuf);
vput(vp);
return (error);
}
@@ -251,7 +257,6 @@ sys_execve(struct proc *p, void *v, register_t *retval
} */ *uap = v;
int error;
struct exec_package pack;
- struct nameidata nid;
struct vattr attr;
struct ucred *cred = p->p_ucred;
char *argp;
@@ -281,18 +286,15 @@ sys_execve(struct proc *p, void *v, register_t *retval
*/
atomic_setbits_int(&pr->ps_flags, PS_INEXEC);
- NDINIT(&nid, LOOKUP, NOFOLLOW, UIO_USERSPACE, SCARG(uap, path), p);
- nid.ni_pledge = PLEDGE_EXEC;
- nid.ni_unveil = UNVEIL_EXEC;
-
/*
* initialize the fields of the exec package.
*/
+ pack.ep_segflg = UIO_USERSPACE;
pack.ep_name = (char *)SCARG(uap, path);
pack.ep_hdr = malloc(exec_maxhdrsz, M_EXEC, M_WAITOK);
pack.ep_hdrlen = exec_maxhdrsz;
pack.ep_hdrvalid = 0;
- pack.ep_ndp = &nid;
+ pack.ep_vp = NULL;
pack.ep_interp = NULL;
pack.ep_emul_arg = NULL;
VMCMDSET_INIT(&pack.ep_vmcmds);
@@ -507,8 +509,7 @@ sys_execve(struct proc *p, void *v, register_t *retval
/* set command name & other accounting info */
memset(pr->ps_comm, 0, sizeof(pr->ps_comm));
- len = min(nid.ni_cnd.cn_namelen, MAXCOMLEN);
- memcpy(pr->ps_comm, nid.ni_cnd.cn_nameptr, len);
+ strlcpy(pr->ps_comm, pack.ep_comm, sizeof(pr->ps_comm));
pr->ps_acflag &= ~AFORK;
/* record proc's vnode, for use by sysctl */
@@ -665,7 +666,6 @@ sys_execve(struct proc *p, void *v, register_t *retval
km_free(argp, NCARGS, &kv_exec, &kp_pageable);
- pool_put(&namei_pool, nid.ni_cnd.cn_pnbuf);
vn_close(pack.ep_vp, FREAD, cred, p);
/*
@@ -736,7 +736,6 @@ bad:
free(pack.ep_emul_arg, M_TEMP, pack.ep_emul_argsize);
/* close and put the exec'd file */
vn_close(pack.ep_vp, FREAD, cred, p);
- pool_put(&namei_pool, nid.ni_cnd.cn_pnbuf);
km_free(argp, NCARGS, &kv_exec, &kp_pageable);
freehdr:
@@ -757,7 +756,6 @@ exec_abort:
pool_put(&namei_pool, pack.ep_interp);
if (pack.ep_emul_arg != NULL)
free(pack.ep_emul_arg, M_TEMP, pack.ep_emul_argsize);
- pool_put(&namei_pool, nid.ni_cnd.cn_pnbuf);
vn_close(pack.ep_vp, FREAD, cred, p);
km_free(argp, NCARGS, &kv_exec, &kp_pageable);
blob - e64674347a5fd8f06b9a39a4c9c0fb5fe4b4794d
file + sys/sys/exec.h
--- sys/sys/exec.h
+++ sys/sys/exec.h
@@ -112,11 +112,12 @@ struct exec_vmcmd_set {
};
struct exec_package {
+ enum uio_seg ep_segflg; /* ep_name location */
char *ep_name; /* file's name */
void *ep_hdr; /* file's exec header */
u_int ep_hdrlen; /* length of ep_hdr */
u_int ep_hdrvalid; /* bytes of ep_hdr that are valid */
- struct nameidata *ep_ndp; /* namei data pointer for lookups */
+ char ep_comm[MAXCOMLEN+1]; /* command name */
struct exec_vmcmd_set ep_vmcmds; /* vmcmds used to build vmspace */
struct vnode *ep_vp; /* executable's vnode */
struct vattr *ep_vap; /* executable's attributes */
