BUG: possible NULL pointer dereference in nfs server

[Dimitry Andric]

On 25 Jan 2014, at 01:38, Rick Macklem <rmacklem at uoguelph.ca> wrote:
> 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.

The point is that the compiler cannot always know this, when it inlines
nfsrv_getlockfh().  In the first invocation of nfsrv_getlockfh(), there
is no problem:

   712  APPLESTATIC void
   713  nfsrv_dumplocks(vnode_t vp, struct nfsd_dumplocks *ldumpp, int maxcnt,
   714      NFSPROC_T *p)
   715  {
...
   726          ret = nfsrv_getlockfh(vp, 0, NULL, &nfh, p);

Here both the 'flags' and 'new_lfpp' arguments are zero, so the part in
nfsrv_getlockfh() where it writes to a NULL pointer can never be
executed, therefore it can be optimized away safely.

In the next invocation, the state of the 'flags' argument is not known
to the compiler, however:

  1369  tryagain:
  1370          if (new_stp->ls_flags & NFSLCK_LOCK)
  1371                  MALLOC(other_lop, struct nfslock *, sizeof (struct nfslock),
  1372                      M_NFSDLOCK, M_WAITOK);
...
  1387          getlckret = nfsrv_getlockfh(vp, new_stp->ls_flags, NULL, &nfh, p);

If it inlines this, the result looks approximately like:

     1	{
     2		fhandle_t *fhp = NULL;
     3		struct nfslockfile *new_lfp;
     4		int error;
     5
     6		if (new_stp->ls_flags & NFSLCK_OPEN) {
     7			new_lfp = *NULL;
     8			fhp = &new_lfp->lf_fh;
     9		} else if (&nfh) {
    10			fhp = &nfh;
    11		} else {
    12			panic("nfsrv_getlockfh");
    13		}
    14		error = nfsvno_getfh(vp, fhp, p);
    15		NFSEXITCODE(error);
    16		getlckret = error;
    17	}

The code in line 7 is the problematic part.  Since this is undefined,
the compiler inserts a trap instruction here.  I think the problem Roman
encountered is that on sparc64, there is no equivalent to x86's ud2
instruction, so it inserts a call to abort() instead, and that function
is not available in kernel-land.


...
> 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?

It's better to avoid undefined behavior in any case.  Just add a NULL
check, that should be sufficient.

-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/20140125/2984eeb2/attachment.sig>