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

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

[Dan Carpenter]

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.

  1680                  return oldflo;
  1681          }

regards,
dan carpenter

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

[Timo Teräs]

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.

It might make this more obvious if we tested xdst for NULL instead of
oldflo, though.

- Timo

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

[Dan Carpenter]

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