BUG: possible NULL pointer dereference in nfs server

[Dimitry Andric]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 203 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.freebsd.org/pipermail/freebsd-fs/attachments/20140223/14c7afdd/attachment.sig>