linux-kernel - Re: [PATCH v2] block: mtip32xx: Prioritize state cleanup over memory freeing in the mtip_pci

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ab3196a1e0ccb8f94eafb83de589c0ae8f82d598.camel@mailbox.org>
Date: Mon, 25 Aug 2025 13:33:17 +0200
From: Philipp Stanner <phasta@...lbox.org>
To: Zhang Heng <zhangheng@...inos.cn>, axboe@...nel.dk, phasta@...nel.org, 
 andriy.shevchenko@...ux.intel.com, broonie@...nel.org, lizetao1@...wei.com,
  viro@...iv.linux.org.uk, fourier.thomas@...il.com, anuj20.g@...sung.com
Cc: linux-block@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] block: mtip32xx: Prioritize state cleanup over
 memory freeing in the mtip_pci_probe error path.

On Sat, 2025-08-23 at 16:32 +0800, Zhang Heng wrote:
> The original sequence kfree(dd); pci_set_drvdata(pdev, NULL); creates a
> theoretical race condition window. Between these two calls, the pci_dev
> structure retains a dangling pointer to the already-freed device private
> data (dd). Any concurrent access to the drvdata (e.g., from an interrupt
> handler or an unexpected call to remove) would lead to a use-after-free
> kernel oops.

I tend to disagree I think . This is the driver's probe() function. It
is always invoked serially by the driver core.

remove() cannot be called while probe() is still running.

Thus, the only potential explosion that could happen would be if an
interrupt handler were to be set up in this probe() and then accesses
dd.

However, if that were the case, everything would be exploding already
because there is no place in the code that tears down that interrupt
handler or other potential parallel accessors before kfree(dd) is
called.

In this case, the relevant call is pci_enable_msi(). Sooner errors,
like iomap_err: are jumped to before that's the case. Which is the only
sane design for probe() anyways. Otherwise, pci_disable_msi() switches
that off again; IOW: there are no racers.

So I think that the pci_set_drvdata(… NULL) can be removed
alltogether. 

When working on the probe() / remove() paths last and this year, I came
to believe that calls like that were often used because of a
misunderstanding of how the driver core APIs work.

P.

> 
> Changes made:
> 1. `pci_set_drvdata(pdev, NULL);` - First, atomically sever the link
> from the pci_dev.
> 2. `kfree(dd);` - Then, safely free the private memory.
> 
> This ensures the kernel state is always consistent before resources
> are released, adhering to defensive programming principles.
> 
> Signed-off-by: Zhang Heng <zhangheng@...inos.cn>
> ---
>  drivers/block/mtip32xx/mtip32xx.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/block/mtip32xx/mtip32xx.c b/drivers/block/mtip32xx/mtip32xx.c
> index 8fc7761397bd..f228363e6b1c 100644
> --- a/drivers/block/mtip32xx/mtip32xx.c
> +++ b/drivers/block/mtip32xx/mtip32xx.c
> @@ -3839,9 +3839,8 @@ static int mtip_pci_probe(struct pci_dev *pdev,
>  	}
>  
>  iomap_err:
> -	kfree(dd);
>  	pci_set_drvdata(pdev, NULL);
> -	return rv;
> +	kfree(dd);
>  done:
>  	return rv;
>  }