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
(Contact us)
$NetBSD: query-full-pr,v 1.39 2013/11/01 18:47:49 spz Exp $
$NetBSD: gnats_config.sh,v 1.8 2006/05/07 09:23:38 tsutsui Exp $
Copyright © 1994-2007
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.