netdev - Re: bug report: xfrm: potential null deref in xfrm_bundle

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Sun, 23 May 2010 21:38:07 +0200
From:	Dan Carpenter <error27@...il.com>
To:	Timo Teräs <timo.teras@....fi>
Cc:	netdev@...r.kernel.org
Subject: Re: bug report: xfrm: potential null deref in xfrm_bundle_lookup()

On Sun, May 23, 2010 at 09:06:31PM +0300, Timo Teräs wrote:
> On 05/22/2010 11:24 PM, Dan Carpenter wrote:
> > This is a smatch thing.  I couldn't tell if it was a real issue so I
> > thought I would send this mail to the experts.  :)
> > 
> > net/xfrm/xfrm_policy.c +1679 xfrm_bundle_lookup(51)
> > 	error: we previously assumed 'xdst' could be null.
> >   1672          new_xdst = xfrm_resolve_and_create_bundle(pols, num_pols, fl, family, dst_orig);
> >   1673          if (IS_ERR(new_xdst)) {
> >   1674                  err = PTR_ERR(new_xdst);
> >   1675                  if (err != -EAGAIN)
> >   1676                          goto error;
> >   1677                  if (oldflo == NULL)
> >   1678                          goto make_dummy_bundle;
> >   1679                  dst_hold(&xdst->u.dst);
> >                                   ^^^^^^^^^^^
> > 	Can xdst be NULL here?  It would have to be something like
> > 	oldflo gets passed in as null and __xfrm_policy_lookup() fails.
> 
> No. xdst and oldflo point to same data structure, just to different
> offset (and data type). If oldflo is not null, xdst is not either. See
> their initialization around lines 1640. Since oldflo is explicitly
> tested for not being null, xdst is valid too.
> 

Yeah yeah.  I'm a dummy.  From my email if "oldflo gets passed in as
null" then we hit the goto.  I was one small step away from understanding
it on my own.

Smatch actually would get this right if it understood that
container_of() basically never returns null.  It's a kind of grizzly
macro but I'll see if I can fix smatch to support that.

thanks again,
dan

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html