Re: bug report: xfrm: potential null deref in xfrm_bundle_lookup()

[Dan Carpenter <error27@gmail.com>]

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