Re: [PATCH] staging: rts5208: add check on NULL before dereference
On Sat, Jun 09, 2018 at 10:34:43PM +0300, Andy Shevchenko wrote:
> On Sat, Jun 9, 2018 at 7:58 PM, 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(>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(>reg_lock);
864 mutex_init(>dev_mutex);
865 init_completion(>cmnd_ready);
866 init_completion(>control_exit);
867 init_completion(>polling_exit);
868 init_completion(>notify);
869 init_completion(>scanning_done);
870 init_waitqueue_head(>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(>cmnd_ready);
So this patch doesn't really fix anything because do everything error
handling is a hopeless approach.
regards,
dan carpenter
Re: [PATCH] staging: rts5208: add check on NULL before dereference
On Sat, Jun 09, 2018 at 10:34:43PM +0300, Andy Shevchenko wrote:
> On Sat, Jun 9, 2018 at 7:58 PM, 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(>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(>reg_lock);
864 mutex_init(>dev_mutex);
865 init_completion(>cmnd_ready);
866 init_completion(>control_exit);
867 init_completion(>polling_exit);
868 init_completion(>notify);
869 init_completion(>scanning_done);
870 init_waitqueue_head(>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(>cmnd_ready);
So this patch doesn't really fix anything because do everything error
handling is a hopeless approach.
regards,
dan carpenter
Re: [PATCH] staging: rts5208: add check on NULL before dereference
On 2018-06-09 15:34, Andy Shevchenko wrote: On Sat, Jun 9, 2018 at 7:58 PM, 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? Only if dev->chip allocation fails. Code tries to cleanup prior resources by calling clean_everything() function which ends up in rtsx_release_resources()
Re: [PATCH] staging: rts5208: add check on NULL before dereference
On 2018-06-09 15:34, Andy Shevchenko wrote: On Sat, Jun 9, 2018 at 7:58 PM, 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? Only if dev->chip allocation fails. Code tries to cleanup prior resources by calling clean_everything() function which ends up in rtsx_release_resources()
Re: [PATCH] staging: rts5208: add check on NULL before dereference
On Sat, Jun 9, 2018 at 7:58 PM, 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? -- With Best Regards, Andy Shevchenko
Re: [PATCH] staging: rts5208: add check on NULL before dereference
On Sat, Jun 9, 2018 at 7:58 PM, 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? -- With Best Regards, Andy Shevchenko
Re: [PATCH] staging: rts5208: add check on NULL before dereference
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). Signed-off-by: Anton Vasilyev --- drivers/staging/rts5208/rtsx.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c index 70e0b8623110..952dd0d580cf 100644 --- a/drivers/staging/rts5208/rtsx.c +++ b/drivers/staging/rts5208/rtsx.c @@ -623,12 +623,13 @@ static void rtsx_release_resources(struct rtsx_dev *dev) I think you should bail out if dev->chip is null rather than adding conditiinals. if (dev->irq > 0) free_irq(dev->irq, (void *)dev); - if (dev->chip->msi_en) + if (dev->chip && dev->chip->msi_en) pci_disable_msi(dev->pci); if (dev->remap_addr) iounmap(dev->remap_addr); + if (dev->chip) + rtsx_release_chip(dev->chip); - rtsx_release_chip(dev->chip); kfree(dev->chip); }
Re: [PATCH] staging: rts5208: add check on NULL before dereference
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). Signed-off-by: Anton Vasilyev --- drivers/staging/rts5208/rtsx.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c index 70e0b8623110..952dd0d580cf 100644 --- a/drivers/staging/rts5208/rtsx.c +++ b/drivers/staging/rts5208/rtsx.c @@ -623,12 +623,13 @@ static void rtsx_release_resources(struct rtsx_dev *dev) I think you should bail out if dev->chip is null rather than adding conditiinals. if (dev->irq > 0) free_irq(dev->irq, (void *)dev); - if (dev->chip->msi_en) + if (dev->chip && dev->chip->msi_en) pci_disable_msi(dev->pci); if (dev->remap_addr) iounmap(dev->remap_addr); + if (dev->chip) + rtsx_release_chip(dev->chip); - rtsx_release_chip(dev->chip); kfree(dev->chip); }
[PATCH] staging: rts5208: add check on NULL before dereference
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). Signed-off-by: Anton Vasilyev --- drivers/staging/rts5208/rtsx.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c index 70e0b8623110..952dd0d580cf 100644 --- a/drivers/staging/rts5208/rtsx.c +++ b/drivers/staging/rts5208/rtsx.c @@ -623,12 +623,13 @@ static void rtsx_release_resources(struct rtsx_dev *dev) if (dev->irq > 0) free_irq(dev->irq, (void *)dev); - if (dev->chip->msi_en) + if (dev->chip && dev->chip->msi_en) pci_disable_msi(dev->pci); if (dev->remap_addr) iounmap(dev->remap_addr); + if (dev->chip) + rtsx_release_chip(dev->chip); - rtsx_release_chip(dev->chip); kfree(dev->chip); } -- 2.17.1
[PATCH] staging: rts5208: add check on NULL before dereference
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). Signed-off-by: Anton Vasilyev --- drivers/staging/rts5208/rtsx.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c index 70e0b8623110..952dd0d580cf 100644 --- a/drivers/staging/rts5208/rtsx.c +++ b/drivers/staging/rts5208/rtsx.c @@ -623,12 +623,13 @@ static void rtsx_release_resources(struct rtsx_dev *dev) if (dev->irq > 0) free_irq(dev->irq, (void *)dev); - if (dev->chip->msi_en) + if (dev->chip && dev->chip->msi_en) pci_disable_msi(dev->pci); if (dev->remap_addr) iounmap(dev->remap_addr); + if (dev->chip) + rtsx_release_chip(dev->chip); - rtsx_release_chip(dev->chip); kfree(dev->chip); } -- 2.17.1
