From owner-freebsd-fs@FreeBSD.ORG Sun Feb 23 21:49:04 2014 Return-Path: Delivered-To: freebsd-fs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 1C91E494; Sun, 23 Feb 2014 21:49:04 +0000 (UTC) Received: from tensor.andric.com (tensor.andric.com [87.251.56.140]) (using TLSv1 with cipher ADH-CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id A9F2618FD; Sun, 23 Feb 2014 21:49:03 +0000 (UTC) Received: from [IPv6:2001:7b8:3a7::2953:bff3:cb76:c707] (unknown [IPv6:2001:7b8:3a7:0:2953:bff3:cb76:c707]) (using TLSv1 with cipher AES128-SHA (128/128 bits)) (No client certificate requested) by tensor.andric.com (Postfix) with ESMTPSA id 825125C45; Sun, 23 Feb 2014 22:49:00 +0100 (CET) Content-Type: multipart/signed; boundary="Apple-Mail=_6EB85032-F88F-4335-91CA-89D723CDADB4"; protocol="application/pgp-signature"; micalg=pgp-sha1 Mime-Version: 1.0 (Mac OS X Mail 7.1 \(1827\)) Subject: Re: BUG: possible NULL pointer dereference in nfs server From: Dimitry Andric In-Reply-To: <3B667D71-2ED6-4EBF-A586-297BEDC9499A@FreeBSD.org> Date: Sun, 23 Feb 2014 22:48:45 +0100 Message-Id: References: <346483760.1090608.1391212324309.JavaMail.root@uoguelph.ca> <3B667D71-2ED6-4EBF-A586-297BEDC9499A@FreeBSD.org> To: Rick Macklem X-Mailer: Apple Mail (2.1827) Cc: freebsd-fs@freebsd.org, Roman Divacky X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 23 Feb 2014 21:49:04 -0000 --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 wrote: > On 01 Feb 2014, at 00:52, Rick Macklem 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--