From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Anton Vasilyev <vasilyev@ispras.ru>
Cc: Dan Carpenter <dan.carpenter@oracle.com>,
Sinan Kaya <okaya@codeaurora.org>,
Johannes Thumshirn <jthumshirn@suse.de>,
Gaurav Pathak <gauravpathak129@gmail.com>,
Hannes Reinecke <hare@suse.de>,
devel@driverdev.osuosl.org,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
ldv-project@linuxtesting.org
Subject: Re: [PATCH v2] staging: rts5208: add check on NULL before dereference
Date: Wed, 13 Jun 2018 20:00:55 +0300 [thread overview]
Message-ID: <CAHp75VetOuS+H4GU9UW-Ekczs22-Ju++jXWdvkMJGktbumDzwQ@mail.gmail.com> (raw)
In-Reply-To: <20180613165501.30669-1-vasilyev@ispras.ru>
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
next prev parent reply other threads:[~2018-06-13 17:01 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-09 16:38 [PATCH] staging: rts5208: add check on NULL before dereference Anton Vasilyev
2018-06-09 16:58 ` okaya
2018-06-09 19:34 ` Andy Shevchenko
2018-06-09 22:22 ` okaya
2018-06-12 13:06 ` Dan Carpenter
2018-06-13 16:55 ` [PATCH v2] " Anton Vasilyev
2018-06-13 17:00 ` Andy Shevchenko [this message]
2018-06-13 17:34 ` [PATCH v3] staging: rts5208: add error handling into rtsx_probe Anton Vasilyev
[not found] ` <20180613173128.32384-1-vasilyev@ispras.ru>
2018-06-19 7:42 ` your mail Dan Carpenter
2018-06-19 15:25 ` [PATCH v4] staging: rts5208: add error handling into rtsx_probe Anton Vasilyev
2018-06-19 17:13 ` Andy Shevchenko
2018-08-01 11:55 ` [PATCH v5] " Anton Vasilyev
2018-08-01 12:18 ` Andy Shevchenko
2018-08-01 14:08 ` Anton Vasilyev
2018-08-01 14:52 ` Dan Carpenter
2018-08-01 15:37 ` Andy Shevchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAHp75VetOuS+H4GU9UW-Ekczs22-Ju++jXWdvkMJGktbumDzwQ@mail.gmail.com \
--to=andy.shevchenko@gmail.com \
--cc=dan.carpenter@oracle.com \
--cc=devel@driverdev.osuosl.org \
--cc=gauravpathak129@gmail.com \
--cc=hare@suse.de \
--cc=jthumshirn@suse.de \
--cc=ldv-project@linuxtesting.org \
--cc=linux-kernel@vger.kernel.org \
--cc=okaya@codeaurora.org \
--cc=vasilyev@ispras.ru \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).