Date: Sun, 23 Feb 2014 22:48:45 +0100 From: Dimitry Andric <dim@FreeBSD.org> To: Rick Macklem <rmacklem@uoguelph.ca> Cc: freebsd-fs@freebsd.org, Roman Divacky <rdivacky@freebsd.org> Subject: Re: BUG: possible NULL pointer dereference in nfs server Message-ID: <FA0EA9BA-66F6-4901-A220-8CB21F684238@FreeBSD.org> In-Reply-To: <3B667D71-2ED6-4EBF-A586-297BEDC9499A@FreeBSD.org> References: <346483760.1090608.1391212324309.JavaMail.root@uoguelph.ca> <3B667D71-2ED6-4EBF-A586-297BEDC9499A@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--Apple-Mail=_6EB85032-F88F-4335-91CA-89D723CDADB4
Content-Transfer-Encoding: 7bit
Content-Type: text/plain;
charset=us-ascii
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
--Apple-Mail=_6EB85032-F88F-4335-91CA-89D723CDADB4
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename=signature.asc
Content-Type: application/pgp-signature;
name=signature.asc
Content-Description: Message signed with OpenPGP using GPGMail
-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.22 (Darwin)
iEYEARECAAYFAlMKbMUACgkQsF6jCi4glqPtJQCbBd3jHAnFjF0olEh7Oq+6HIS/
G/4AoPaAKm92JuBSlBd/Gw14rw0SLnMY
=I+2a
-----END PGP SIGNATURE-----
--Apple-Mail=_6EB85032-F88F-4335-91CA-89D723CDADB4--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?FA0EA9BA-66F6-4901-A220-8CB21F684238>