Re: [PATCH] staging: rts5208: add check on NULL before dereference

[Dan Carpenter <dan.carpenter@oracle.com>]

On Sat, Jun 09, 2018 at 10:34:43PM +0300, Andy Shevchenko wrote:
> On Sat, Jun 9, 2018 at 7:58 PM,  <okaya@codeaurora.org> wrote:
> > On 2018-06-09 12:38, Anton Vasilyev wrote:
> >>
> >> If rtsx_probe fails to allocate dev->chip, then NULL pointer
> >> dereference occurs at rtsx_release_resources().
> >>
> >> Patch adds checks chip on NULL before its dereference at
> >> rtsx_release_resources and passing with dereference inside
> >> rtsx_release_chip.
> >>
> >> Found by Linux Driver Verification project (linuxtesting.org).
> 
> > I think you should bail out if dev->chip is null rather than adding
> > conditiinals.
> 
> I'm wondering if it's false positive. At which circumstances that may happen?
> 


Here's how the code looks like in rtsx_probe().

   972          /* We come here if there are any problems */
   973  errout:
   974          dev_err(&pci->dev, "%s failed\n", __func__);
   975          release_everything(dev);
   976  
   977          return err;
   978  }

Do everything error handling is error prone because you're trying to
free some things which haven't been allocated.  It's also more
complicated so it leads to leaks.

The correct way to do error handling is to have a series of labels which
undo one thing at a time.  The labels should be named properly so that
you can tell what the goto does such as "goto err_release_foo;".  That
way you never have to worry about freeing things which haven't been
allocated.  As you read the code, you just have to track the most
recently allocated resource and verify that the goto does what is
expected so you avoid leaks.

In this example:

   857          dev->chip = kzalloc(sizeof(*dev->chip), GFP_KERNEL);
   858          if (!dev->chip) {
   859                  err = -ENOMEM;
   860                  goto errout;
   861          }
   862  
   863          spin_lock_init(&dev->reg_lock);
   864          mutex_init(&dev->dev_mutex);
   865          init_completion(&dev->cmnd_ready);
   866          init_completion(&dev->control_exit);
   867          init_completion(&dev->polling_exit);
   868          init_completion(&dev->notify);
   869          init_completion(&dev->scanning_done);
   870          init_waitqueue_head(&dev->delay_wait);

If the kzalloc() fails, then we call release_everything() with
"dev->chip" NULL.  But we'll actually crash before we hit the NULL
dereference because we didn't do the init_completion(&dev->cmnd_ready);

So this patch doesn't really fix anything because do everything error
handling is a hopeless approach.

regards,
dan carpenter