From 68ff2a4397ed369d092bb453d6dcfaaca272cc5e Mon Sep 17 00:00:00 2001 From: "Christian S.J. Peron" Date: Thu, 15 Sep 2005 15:03:48 +0000 Subject: [PATCH] Improve the MP safeness associated with the creation of symbolic links and the execution of ELF binaries. Two problems were found: 1) The link path wasn't tagged as being MP safe and thus was not properly protected. 2) The ELF interpreter vnode wasnt being locked in namei(9) and thus was insufficiently protected. This commit makes the following changes: -Sets the MPSAFE flag in NDINIT for symbolic link paths -Sets the MPSAFE flag in NDINIT and introduce a vfslocked variable which will be used to instruct VFS_UNLOCK_GIANT to unlock Giant if it has been picked up. -Drop in an assertion into vfs_lookup which ensures that if the MPSAFE flag is NOT set, that we have picked up giant. If not panic (if WITNESS compiled into the kernel). This should help us find conditions where vnode operations are in-sufficiently protected. This is a RELENG_6 candidate. Discussed with: jeff MFC after: 4 days --- sys/kern/imgact_elf.c | 10 +++++++--- sys/kern/vfs_extattr.c | 2 +- sys/kern/vfs_lookup.c | 2 ++ sys/kern/vfs_syscalls.c | 2 +- 4 files changed, 11 insertions(+), 5 deletions(-) diff --git a/sys/kern/imgact_elf.c b/sys/kern/imgact_elf.c index 6a885006302..b469898359c 100644 --- a/sys/kern/imgact_elf.c +++ b/sys/kern/imgact_elf.c @@ -41,6 +41,7 @@ __FBSDID("$FreeBSD$"); #include #include #include +#include #include #include #include @@ -515,7 +516,7 @@ __elfN(load_file)(struct proc *p, const char *file, u_long *addr, vm_prot_t prot; u_long rbase; u_long base_addr = 0; - int error, i, numsegs; + int vfslocked, error, i, numsegs; if (curthread->td_proc != p) panic("elf_load_file - thread"); /* XXXKSE DIAGNOSTIC */ @@ -536,12 +537,14 @@ __elfN(load_file)(struct proc *p, const char *file, u_long *addr, imgp->execlabel = NULL; /* XXXKSE */ - NDINIT(nd, LOOKUP, LOCKLEAF|FOLLOW, UIO_SYSSPACE, file, curthread); - + NDINIT(nd, LOOKUP, MPSAFE|LOCKLEAF|FOLLOW, UIO_SYSSPACE, file, + curthread); + vfslocked = 0; if ((error = namei(nd)) != 0) { nd->ni_vp = NULL; goto fail; } + vfslocked = NDHASGIANT(nd); NDFREE(nd, NDF_ONLY_PNBUF); imgp->vp = nd->ni_vp; @@ -629,6 +632,7 @@ fail: if (nd->ni_vp) vrele(nd->ni_vp); + VFS_UNLOCK_GIANT(vfslocked); free(tempdata, M_TEMP); return (error); diff --git a/sys/kern/vfs_extattr.c b/sys/kern/vfs_extattr.c index 70f98127a4e..97533999164 100644 --- a/sys/kern/vfs_extattr.c +++ b/sys/kern/vfs_extattr.c @@ -1423,7 +1423,7 @@ kern_link(struct thread *td, char *path, char *link, enum uio_seg segflg) VFS_UNLOCK_GIANT(vfslocked); return (error); } - NDINIT(&nd, CREATE, LOCKPARENT | SAVENAME, segflg, link, td); + NDINIT(&nd, CREATE, LOCKPARENT | SAVENAME | MPSAFE, segflg, link, td); if ((error = namei(&nd)) == 0) { lvfslocked = NDHASGIANT(&nd); if (nd.ni_vp != NULL) { diff --git a/sys/kern/vfs_lookup.c b/sys/kern/vfs_lookup.c index 19dac650e75..4bb7656b4df 100644 --- a/sys/kern/vfs_lookup.c +++ b/sys/kern/vfs_lookup.c @@ -120,6 +120,8 @@ namei(ndp) struct proc *p = td->td_proc; int vfslocked; + KASSERT((cnp->cn_flags & MPSAFE) != 0 || mtx_owned(&Giant) != 0, + ("NOT MPSAFE and Giant not held")); ndp->ni_cnd.cn_cred = ndp->ni_cnd.cn_thread->td_ucred; KASSERT(cnp->cn_cred && p, ("namei: bad cred/proc")); KASSERT((cnp->cn_nameiop & (~OPMASK)) == 0, diff --git a/sys/kern/vfs_syscalls.c b/sys/kern/vfs_syscalls.c index 70f98127a4e..97533999164 100644 --- a/sys/kern/vfs_syscalls.c +++ b/sys/kern/vfs_syscalls.c @@ -1423,7 +1423,7 @@ kern_link(struct thread *td, char *path, char *link, enum uio_seg segflg) VFS_UNLOCK_GIANT(vfslocked); return (error); } - NDINIT(&nd, CREATE, LOCKPARENT | SAVENAME, segflg, link, td); + NDINIT(&nd, CREATE, LOCKPARENT | SAVENAME | MPSAFE, segflg, link, td); if ((error = namei(&nd)) == 0) { lvfslocked = NDHASGIANT(&nd); if (nd.ni_vp != NULL) {