linux-kernel - Re: [PATCH] stmmac: Fix incorrect dereference in stmmac_*

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]

Date: Tue, 6 Feb 2024 15:07:04 +0000
From: Simon Horman <horms@...nel.org>
To: Pavel Sakharov <p.sakharov@...ras.ru>
Cc: Alexandre Torgue <alexandre.torgue@...s.st.com>,
	Jose Abreu <joabreu@...opsys.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Maxime Coquelin <mcoquelin.stm32@...il.com>, netdev@...r.kernel.org,
	linux-stm32@...md-mailman.stormreply.com,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	lvc-project@...uxtesting.org,
	Alexey Khoroshilov <khoroshilov@...ras.ru>
Subject: Re: [PATCH] stmmac: Fix incorrect dereference in stmmac_*_interrupt()

On Sat, Feb 03, 2024 at 06:03:21PM +0300, Pavel Sakharov wrote:
> If 'dev' is NULL, the 'priv' variable has an incorrect address when
> dereferencing calling netdev_err().
> 
> Pass 'dev' instead of 'priv->dev" to the function.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Pavel Sakharov <p.sakharov@...ras.ru>

Thanks Pavel,

I agree with your analysis that this can result in a NULL dereference.
And that your proposed fix is good: netdev_err() can handle a NULL
dev argument.

As this seems to be a fix I suggest it should be for net.
And that it should be based on that tree and designated as such
in the subject:

Subject: [PATCH net] ...

Also if it is a fix, it should have a fixes tag.
Perhaps this one:

Fixes: 8532f613bc78 ("net: stmmac: introduce MSI Interrupt routines for mac, safety, RX & TX")


I don't think there is a need to respin for the above, though please
keep this in mind when posting Networking patches in future.


Looking at the patch above, and stmmac_main.c, it seems that the following
functions also suffer from a similar problem:

static irqreturn_t stmmac_msi_intr_tx(int irq, void *data)
{
	struct stmmac_tx_queue *tx_q = (struct stmmac_tx_queue *)data;
	...
	dma_conf = container_of(tx_q, struct stmmac_dma_conf, tx_queue[chan]);
	priv = container_of(dma_conf, struct stmmac_priv, dma_conf);

	if (unlikely(!data)) {
		netdev_err(priv->dev, "%s: invalid dev pointer\n", __func__);
		...

And stmmac_msi_intr_rx(), which follows a similar pattern
to stmmac_msi_intr_tx().

I also note that in those functions "invalid dev pointer" seems misleading,
perhaps it ought to be "invalid queue" pointer.

As these problems seem to all have been introduced at the same time,
perhaps it is appropriate to fix them all in one patch?

> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 4727f7be4f86..5ab5148013cd 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5848,7 +5848,7 @@ static irqreturn_t stmmac_mac_interrupt(int irq, void *dev_id)
>  	struct stmmac_priv *priv = netdev_priv(dev);
> 
>  	if (unlikely(!dev)) {
> -		netdev_err(priv->dev, "%s: invalid dev pointer\n", __func__);
> +		netdev_err(dev, "%s: invalid dev pointer\n", __func__);
>  		return IRQ_NONE;
>  	}
> 
> @@ -5868,7 +5868,7 @@ static irqreturn_t stmmac_safety_interrupt(int irq, void *dev_id)
>  	struct stmmac_priv *priv = netdev_priv(dev);
> 
>  	if (unlikely(!dev)) {
> -		netdev_err(priv->dev, "%s: invalid dev pointer\n", __func__);
> +		netdev_err(dev, "%s: invalid dev pointer\n", __func__);
>  		return IRQ_NONE;
>  	}
> 
>