| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aNDspDsgj5I5KGLe@stanley.mountain>
Date: Mon, 22 Sep 2025 09:28:52 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Ma Ke <make24@...as.ac.cn>
Cc: dpenkler@...il.com, gregkh@...uxfoundation.org,
matchstick@...erthere.org, dominik.karol.piatkowski@...tonmail.com,
arnd@...db.de, nichen@...as.ac.cn, paul.retourne@...nge.fr,
linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
akpm@...ux-foundation.org, stable@...r.kernel.org
Subject: Re: [PATCH] staging: gpib: Fix device reference leak in fmh_gpib
driver
On Mon, Sep 22, 2025 at 10:38:31AM +0800, Ma Ke wrote:
> The fmh_gpib driver contains a device reference count leak in
> fmh_gpib_attach_impl() where driver_find_device() increases the
> reference count of the device by get_device() when matching but this
> reference is not properly decreased. Add put_device() in
> fmh_gpib_attach_impl() and add put_device() in fmh_gpib_detach(),
> which ensures that the reference count of the device is correctly
> managed.
>
> Found by code review.
>
> Cc: stable@...r.kernel.org
> Fixes: 8e4841a0888c ("staging: gpib: Add Frank Mori Hess FPGA PCI GPIB driver")
> Signed-off-by: Ma Ke <make24@...as.ac.cn>
> ---
> drivers/staging/gpib/fmh_gpib/fmh_gpib.c | 23 +++++++++++++++++++++--
> 1 file changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/gpib/fmh_gpib/fmh_gpib.c b/drivers/staging/gpib/fmh_gpib/fmh_gpib.c
> index 4138f3d2bae7..245c8fe87eaa 100644
> --- a/drivers/staging/gpib/fmh_gpib/fmh_gpib.c
> +++ b/drivers/staging/gpib/fmh_gpib/fmh_gpib.c
> @@ -1395,14 +1395,17 @@ static int fmh_gpib_attach_impl(struct gpib_board *board, const struct gpib_boar
> pdev = to_platform_device(board->dev);
>
> retval = fmh_gpib_generic_attach(board);
> - if (retval)
> + if (retval) {
> + put_device(board->dev);
> return retval;
Do this with an unwind goto.
if (reval)
goto put_dev;
...
retval = fmh_gpib_init(...); /* this bug wasn't fixed btw */
if (retval)
goto put_dev;
return 0;
put_dev:
put_device(board->dev);
return retval;
Actually, this function needs a bunch of other frees as well. See
my blog for more details:
https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/
> + }
>
> e_priv = board->private_data;
> nec_priv = &e_priv->nec7210_priv;
>
> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gpib_control_status");
> if (!res) {
> + put_device(board->dev);
> dev_err(board->dev, "Unable to locate mmio resource\n");
> return -ENODEV;
> }
> @@ -1410,6 +1413,7 @@ static int fmh_gpib_attach_impl(struct gpib_board *board, const struct gpib_boar
> if (request_mem_region(res->start,
> resource_size(res),
> pdev->name) == NULL) {
> + put_device(board->dev);
> dev_err(board->dev, "cannot claim registers\n");
> return -ENXIO;
> }
request_mem_region() needs a release_region().
> @@ -1418,6 +1422,7 @@ static int fmh_gpib_attach_impl(struct gpib_board *board, const struct gpib_boar
> nec_priv->mmiobase = ioremap(e_priv->gpib_iomem_res->start,
> resource_size(e_priv->gpib_iomem_res));
> if (!nec_priv->mmiobase) {
> + put_device(board->dev);
> dev_err(board->dev, "Could not map I/O memory\n");
> return -ENOMEM;
> }
ioremap() needs an iounmap();
> @@ -1426,12 +1431,14 @@ static int fmh_gpib_attach_impl(struct gpib_board *board, const struct gpib_boar
>
> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dma_fifos");
> if (!res) {
> + put_device(board->dev);
> dev_err(board->dev, "Unable to locate mmio resource for gpib dma port\n");
> return -ENODEV;
> }
> if (request_mem_region(res->start,
> resource_size(res),
> pdev->name) == NULL) {
> + put_device(board->dev);
> dev_err(board->dev, "cannot claim registers\n");
> return -ENXIO;
> }
release_region()
> @@ -1439,6 +1446,7 @@ static int fmh_gpib_attach_impl(struct gpib_board *board, const struct gpib_boar
> e_priv->fifo_base = ioremap(e_priv->dma_port_res->start,
> resource_size(e_priv->dma_port_res));
> if (!e_priv->fifo_base) {
> + put_device(board->dev);
> dev_err(board->dev, "Could not map I/O memory for fifos\n");
> return -ENOMEM;
> }
iounmap();
> @@ -1447,10 +1455,14 @@ static int fmh_gpib_attach_impl(struct gpib_board *board, const struct gpib_boar
> (unsigned long)resource_size(e_priv->dma_port_res));
>
> irq = platform_get_irq(pdev, 0);
> - if (irq < 0)
> + if (irq < 0) {
> + put_device(board->dev);
> return -EBUSY;
> + }
> +
> retval = request_irq(irq, fmh_gpib_interrupt, IRQF_SHARED, pdev->name, board);
> if (retval) {
> + put_device(board->dev);
> dev_err(board->dev,
> "cannot register interrupt handler err=%d\n",
> retval);
request_irq() needs a release_irq()
> @@ -1461,6 +1473,7 @@ static int fmh_gpib_attach_impl(struct gpib_board *board, const struct gpib_boar
> if (acquire_dma) {
> e_priv->dma_channel = dma_request_slave_channel(board->dev, "rxtx");
> if (!e_priv->dma_channel) {
> + put_device(board->dev);
> dev_err(board->dev, "failed to acquire dma channel \"rxtx\".\n");
> return -EIO;
> }
This needs a free too. There may be other things outside of what I
can see in this email.
> @@ -1517,6 +1530,12 @@ void fmh_gpib_detach(struct gpib_board *board)
> resource_size(e_priv->gpib_iomem_res));
> }
> fmh_gpib_generic_detach(board);
> +
> + if (board->dev) {
> + dev_set_drvdata(board->dev, NULL);
> + put_device(board->dev);
> + board->dev = NULL;
I explain this in my blog, that the free function should be a cut
and paste of the cleanup. So this stuff isn't done in the cleanup
so one or the other of these is not correct.
The question is should this be done in one patch or several patches?
Adding cleanup to one function is generally considered One Thing
in terms of One Thing Per Path. If we were going to backport bits
of cleanup to different stable kernels then we would want to break
it up into the easiest way for backporting. But realistically we're
not going to do that here because this doesn't affect real life
users generally. It's just from review. It's not a security patch.
And this is staging as well so the standards for backports are not
necessarily as strict. (Staging drivers are often really really bad).
regards,
dan carpenter
Powered by blists - more mailing lists