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

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

[Anton Vasilyev]

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 <vasilyev@ispras.ru>
---
 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

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

[okaya]

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 <vasilyev@ispras.ru>
> ---
>  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

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

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

[okaya]

On 2018-06-09 15:34, 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?

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

[Dan Carpenter]

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

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

[Anton Vasilyev]

If rtsx_probe() fails to allocate dev->chip, then NULL pointer
dereference occurs at release_everything()->rtsx_release_resources().

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Anton Vasilyev <vasilyev@ispras.ru>
---
v2: Add error handling into rtsx_probe based on Dan Carpenter's comment.
I do not have corresponding hardware, so patch was tested by compilation only.

I faced with inaccuracy at rtsx_remove() and original rtsx_probe():
there is quiesce_and_remove_host() call with scsi_remove_host() inside,
whereas release_everything() calls scsi_host_put() after this
scsi_remove_host() call. This is strange for me.

Also I do not know is it require to check result value of
rtsx_init_chip() call on rtsx_probe().
---
 drivers/staging/rts5208/rtsx.c | 38 +++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
index 70e0b8623110..69e6abe14abf 100644
--- a/drivers/staging/rts5208/rtsx.c
+++ b/drivers/staging/rts5208/rtsx.c
@@ -857,7 +857,7 @@ static int rtsx_probe(struct pci_dev *pci,
 	dev->chip = kzalloc(sizeof(*dev->chip), GFP_KERNEL);
 	if (!dev->chip) {
 		err = -ENOMEM;
-		goto errout;
+		goto chip_alloc_fail;
 	}
 
 	spin_lock_init(&dev->reg_lock);
@@ -879,7 +879,7 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (!dev->remap_addr) {
 		dev_err(&pci->dev, "ioremap error\n");
 		err = -ENXIO;
-		goto errout;
+		goto ioremap_fail;
 	}
 
 	/*
@@ -894,7 +894,7 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (!dev->rtsx_resv_buf) {
 		dev_err(&pci->dev, "alloc dma buffer fail\n");
 		err = -ENXIO;
-		goto errout;
+		goto dma_alloc_fail;
 	}
 	dev->chip->host_cmds_ptr = dev->rtsx_resv_buf;
 	dev->chip->host_cmds_addr = dev->rtsx_resv_buf_addr;
@@ -915,7 +915,7 @@ static int rtsx_probe(struct pci_dev *pci,
 
 	if (rtsx_acquire_irq(dev) < 0) {
 		err = -EBUSY;
-		goto errout;
+		goto irq_acquire_fail;
 	}
 
 	pci_set_master(pci);
@@ -935,14 +935,14 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (IS_ERR(th)) {
 		dev_err(&pci->dev, "Unable to start control thread\n");
 		err = PTR_ERR(th);
-		goto errout;
+		goto control_thread_fail;
 	}
 	dev->ctl_thread = th;
 
 	err = scsi_add_host(host, &pci->dev);
 	if (err) {
 		dev_err(&pci->dev, "Unable to add the scsi host\n");
-		goto errout;
+		goto scsi_add_host_fail;
 	}
 
 	/* Start up the thread for delayed SCSI-device scanning */
@@ -950,18 +950,16 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (IS_ERR(th)) {
 		dev_err(&pci->dev, "Unable to start the device-scanning thread\n");
 		complete(&dev->scanning_done);
-		quiesce_and_remove_host(dev);
 		err = PTR_ERR(th);
-		goto errout;
+		goto scan_thread_fail;
 	}
 
 	/* Start up the thread for polling thread */
 	th = kthread_run(rtsx_polling_thread, dev, "rtsx-polling");
 	if (IS_ERR(th)) {
 		dev_err(&pci->dev, "Unable to start the device-polling thread\n");
-		quiesce_and_remove_host(dev);
 		err = PTR_ERR(th);
-		goto errout;
+		goto scan_thread_fail;
 	}
 	dev->polling_thread = th;
 
@@ -970,9 +968,25 @@ static int rtsx_probe(struct pci_dev *pci,
 	return 0;
 
 	/* We come here if there are any problems */
-errout:
+scan_thread_fail:
+	quiesce_and_remove_host(dev);
+scsi_add_host_fail:
+	complete(&dev->cmnd_ready);
+	wait_for_completion(&dev->control_exit);
+control_thread_fail:
+	free_irq(dev->irq, (void *)dev);
+	rtsx_release_chip(dev->chip);
+irq_acquire_fail:
+	dev->chip->host_cmds_ptr = NULL;
+	dev->chip->host_sg_tbl_ptr = NULL;
+	if (dev->chip->msi_en)
+		pci_disable_msi(dev->pci);
+dma_alloc_fail:
+	iounmap(dev->remap_addr);
+ioremap_fail:
+	kfree(dev->chip);
+chip_alloc_fail:
 	dev_err(&pci->dev, "%s failed\n", __func__);
-	release_everything(dev);
 
 	return err;
 }
-- 
2.17.1

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

[Andy Shevchenko]

On Wed, Jun 13, 2018 at 7:55 PM, Anton Vasilyev <vasilyev@ispras.ru> wrote:
> If rtsx_probe() fails to allocate dev->chip, then NULL pointer
> dereference occurs at release_everything()->rtsx_release_resources().
>
> Found by Linux Driver Verification project (linuxtesting.org).
>

You forgot to adjust subject and commit message to the new reality
which is brought by this change.

> Signed-off-by: Anton Vasilyev <vasilyev@ispras.ru>
> ---
> v2: Add error handling into rtsx_probe based on Dan Carpenter's comment.
> I do not have corresponding hardware, so patch was tested by compilation only.
>
> I faced with inaccuracy at rtsx_remove() and original rtsx_probe():
> there is quiesce_and_remove_host() call with scsi_remove_host() inside,
> whereas release_everything() calls scsi_host_put() after this
> scsi_remove_host() call. This is strange for me.
>
> Also I do not know is it require to check result value of
> rtsx_init_chip() call on rtsx_probe().
> ---
>  drivers/staging/rts5208/rtsx.c | 38 +++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
> index 70e0b8623110..69e6abe14abf 100644
> --- a/drivers/staging/rts5208/rtsx.c
> +++ b/drivers/staging/rts5208/rtsx.c
> @@ -857,7 +857,7 @@ static int rtsx_probe(struct pci_dev *pci,
>         dev->chip = kzalloc(sizeof(*dev->chip), GFP_KERNEL);
>         if (!dev->chip) {
>                 err = -ENOMEM;
> -               goto errout;
> +               goto chip_alloc_fail;
>         }
>
>         spin_lock_init(&dev->reg_lock);
> @@ -879,7 +879,7 @@ static int rtsx_probe(struct pci_dev *pci,
>         if (!dev->remap_addr) {
>                 dev_err(&pci->dev, "ioremap error\n");
>                 err = -ENXIO;
> -               goto errout;
> +               goto ioremap_fail;
>         }
>
>         /*
> @@ -894,7 +894,7 @@ static int rtsx_probe(struct pci_dev *pci,
>         if (!dev->rtsx_resv_buf) {
>                 dev_err(&pci->dev, "alloc dma buffer fail\n");
>                 err = -ENXIO;
> -               goto errout;
> +               goto dma_alloc_fail;
>         }
>         dev->chip->host_cmds_ptr = dev->rtsx_resv_buf;
>         dev->chip->host_cmds_addr = dev->rtsx_resv_buf_addr;
> @@ -915,7 +915,7 @@ static int rtsx_probe(struct pci_dev *pci,
>
>         if (rtsx_acquire_irq(dev) < 0) {
>                 err = -EBUSY;
> -               goto errout;
> +               goto irq_acquire_fail;
>         }
>
>         pci_set_master(pci);
> @@ -935,14 +935,14 @@ static int rtsx_probe(struct pci_dev *pci,
>         if (IS_ERR(th)) {
>                 dev_err(&pci->dev, "Unable to start control thread\n");
>                 err = PTR_ERR(th);
> -               goto errout;
> +               goto control_thread_fail;
>         }
>         dev->ctl_thread = th;
>
>         err = scsi_add_host(host, &pci->dev);
>         if (err) {
>                 dev_err(&pci->dev, "Unable to add the scsi host\n");
> -               goto errout;
> +               goto scsi_add_host_fail;
>         }
>
>         /* Start up the thread for delayed SCSI-device scanning */
> @@ -950,18 +950,16 @@ static int rtsx_probe(struct pci_dev *pci,
>         if (IS_ERR(th)) {
>                 dev_err(&pci->dev, "Unable to start the device-scanning thread\n");
>                 complete(&dev->scanning_done);
> -               quiesce_and_remove_host(dev);
>                 err = PTR_ERR(th);
> -               goto errout;
> +               goto scan_thread_fail;
>         }
>
>         /* Start up the thread for polling thread */
>         th = kthread_run(rtsx_polling_thread, dev, "rtsx-polling");
>         if (IS_ERR(th)) {
>                 dev_err(&pci->dev, "Unable to start the device-polling thread\n");
> -               quiesce_and_remove_host(dev);
>                 err = PTR_ERR(th);
> -               goto errout;
> +               goto scan_thread_fail;
>         }
>         dev->polling_thread = th;
>
> @@ -970,9 +968,25 @@ static int rtsx_probe(struct pci_dev *pci,
>         return 0;
>
>         /* We come here if there are any problems */
> -errout:
> +scan_thread_fail:
> +       quiesce_and_remove_host(dev);
> +scsi_add_host_fail:
> +       complete(&dev->cmnd_ready);
> +       wait_for_completion(&dev->control_exit);
> +control_thread_fail:
> +       free_irq(dev->irq, (void *)dev);
> +       rtsx_release_chip(dev->chip);
> +irq_acquire_fail:
> +       dev->chip->host_cmds_ptr = NULL;
> +       dev->chip->host_sg_tbl_ptr = NULL;
> +       if (dev->chip->msi_en)
> +               pci_disable_msi(dev->pci);
> +dma_alloc_fail:
> +       iounmap(dev->remap_addr);
> +ioremap_fail:
> +       kfree(dev->chip);
> +chip_alloc_fail:
>         dev_err(&pci->dev, "%s failed\n", __func__);
> -       release_everything(dev);
>
>         return err;
>  }
> --
> 2.17.1
>



-- 
With Best Regards,
Andy Shevchenko

[PATCH v3] staging: rts5208: add error handling into rtsx_probe

[Anton Vasilyev]

If rtsx_probe() fails to allocate dev->chip, then release_everything()
will crash on uninitialized dev->cmnd_ready complete.

Patch adds error handling into rtsx_probe.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Anton Vasilyev <vasilyev@ispras.ru>
---
v3: fix subject and commit message
v2: Add error handling into rtsx_probe based on Dan Carpenter's comment.
I do not have corresponding hardware, so patch was tested by compilation only.

I faced with inaccuracy at rtsx_remove() and original rtsx_probe():
there is quiesce_and_remove_host() call with scsi_remove_host() inside,
whereas release_everything() calls scsi_host_put() after this
scsi_remove_host() call. This is strange for me.
Also I do not know is it require to check result value of
rtsx_init_chip() call on rtsx_probe().
---
 drivers/staging/rts5208/rtsx.c | 38 +++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
index 70e0b8623110..69e6abe14abf 100644
--- a/drivers/staging/rts5208/rtsx.c
+++ b/drivers/staging/rts5208/rtsx.c
@@ -857,7 +857,7 @@ static int rtsx_probe(struct pci_dev *pci,
 	dev->chip = kzalloc(sizeof(*dev->chip), GFP_KERNEL);
 	if (!dev->chip) {
 		err = -ENOMEM;
-		goto errout;
+		goto chip_alloc_fail;
 	}
 
 	spin_lock_init(&dev->reg_lock);
@@ -879,7 +879,7 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (!dev->remap_addr) {
 		dev_err(&pci->dev, "ioremap error\n");
 		err = -ENXIO;
-		goto errout;
+		goto ioremap_fail;
 	}
 
 	/*
@@ -894,7 +894,7 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (!dev->rtsx_resv_buf) {
 		dev_err(&pci->dev, "alloc dma buffer fail\n");
 		err = -ENXIO;
-		goto errout;
+		goto dma_alloc_fail;
 	}
 	dev->chip->host_cmds_ptr = dev->rtsx_resv_buf;
 	dev->chip->host_cmds_addr = dev->rtsx_resv_buf_addr;
@@ -915,7 +915,7 @@ static int rtsx_probe(struct pci_dev *pci,
 
 	if (rtsx_acquire_irq(dev) < 0) {
 		err = -EBUSY;
-		goto errout;
+		goto irq_acquire_fail;
 	}
 
 	pci_set_master(pci);
@@ -935,14 +935,14 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (IS_ERR(th)) {
 		dev_err(&pci->dev, "Unable to start control thread\n");
 		err = PTR_ERR(th);
-		goto errout;
+		goto control_thread_fail;
 	}
 	dev->ctl_thread = th;
 
 	err = scsi_add_host(host, &pci->dev);
 	if (err) {
 		dev_err(&pci->dev, "Unable to add the scsi host\n");
-		goto errout;
+		goto scsi_add_host_fail;
 	}
 
 	/* Start up the thread for delayed SCSI-device scanning */
@@ -950,18 +950,16 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (IS_ERR(th)) {
 		dev_err(&pci->dev, "Unable to start the device-scanning thread\n");
 		complete(&dev->scanning_done);
-		quiesce_and_remove_host(dev);
 		err = PTR_ERR(th);
-		goto errout;
+		goto scan_thread_fail;
 	}
 
 	/* Start up the thread for polling thread */
 	th = kthread_run(rtsx_polling_thread, dev, "rtsx-polling");
 	if (IS_ERR(th)) {
 		dev_err(&pci->dev, "Unable to start the device-polling thread\n");
-		quiesce_and_remove_host(dev);
 		err = PTR_ERR(th);
-		goto errout;
+		goto scan_thread_fail;
 	}
 	dev->polling_thread = th;
 
@@ -970,9 +968,25 @@ static int rtsx_probe(struct pci_dev *pci,
 	return 0;
 
 	/* We come here if there are any problems */
-errout:
+scan_thread_fail:
+	quiesce_and_remove_host(dev);
+scsi_add_host_fail:
+	complete(&dev->cmnd_ready);
+	wait_for_completion(&dev->control_exit);
+control_thread_fail:
+	free_irq(dev->irq, (void *)dev);
+	rtsx_release_chip(dev->chip);
+irq_acquire_fail:
+	dev->chip->host_cmds_ptr = NULL;
+	dev->chip->host_sg_tbl_ptr = NULL;
+	if (dev->chip->msi_en)
+		pci_disable_msi(dev->pci);
+dma_alloc_fail:
+	iounmap(dev->remap_addr);
+ioremap_fail:
+	kfree(dev->chip);
+chip_alloc_fail:
 	dev_err(&pci->dev, "%s failed\n", __func__);
-	release_everything(dev);
 
 	return err;
 }
-- 
2.17.1

Re: your mail

[Dan Carpenter]

Thanks for this.  This is a lot of work.

On Wed, Jun 13, 2018 at 08:31:28PM +0300, Anton Vasilyev wrote:
> diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
> index 70e0b8623110..69e6abe14abf 100644
> --- a/drivers/staging/rts5208/rtsx.c
> +++ b/drivers/staging/rts5208/rtsx.c
> @@ -857,7 +857,7 @@ static int rtsx_probe(struct pci_dev *pci,
>  	dev->chip = kzalloc(sizeof(*dev->chip), GFP_KERNEL);
>  	if (!dev->chip) {
>  		err = -ENOMEM;
> -		goto errout;
> +		goto chip_alloc_fail;

The most recent successful allocation is scsi_host_alloc().  I was
really hoping this would say something like "goto err_free_host;" or
something.  The naming style here is a "come from" label which doesn't
say if it's going to free the scsi host or not...  It turns out we don't
free the the host, but we should:

err_put_host:
	scsi_host_put(host);

The kzalloc() has it's own error message built in, and all the other
error paths as well so the dev_err() is not super important to me...

Killing the threads seems actually really complicated so maybe we should
just have a separate error paths for that.  I'm not sure...

regards,
dan carpenter

[PATCH v4] staging: rts5208: add error handling into rtsx_probe

[Anton Vasilyev]

If rtsx_probe() fails to allocate dev->chip, then release_everything()
will crash on uninitialized dev->cmnd_ready complete

Patch adds error handling into rtsx_probe.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Anton Vasilyev <vasilyev@ispras.ru>
---
v4: rename labels baced on Dan Carpenter's recommendation
v3: fix subject and commit message
v2: Add error handling into rtsx_probe based on Dan Carpenter's comment.
I do not have corresponding hardware, so patch was tested by compilation only.

I faced with inaccuracy at rtsx_remove() and original rtsx_probe():
there is quiesce_and_remove_host() call with scsi_remove_host() inside,
whereas release_everything() calls scsi_host_put() after this
scsi_remove_host() call. This is strange for me.
Also I do not know is it require to check result value of
rtsx_init_chip() call on rtsx_probe().
---
 drivers/staging/rts5208/rtsx.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
index 70e0b8623110..233026ee5dd4 100644
--- a/drivers/staging/rts5208/rtsx.c
+++ b/drivers/staging/rts5208/rtsx.c
@@ -879,7 +879,7 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (!dev->remap_addr) {
 		dev_err(&pci->dev, "ioremap error\n");
 		err = -ENXIO;
-		goto errout;
+		goto err_chip_free;
 	}
 
 	/*
@@ -894,7 +894,7 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (!dev->rtsx_resv_buf) {
 		dev_err(&pci->dev, "alloc dma buffer fail\n");
 		err = -ENXIO;
-		goto errout;
+		goto err_addr_unmap;
 	}
 	dev->chip->host_cmds_ptr = dev->rtsx_resv_buf;
 	dev->chip->host_cmds_addr = dev->rtsx_resv_buf_addr;
@@ -915,7 +915,7 @@ static int rtsx_probe(struct pci_dev *pci,
 
 	if (rtsx_acquire_irq(dev) < 0) {
 		err = -EBUSY;
-		goto errout;
+		goto err_disable_msi;
 	}
 
 	pci_set_master(pci);
@@ -935,14 +935,14 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (IS_ERR(th)) {
 		dev_err(&pci->dev, "Unable to start control thread\n");
 		err = PTR_ERR(th);
-		goto errout;
+		goto err_rtsx_release;
 	}
 	dev->ctl_thread = th;
 
 	err = scsi_add_host(host, &pci->dev);
 	if (err) {
 		dev_err(&pci->dev, "Unable to add the scsi host\n");
-		goto errout;
+		goto err_complete_control_thread;
 	}
 
 	/* Start up the thread for delayed SCSI-device scanning */
@@ -950,18 +950,16 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (IS_ERR(th)) {
 		dev_err(&pci->dev, "Unable to start the device-scanning thread\n");
 		complete(&dev->scanning_done);
-		quiesce_and_remove_host(dev);
 		err = PTR_ERR(th);
-		goto errout;
+		goto err_stop_host;
 	}
 
 	/* Start up the thread for polling thread */
 	th = kthread_run(rtsx_polling_thread, dev, "rtsx-polling");
 	if (IS_ERR(th)) {
 		dev_err(&pci->dev, "Unable to start the device-polling thread\n");
-		quiesce_and_remove_host(dev);
 		err = PTR_ERR(th);
-		goto errout;
+		goto err_stop_host;
 	}
 	dev->polling_thread = th;
 
@@ -970,9 +968,25 @@ static int rtsx_probe(struct pci_dev *pci,
 	return 0;
 
 	/* We come here if there are any problems */
+err_stop_host:
+	quiesce_and_remove_host(dev);
+err_complete_control_thread:
+	complete(&dev->cmnd_ready);
+	wait_for_completion(&dev->control_exit);
+err_rtsx_release:
+	free_irq(dev->irq, (void *)dev);
+	rtsx_release_chip(dev->chip);
+err_disable_msi:
+	dev->chip->host_cmds_ptr = NULL;
+	dev->chip->host_sg_tbl_ptr = NULL;
+	if (dev->chip->msi_en)
+		pci_disable_msi(dev->pci);
+err_addr_unmap:
+	iounmap(dev->remap_addr);
+err_chip_free:
+	kfree(dev->chip);
 errout:
 	dev_err(&pci->dev, "%s failed\n", __func__);
-	release_everything(dev);
 
 	return err;
 }
-- 
2.17.1

Re: [PATCH v4] staging: rts5208: add error handling into rtsx_probe

[Andy Shevchenko]

On Tue, Jun 19, 2018 at 6:25 PM, Anton Vasilyev <vasilyev@ispras.ru> wrote:
> If rtsx_probe() fails to allocate dev->chip, then release_everything()
> will crash on uninitialized dev->cmnd_ready complete

Period is missed at the end.

>
> Patch adds error handling into rtsx_probe.

an error

>
> Found by Linux Driver Verification project (linuxtesting.org).
>

Reviewed-by: Andy Shevchenko <andy.shevhchenko@gmail.com>

Couple of comments below though.

> Signed-off-by: Anton Vasilyev <vasilyev@ispras.ru>
> ---
> v4: rename labels baced on Dan Carpenter's recommendation
> v3: fix subject and commit message
> v2: Add error handling into rtsx_probe based on Dan Carpenter's comment.
> I do not have corresponding hardware, so patch was tested by compilation only.
>
> I faced with inaccuracy at rtsx_remove() and original rtsx_probe():
> there is quiesce_and_remove_host() call with scsi_remove_host() inside,
> whereas release_everything() calls scsi_host_put() after this
> scsi_remove_host() call. This is strange for me.
> Also I do not know is it require to check result value of
> rtsx_init_chip() call on rtsx_probe().
> ---
>  drivers/staging/rts5208/rtsx.c | 34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
> index 70e0b8623110..233026ee5dd4 100644
> --- a/drivers/staging/rts5208/rtsx.c
> +++ b/drivers/staging/rts5208/rtsx.c
> @@ -879,7 +879,7 @@ static int rtsx_probe(struct pci_dev *pci,
>         if (!dev->remap_addr) {
>                 dev_err(&pci->dev, "ioremap error\n");
>                 err = -ENXIO;
> -               goto errout;
> +               goto err_chip_free;
>         }
>
>         /*
> @@ -894,7 +894,7 @@ static int rtsx_probe(struct pci_dev *pci,
>         if (!dev->rtsx_resv_buf) {
>                 dev_err(&pci->dev, "alloc dma buffer fail\n");
>                 err = -ENXIO;
> -               goto errout;
> +               goto err_addr_unmap;
>         }
>         dev->chip->host_cmds_ptr = dev->rtsx_resv_buf;
>         dev->chip->host_cmds_addr = dev->rtsx_resv_buf_addr;
> @@ -915,7 +915,7 @@ static int rtsx_probe(struct pci_dev *pci,
>
>         if (rtsx_acquire_irq(dev) < 0) {
>                 err = -EBUSY;
> -               goto errout;
> +               goto err_disable_msi;
>         }
>
>         pci_set_master(pci);
> @@ -935,14 +935,14 @@ static int rtsx_probe(struct pci_dev *pci,
>         if (IS_ERR(th)) {
>                 dev_err(&pci->dev, "Unable to start control thread\n");
>                 err = PTR_ERR(th);
> -               goto errout;
> +               goto err_rtsx_release;
>         }
>         dev->ctl_thread = th;
>
>         err = scsi_add_host(host, &pci->dev);
>         if (err) {
>                 dev_err(&pci->dev, "Unable to add the scsi host\n");
> -               goto errout;
> +               goto err_complete_control_thread;
>         }
>
>         /* Start up the thread for delayed SCSI-device scanning */
> @@ -950,18 +950,16 @@ static int rtsx_probe(struct pci_dev *pci,
>         if (IS_ERR(th)) {
>                 dev_err(&pci->dev, "Unable to start the device-scanning thread\n");
>                 complete(&dev->scanning_done);
> -               quiesce_and_remove_host(dev);
>                 err = PTR_ERR(th);
> -               goto errout;
> +               goto err_stop_host;
>         }
>
>         /* Start up the thread for polling thread */
>         th = kthread_run(rtsx_polling_thread, dev, "rtsx-polling");
>         if (IS_ERR(th)) {
>                 dev_err(&pci->dev, "Unable to start the device-polling thread\n");
> -               quiesce_and_remove_host(dev);
>                 err = PTR_ERR(th);
> -               goto errout;
> +               goto err_stop_host;
>         }
>         dev->polling_thread = th;
>
> @@ -970,9 +968,25 @@ static int rtsx_probe(struct pci_dev *pci,
>         return 0;
>
>         /* We come here if there are any problems */
> +err_stop_host:
> +       quiesce_and_remove_host(dev);
> +err_complete_control_thread:
> +       complete(&dev->cmnd_ready);
> +       wait_for_completion(&dev->control_exit);
> +err_rtsx_release:
> +       free_irq(dev->irq, (void *)dev);
> +       rtsx_release_chip(dev->chip);
> +err_disable_msi:

> +       dev->chip->host_cmds_ptr = NULL;
> +       dev->chip->host_sg_tbl_ptr = NULL;

This seems superfluous, you are freeing entire struct few calls after.

> +       if (dev->chip->msi_en)
> +               pci_disable_msi(dev->pci);

This might be material for another improvement, PCI core tracks
MSI/MSI-X enable state.
So, the conditional with the local variable looks superfluous as well
(in some, rare, cases we indeed need something like this to basically
distinguish MSI from MSI-X, though I don't think it's a case here)

> +err_addr_unmap:
> +       iounmap(dev->remap_addr);
> +err_chip_free:
> +       kfree(dev->chip);
>  errout:
>         dev_err(&pci->dev, "%s failed\n", __func__);
> -       release_everything(dev);
>
>         return err;
>  }
> --
> 2.17.1
>



-- 
With Best Regards,
Andy Shevchenko

[PATCH v5] staging: rts5208: add error handling into rtsx_probe

[Anton Vasilyev]

If rtsx_probe() fails to allocate dev->chip, then release_everything()
will crash on uninitialized dev->cmnd_ready complete.

Patch adds an error handling into rtsx_probe.
Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Anton Vasilyev <vasilyev@ispras.ru>
---
v5: fix mistype and remove superfluous pointers zeroing
v4: rename labels baced on Dan Carpenter's recommendation
v3: fix subject and commit message
v2: Add error handling into rtsx_probe based on Dan Carpenter's comment.
I do not have corresponding hardware, so patch was tested by compilation only.

I faced with inaccuracy at rtsx_remove() and original rtsx_probe():
there is quiesce_and_remove_host() call with scsi_remove_host() inside,
whereas release_everything() calls scsi_host_put() after this
scsi_remove_host() call. This is strange for me.
Also I do not know is it require to check result value of
rtsx_init_chip() call on rtsx_probe().
---
 drivers/staging/rts5208/rtsx.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
index 70e0b8623110..6a5c670634b1 100644
--- a/drivers/staging/rts5208/rtsx.c
+++ b/drivers/staging/rts5208/rtsx.c
@@ -879,7 +879,7 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (!dev->remap_addr) {
 		dev_err(&pci->dev, "ioremap error\n");
 		err = -ENXIO;
-		goto errout;
+		goto err_chip_free;
 	}
 
 	/*
@@ -894,7 +894,7 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (!dev->rtsx_resv_buf) {
 		dev_err(&pci->dev, "alloc dma buffer fail\n");
 		err = -ENXIO;
-		goto errout;
+		goto err_addr_unmap;
 	}
 	dev->chip->host_cmds_ptr = dev->rtsx_resv_buf;
 	dev->chip->host_cmds_addr = dev->rtsx_resv_buf_addr;
@@ -915,7 +915,7 @@ static int rtsx_probe(struct pci_dev *pci,
 
 	if (rtsx_acquire_irq(dev) < 0) {
 		err = -EBUSY;
-		goto errout;
+		goto err_disable_msi;
 	}
 
 	pci_set_master(pci);
@@ -935,14 +935,14 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (IS_ERR(th)) {
 		dev_err(&pci->dev, "Unable to start control thread\n");
 		err = PTR_ERR(th);
-		goto errout;
+		goto err_rtsx_release;
 	}
 	dev->ctl_thread = th;
 
 	err = scsi_add_host(host, &pci->dev);
 	if (err) {
 		dev_err(&pci->dev, "Unable to add the scsi host\n");
-		goto errout;
+		goto err_complete_control_thread;
 	}
 
 	/* Start up the thread for delayed SCSI-device scanning */
@@ -950,18 +950,16 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (IS_ERR(th)) {
 		dev_err(&pci->dev, "Unable to start the device-scanning thread\n");
 		complete(&dev->scanning_done);
-		quiesce_and_remove_host(dev);
 		err = PTR_ERR(th);
-		goto errout;
+		goto err_stop_host;
 	}
 
 	/* Start up the thread for polling thread */
 	th = kthread_run(rtsx_polling_thread, dev, "rtsx-polling");
 	if (IS_ERR(th)) {
 		dev_err(&pci->dev, "Unable to start the device-polling thread\n");
-		quiesce_and_remove_host(dev);
 		err = PTR_ERR(th);
-		goto errout;
+		goto err_stop_host;
 	}
 	dev->polling_thread = th;
 
@@ -970,9 +968,23 @@ static int rtsx_probe(struct pci_dev *pci,
 	return 0;
 
 	/* We come here if there are any problems */
+err_stop_host:
+	quiesce_and_remove_host(dev);
+err_complete_control_thread:
+	complete(&dev->cmnd_ready);
+	wait_for_completion(&dev->control_exit);
+err_rtsx_release:
+	free_irq(dev->irq, (void *)dev);
+	rtsx_release_chip(dev->chip);
+err_disable_msi:
+	if (dev->chip->msi_en)
+		pci_disable_msi(dev->pci);
+err_addr_unmap:
+	iounmap(dev->remap_addr);
+err_chip_free:
+	kfree(dev->chip);
 errout:
 	dev_err(&pci->dev, "%s failed\n", __func__);
-	release_everything(dev);
 
 	return err;
 }
-- 
2.18.0

Re: [PATCH v5] staging: rts5208: add error handling into rtsx_probe

[Andy Shevchenko]

On Wed, Aug 1, 2018 at 2:55 PM, Anton Vasilyev <vasilyev@ispras.ru> wrote:
> If rtsx_probe() fails to allocate dev->chip, then release_everything()
> will crash on uninitialized dev->cmnd_ready complete.
>
> Patch adds an error handling into rtsx_probe.
> Found by Linux Driver Verification project (linuxtesting.org).

Have you based your change on staging-next?

Seems not. You need to rebase and resend.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5] staging: rts5208: add error handling into rtsx_probe

[Anton Vasilyev]

I found that staging-next already contains my patch v3, committed by 
Greg Kroah-Hartman.

Do I need to send a new patch with a label renaming based on Dan 
Carpenter comments?


-- 
Anton Vasilyev
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: vasilyev@ispras.ru

On 01.08.2018 15:18, Andy Shevchenko wrote:
> On Wed, Aug 1, 2018 at 2:55 PM, Anton Vasilyev <vasilyev@ispras.ru> wrote:
>> If rtsx_probe() fails to allocate dev->chip, then release_everything()
>> will crash on uninitialized dev->cmnd_ready complete.
>>
>> Patch adds an error handling into rtsx_probe.
>> Found by Linux Driver Verification project (linuxtesting.org).
> Have you based your change on staging-next?
>
> Seems not. You need to rebase and resend.
>

Re: [PATCH v5] staging: rts5208: add error handling into rtsx_probe

[Dan Carpenter]

On Wed, Aug 01, 2018 at 05:08:48PM +0300, Anton Vasilyev wrote:
> I found that staging-next already contains my patch v3, committed by Greg
> Kroah-Hartman.
> 
> Do I need to send a new patch with a label renaming based on Dan Carpenter
> comments?

I had to look to see what I had said earlier...  The naming isn't really
a problem but we should call scsi_host_put(host); if the
"dev->chip = kzalloc(sizeof(*dev->chip), GFP_KERNEL);" allocation fails.

regards,
dan capenter

Re: [PATCH v5] staging: rts5208: add error handling into rtsx_probe

[Andy Shevchenko]

On Wed, Aug 1, 2018 at 5:08 PM, Anton Vasilyev <vasilyev@ispras.ru> wrote:
> I found that staging-next already contains my patch v3, committed by Greg
> Kroah-Hartman.
>
> Do I need to send a new patch

Yes. Based on staging-next.

> with a label renaming based on Dan Carpenter
> comments?

Dan is talking for himself :-)

-- 
With Best Regards,
Andy Shevchenko