NetBSD Problem Report #38336

From martin@duskware.de  Sat Mar 29 23:13:57 2008
Return-Path: <martin@duskware.de>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by narn.NetBSD.org (Postfix) with ESMTP id A0A6863B8B8
	for <gnats-bugs@gnats.netbsd.org>; Sat, 29 Mar 2008 23:13:57 +0000 (UTC)
Message-Id: <20080329151037.C211963B863@narn.NetBSD.org>
Date: Sat, 29 Mar 2008 15:10:37 +0000 (UTC)
From: ad@netbsd.org
Reply-To: ad@netbsd.org
To: netbsd-bugs-owner@NetBSD.org
Subject: NULL deref in nfs_lookup
X-Send-Pr-Version: www-1.0

>Number:         38336
>Category:       kern
>Synopsis:       NULL deref in nfs_lookup
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Mar 29 23:15:00 +0000 2008
>Last-Modified:  Mon Mar 31 02:50:04 +0000 2008
>Originator:     Andrew Doran
>Release:        4.99.58
>Organization:
The NetBSD Project
>Environment:
4.99.58 + local changes to socket code
>Description:
nfs_vnops.c:

    925 	nfsm_request(np, NFSPROC_LOOKUP, curlwp, cnp->cn_cred);
    926 	if (error) {
    927 		nfsm_postop_attr(dvp, attrflag, 0);
    928 		m_freem(mrep);
    929 		goto nfsmout;
    930 	}

nfsm_request() fills 'error' and typically 'md'. In case of error,
'md' is sometimes not filled and can contain junk from the stack,
but nfsm_postop_attr() assumes that 'md' is always filled. Here is 
nfsm_postop_attr() partially decrypted:

        struct vnode *ttvp = dvp;
        {
                t1 = ((char *) ((md)->m_hdr.mh_data)) + md->m_hdr.mh_len - dpos;
                if (t1 >= (4) && 1) {
                        (tl) = (u_int32_t *) (dpos);
                        dpos += (4);
                } else if ((t1 = nfsm_disct(&md, &dpos, (4), t1, &cp2)) != 0) {
                        error = t1;
                        m_freem(mrep);
                        goto nfsmout; 
                } else {
                        (tl) = (u_int32_t *) cp2;
                } }; if (((attrflag) = ((int)
        (__builtin_constant_p(((__uint32_t) ((__int32_t) (*tl)))) ?
        (((((__uint32_t) ((__int32_t) (*tl))) & 0xff000000) >> 24) |
        ((((__uint32_t) ((__int32_t) (*tl))) & 0x00ff0000) >> 8) |
        ((((__uint32_t) ((__int32_t) (*tl))) & 0x0000ff00) << 8) |
        ((((__uint32_t) ((__int32_t) (*tl))) & 0x000000ff) << 24)) :
        __byte_swap_u32_variable((__uint32_t) ((__int32_t) (*tl)))))) != 0)
        {
                if ((t1 = nfsm_loadattrcache(&ttvp, &md, &dpos, (struct vattr *) 0, (0))) != 0) {
                        error = t1;
                        (attrflag) = 0;
                        m_freem(mrep);
                        goto nfsmout;
                }
                (dvp) = ttvp;
        }
        m_freem(mrep);
        goto nfsmout;



>How-To-Repeat:
Make nfsm_request() fail.

>Fix:
Not yet known.

>Audit-Trail:
From: yamt@mwd.biglobe.ne.jp (YAMAMOTO Takashi)
To: gnats-bugs@NetBSD.org
Cc: kern-bug-people@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: kern/38336: NULL deref in nfs_lookup
Date: Sun, 30 Mar 2008 22:55:03 +0900 (JST)

 > nfs_vnops.c:
 > 
 >     925 	nfsm_request(np, NFSPROC_LOOKUP, curlwp, cnp->cn_cred);
 >     926 	if (error) {
 >     927 		nfsm_postop_attr(dvp, attrflag, 0);
 >     928 		m_freem(mrep);
 >     929 		goto nfsmout;
 >     930 	}
 > 
 > nfsm_request() fills 'error' and typically 'md'. In case of error,
 > 'md' is sometimes not filled and can contain junk from the stack,
 > but nfsm_postop_attr() assumes that 'md' is always filled. Here is 

 can you be specific about "sometimes"?
 unless NFSERR_RETERR is set, nfsm_request macro itself does "goto nfsmout"
 so "if (error)" in the above code is not executed.

 YAMAMOTO Takashi

From: Andrew Doran <ad@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/38336: NULL deref in nfs_lookup
Date: Sun, 30 Mar 2008 15:55:06 +0100

 On Sun, Mar 30, 2008 at 02:00:05PM +0000, YAMAMOTO Takashi wrote:

 >  > nfs_vnops.c:
 >  > 
 >  >     925 	nfsm_request(np, NFSPROC_LOOKUP, curlwp, cnp->cn_cred);
 >  >     926 	if (error) {
 >  >     927 		nfsm_postop_attr(dvp, attrflag, 0);
 >  >     928 		m_freem(mrep);
 >  >     929 		goto nfsmout;
 >  >     930 	}
 >  > 
 >  > nfsm_request() fills 'error' and typically 'md'. In case of error,
 >  > 'md' is sometimes not filled and can contain junk from the stack,
 >  > but nfsm_postop_attr() assumes that 'md' is always filled. Here is 
 >  
 >  can you be specific about "sometimes"?
 >  unless NFSERR_RETERR is set, nfsm_request macro itself does "goto nfsmout"
 >  so "if (error)" in the above code is not executed.

 I had a broken sbwait() that returned ERESTART when it should have been
 ignoring signals. I haven't checked to see if the error can happen in
 -current, but it seems better to handle it. I think the return path for the
 error code was:

 sbwait -> soreceive -> nfs_receive -> nfs_reply -> nfs_request -> nfs_lookup

 Andrew

From: yamt@mwd.biglobe.ne.jp (YAMAMOTO Takashi)
To: gnats-bugs@NetBSD.org
Cc: kern-bug-people@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org,
        ad@netbsd.org
Subject: Re: kern/38336: NULL deref in nfs_lookup
Date: Mon, 31 Mar 2008 11:46:58 +0900 (JST)

 >  On Sun, Mar 30, 2008 at 02:00:05PM +0000, YAMAMOTO Takashi wrote:
 >  
 >  >  > nfs_vnops.c:
 >  >  > 
 >  >  >     925 	nfsm_request(np, NFSPROC_LOOKUP, curlwp, cnp->cn_cred);
 >  >  >     926 	if (error) {
 >  >  >     927 		nfsm_postop_attr(dvp, attrflag, 0);
 >  >  >     928 		m_freem(mrep);
 >  >  >     929 		goto nfsmout;
 >  >  >     930 	}
 >  >  > 
 >  >  > nfsm_request() fills 'error' and typically 'md'. In case of error,
 >  >  > 'md' is sometimes not filled and can contain junk from the stack,
 >  >  > but nfsm_postop_attr() assumes that 'md' is always filled. Here is 
 >  >  
 >  >  can you be specific about "sometimes"?
 >  >  unless NFSERR_RETERR is set, nfsm_request macro itself does "goto nfsmout"
 >  >  so "if (error)" in the above code is not executed.
 >  
 >  I had a broken sbwait() that returned ERESTART when it should have been
 >  ignoring signals. I haven't checked to see if the error can happen in
 >  -current, but it seems better to handle it. I think the return path for the
 >  error code was:
 >  
 >  sbwait -> soreceive -> nfs_receive -> nfs_reply -> nfs_request -> nfs_lookup
 >  
 >  Andrew

 my understanding is that, if nfs_reply returns an error,
 nfs_request doesn't set NFSERR_RETERR, thus the above nfsm_postop_attr
 is not executed.

 YAMAMOTO Takashi

Home	PR Database Search