Date: Sun, 23 Feb 2014 22:11:55 -0500 (EST) From: Rick Macklem <rmacklem@uoguelph.ca> To: Dimitry Andric <dim@FreeBSD.org> Cc: freebsd-fs@freebsd.org, Roman Divacky <rdivacky@freebsd.org> Subject: Re: BUG: possible NULL pointer dereference in nfs server Message-ID: <116945763.10073177.1393211515850.JavaMail.root@uoguelph.ca> In-Reply-To: <FA0EA9BA-66F6-4901-A220-8CB21F684238@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Dimitry Andric wrote:
> On 23 Feb 2014, at 22:33, Dimitry Andric <dim@freebsd.org> wrote:
> > On 01 Feb 2014, at 00:52, Rick Macklem <rmacklem@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?116945763.10073177.1393211515850.JavaMail.root>