BUG: possible NULL pointer dereference in nfs server

[John Baldwin]

On Tuesday, January 28, 2014 4:40:45 am Roman Divacky wrote:
> > > Yea, so long as it includes a comment that states this is done to
> > > work around a stupid compiler bug.
> > 
> > Ugh.
> > 
> > The above has the following bugs:
> > - gross style bugs (lines longer than 80 characters)
> > - large code to do nothing
> > - would be even larger with a comment
> > - cannot actually work around any compiler bug involving abort(), since it
> >    has no effect on production kernels where KASSERT() is null.
> > 
> > >> It is present even in your setup :) Just "objdump -d kernel | grep
> > >> ud2" on kernel compiled
> > >> by clang.
> > >>
> > > I actually use gcc, but I believe you. I'll admit I still don't understand
> > > why having a trap instruction that never gets executed is a bad thing?
> > 
> > It isn't but trying to link to the noexistent function abort() on
> > arches that don't have any trap instruction is a bad thing.  According
> > the above, this occurs on sparc64.  It must be a gcc bug, since sparc64
> > doesn't have clang.  However, I couldn't get gcc on sparc64 to generate
> > an abort() for *NULL.  Similarly on x86 (gcc is hard to test on x86,
> > since it is broken (not installed) on FreeBSD cluster machines for
> 
> I was testing clang compiled kernel on sparc64. 
> 
> The problem is that there's nothing making sure that the NULL pointer
> dereference doesnt happen. So if someone happens to call the function with
> wrong flags it will crash.
> 
> Thats why I want to add the KASSERT, to catch possible future cases
> when this happens.
> 
> Unfortunately our KASSERT is not an assert so to remove the actual
> abort/trap/ud2 I have to remove the flag.

Why not make a simple abort() that calls panic()?  It seems clumsy to have to
add hacks in the source code.

OTOH, the new_lfpp thing just seems to be obfuscation.  Seems you can remove
one layer of pointer there.  It doesn't help you with the compiler not being
able to see the invariant that prevents the problem though.

Index: nfs_nfsdstate.c
===================================================================
--- nfs_nfsdstate.c	(revision 261291)
+++ nfs_nfsdstate.c	(working copy)
@@ -79,7 +79,7 @@ static int nfsrv_getstate(struct nfsclient *clp, n
 static void nfsrv_getowner(struct nfsstatehead *hp, struct nfsstate *new_stp,
     struct nfsstate **stpp);
 static int nfsrv_getlockfh(vnode_t vp, u_short flags,
-    struct nfslockfile **new_lfpp, fhandle_t *nfhp, NFSPROC_T *p);
+    struct nfslockfile *new_lfp, fhandle_t *nfhp, NFSPROC_T *p);
 static int nfsrv_getlockfile(u_short flags, struct nfslockfile **new_lfpp,
     struct nfslockfile **lfpp, fhandle_t *nfhp, int lockit);
 static void nfsrv_insertlock(struct nfslock *new_lop,
@@ -1985,7 +1985,7 @@ tryagain:
 	MALLOC(new_lfp, struct nfslockfile *, sizeof (struct nfslockfile),
 	    M_NFSDLOCKFILE, M_WAITOK);
 	if (vp)
-		getfhret = nfsrv_getlockfh(vp, new_stp->ls_flags, &new_lfp,
+		getfhret = nfsrv_getlockfh(vp, new_stp->ls_flags, new_lfp,
 		    NULL, p);
 	NFSLOCKSTATE();
 	/*
@@ -2235,7 +2235,7 @@ tryagain:
 	    M_NFSDSTATE, M_WAITOK);
 	MALLOC(new_deleg, struct nfsstate *, sizeof (struct nfsstate),
 	    M_NFSDSTATE, M_WAITOK);
-	getfhret = nfsrv_getlockfh(vp, new_stp->ls_flags, &new_lfp,
+	getfhret = nfsrv_getlockfh(vp, new_stp->ls_flags, new_lfp,
 	    NULL, p);
 	NFSLOCKSTATE();
 	/*
@@ -3143,10 +3143,9 @@ out:
  */
 static int
 nfsrv_getlockfh(vnode_t vp, u_short flags,
-    struct nfslockfile **new_lfpp, fhandle_t *nfhp, NFSPROC_T *p)
+    struct nfslockfile *new_lfp, fhandle_t *nfhp, NFSPROC_T *p)
 {
 	fhandle_t *fhp = NULL;
-	struct nfslockfile *new_lfp;
 	int error;
 
 	/*
@@ -3154,7 +3153,6 @@ nfsrv_getlockfh(vnode_t vp, u_short flags,
 	 * a fhandle_t on the stack.
 	 */
 	if (flags & NFSLCK_OPEN) {
-		new_lfp = *new_lfpp;
 		fhp = &new_lfp->lf_fh;
 	} else if (nfhp) {
 		fhp = nfhp;


-- 
John Baldwin