BUG: possible NULL pointer dereference in nfs server

[Rick Macklem]

Dimitry Andric wrote:
> On 23 Feb 2014, at 22:33, Dimitry Andric <dim at freebsd.org> wrote:
> > On 01 Feb 2014, at 00:52, Rick Macklem <rmacklem at uoguelph.ca>
> > wrote:
> >> John Baldwin wrote:
> > ...
> >>> 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;
> >>> 
> >>> 
> >> Yep, this looks good to me, although I have no idea if it makes
> >> the
> >> compiler happy?
> > 
> > It seems to, though I think it could still crash if it ever got its
> > flags in an unexpected state, while new_lfp is NULL.  Let's just
> > hope
> > that never happens.
> > 
> > So can we commit jhb's patch now?  That would be very handy for my
> > clang-sparc64 project branch. :)
> 
> Alternatively, just this simple fix:
> 
> Index: sys/fs/nfsserver/nfs_nfsdstate.c
> ===================================================================
> --- sys/fs/nfsserver/nfs_nfsdstate.c    (revision 262397)
> +++ sys/fs/nfsserver/nfs_nfsdstate.c    (working copy)
> @@ -3154,6 +3154,9 @@ nfsrv_getlockfh(vnode_t vp, u_short flags,
>          * a fhandle_t on the stack.
>          */
>         if (flags & NFSLCK_OPEN) {
> +               if (new_lfpp == NULL) {
> +                       panic("nfsrv_getlockfh");
> +               }
>                 new_lfp = *new_lfpp;
>                 fhp = &new_lfp->lf_fh;
>         } else if (nfhp) {
> 
> If new_lfpp is really never going to be NULL the panic will never be
> hit.  If the "impossible" happens anyway, you will have a nice panic.
> 
> No undefined behavior anywhere anymore, and no need for abort()
> implementations. :-)
> 
> -Dimitry
> 
> 
I have no strong opinion on this, but I do think jhb@'s patch cleans up
the code (and I think I mentioned before that I didn't know if it made the
compiler happy).

Personally, I'd suggest jhb@'s patch plus whatever it takes to make clang
happy.

I can't do commits until mid-April, so I'd suggest you guys commit whatever
you think is appropriate.

rick