Date: Fri, 24 Jan 2014 19:38:35 -0500 (EST) From: Rick Macklem <rmacklem@uoguelph.ca> To: Roman Divacky <rdivacky@freebsd.org> Cc: fs@freebsd.org Subject: Re: BUG: possible NULL pointer dereference in nfs server Message-ID: <1577222508.16050114.1390610315654.JavaMail.root@uoguelph.ca> In-Reply-To: <20140124184141.GA19458@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Roman Divacky wrote:
> Hi,
>
> In nfs_nfsdstate.c:nfsrv_lockctrl() we call
>
> getlckret = nfsrv_getlockfh(vp, new_stp->ls_flags, NULL, &nfh, p);
>
> then in nfsrv_getlockfh() we, based on the value of flags, might
> dereference the NULL pointer:
>
>
> nfsrv_getlockfh(vnode_t vp, u_short flags,
> struct nfslockfile **new_lfpp, fhandle_t *nfhp, NFSPROC_T *p)
>
>
> if (flags & NFSLCK_OPEN) {
> new_lfp = *new_lfpp;
> fhp = &new_lfp->lf_fh;
>
>
I took a look and don't see a problem. NFSLCK_OPEN is only set for Opens
{ nfsrvd_open() } and it never calls nfsrv_lockctrl(). nfsrv_lockctrl() is
always called with other flags in new_stp->ls_flags set, but never NFSLCK_OPEN,
because nfsrv_lockctrl() is handling byte range lock cases after a file has been
opened { which means the "else" case for this "if" always applies.
You could add a KASSERT() like:
KASSERT((new_stp->ls_flags & NFSLCK_OPEN) == 0, ("nfsrv_lockctrl: calling nfsrv_getlockfh with NFSLCK_OPEN"));
just before the nfsrv_getlockfh() call if you'd like, but I'd be very surprised
if this ever happens. It wouldn't make sense to call nfsrv_lockctrl() with NFSLCK_OPEN
and I can't see anywhere it the code that it would do this. (And I've never seen
a report of a crash that this would have caused.)
> I am not sure what the right fix is. Or if it's even possible to hit
> (but I think it is). Anyway the compiler currently generates
> a trap instruction (ud2 on x86) in this code. It's the only trap
> in GENERIC btw.
>
Sorry, I'm not a compiler guy, so I don't know why a compiler would
generate a trap instruction, but since new_lfpp is never NULL when
this is executed, I don't see a problem.
If others feel that this needs to be re-coded, please let me know what
you think the code should look like? (A test for non-NULL with a panic()
before it is used?)
Is a trap instruction that never gets executed a problem?
rick
> Would be lovely to fix this.
>
> Roman
>
> P.S. CC me on your replies as I am not subscribed to the list.
>
> _______________________________________________
> freebsd-fs@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-fs
> To unsubscribe, send any mail to "freebsd-fs-unsubscribe@freebsd.org"
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1577222508.16050114.1390610315654.JavaMail.root>