netdev - Re: [PATCH iwl-net 3/3] idpf: fix UAFs when destroying the queues

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: <20240726162214.GQ97837@kernel.org>
Date: Fri, 26 Jul 2024 17:22:14 +0100
From: Simon Horman <horms@...nel.org>
To: Alexander Lobakin <aleksander.lobakin@...el.com>
Cc: intel-wired-lan@...ts.osuosl.org,
	Tony Nguyen <anthony.l.nguyen@...el.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	nex.sw.ncis.osdt.itp.upstreaming@...el.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Michal Kubiak <michal.kubiak@...el.com>
Subject: Re: [PATCH iwl-net 3/3] idpf: fix UAFs when destroying the queues

On Wed, Jul 24, 2024 at 03:40:24PM +0200, Alexander Lobakin wrote:
> The second tagged commit started sometimes (very rarely, but possible)
> throwing WARNs from
> net/core/page_pool.c:page_pool_disable_direct_recycling().
> Turned out idpf frees interrupt vectors with embedded NAPIs *before*
> freeing the queues making page_pools' NAPI pointers lead to freed
> memory before these pools are destroyed by libeth.
> It's not clear whether there are other accesses to the freed vectors
> when destroying the queues, but anyway, we usually free queue/interrupt
> vectors only when the queues are destroyed and the NAPIs are guaranteed
> to not be referenced anywhere.
> 
> Invert the allocation and freeing logic making queue/interrupt vectors
> be allocated first and freed last. Vectors don't require queues to be
> present, so this is safe. Additionally, this change allows to remove
> that useless queue->q_vector pointer cleanup, as vectors are still
> valid when freeing the queues (+ both are freed within one function,
> so it's not clear why nullify the pointers at all).
> 
> Fixes: 1c325aac10a8 ("idpf: configure resources for TX queues")
> Fixes: 90912f9f4f2d ("idpf: convert header split mode to libeth + napi_build_skb()")
> Reported-by: Michal Kubiak <michal.kubiak@...el.com>
> Signed-off-by: Alexander Lobakin <aleksander.lobakin@...el.com>

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