netdev - Re: [PATCH net 1/5] net/mlx5e: Fix netif state handling

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: <20240510153133.GC2347895@kernel.org>
Date: Fri, 10 May 2024 16:31:33 +0100
From: Simon Horman <horms@...nel.org>
To: Tariq Toukan <tariqt@...dia.com>
Cc: "David S. Miller" <davem@...emloft.net>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Eric Dumazet <edumazet@...gle.com>, netdev@...r.kernel.org,
	Saeed Mahameed <saeedm@...dia.com>, Gal Pressman <gal@...dia.com>,
	Leon Romanovsky <leonro@...dia.com>, Shay Drory <shayd@...dia.com>
Subject: Re: [PATCH net 1/5] net/mlx5e: Fix netif state handling

On Thu, May 09, 2024 at 02:29:47PM +0300, Tariq Toukan wrote:
> From: Shay Drory <shayd@...dia.com>
> 
> mlx5e_suspend cleans resources only if netif_device_present() returns
> true. However, mlx5e_resume changes the state of netif, via
> mlx5e_nic_enable, only if reg_state == NETREG_REGISTERED.
> In the below case, the above leads to NULL-ptr Oops[1] and memory
> leaks:
> 
> mlx5e_probe
>  _mlx5e_resume
>   mlx5e_attach_netdev
>    mlx5e_nic_enable  <-- netdev not reg, not calling netif_device_attach()
>   register_netdev <-- failed for some reason.
> ERROR_FLOW:
>  _mlx5e_suspend <-- netif_device_present return false, resources aren't freed :(
> 
> Hence, clean resources in this case as well.
> 
> [1]
> BUG: kernel NULL pointer dereference, address: 0000000000000000

...

> Fixes: 2c3b5beec46a ("net/mlx5e: More generic netdev management API")
> Signed-off-by: Shay Drory <shayd@...dia.com>
> Signed-off-by: Tariq Toukan <tariqt@...dia.com>

Hi,

I think that this bug is caused by asymmetry in resource allocation/freeing
such that there are cases where _mlx5e_suspend() doesn't unwind
_mlx5e_resume().

It seems to me that asymmetry was introduced by the check for
reg_state != NETREG_REGISTERED in mlx5e_nic_enable() by:

610e89e05c3f ("net/mlx5e: Don't sync netdev state when not registered")

So perhaps that is a more appropriate commit for the Fixes tag.
I do note that commit was a fix for:

26e59d8077a3 ("net/mlx5e: Implement mlx5e interface attach/detach callbacks")

So perhaps a second fixes tag for that commit is also appropriate.

Perhaps it's not important enough to revise things, I don't feel strongly
about it, so feel free to add the following regardless.

Reviewed-by: Simon Horman <horms@...nel.org>

All that said, I do wonder if it would be better in the long run to
implement things in such a way that there is symmetry in resource
allocation / deallocation. Passing flags to control how much cleanup is
performed does seem a bit awkward.