linux-kernel - Re: [PATCH 1/3] wifi: ath11k: fix dest ring-buffer corruption

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: <aD7h0OOoGjVm8pDK@hovoldconsulting.com>
Date: Tue, 3 Jun 2025 13:51:44 +0200
From: Johan Hovold <johan@...nel.org>
To: Baochen Qiang <quic_bqiang@...cinc.com>
Cc: Miaoqing Pan <quic_miaoqing@...cinc.com>,
	Johan Hovold <johan+linaro@...nel.org>,
	Jeff Johnson <jjohnson@...nel.org>, linux-wireless@...r.kernel.org,
	ath11k@...ts.infradead.org, linux-kernel@...r.kernel.org,
	stable@...r.kernel.org
Subject: Re: [PATCH 1/3] wifi: ath11k: fix dest ring-buffer corruption

On Tue, Jun 03, 2025 at 06:52:37PM +0800, Baochen Qiang wrote:
> On 6/2/2025 4:03 PM, Johan Hovold wrote:

> > No, the barrier is needed between reading the head pointer and accessing
> > descriptor fields, that's what matters.
> > 
> > You can still end up with reading stale descriptor data even when
> > ath11k_hal_srng_dst_get_next_entry() returns non-NULL due to speculation
> > (that's what happens on the X13s).
> 
> The fact is that a dma_rmb() does not even prevent speculation, no matter where it is
> placed, right?

It prevents the speculated load from being used.

> If so the whole point of dma_rmb() is to prevent from compiler reordering
> or CPU reordering, but is it really possible?
> 
> The sequence is
> 
> 	1# reading HP
> 		srng->u.dst_ring.cached_hp = READ_ONCE(*srng->u.dst_ring.hp_addr);
> 
> 	2# validate HP
> 		if (srng->u.dst_ring.tp == srng->u.dst_ring.cached_hp)
> 			return NULL;
> 
> 	3# get desc
> 		desc = srng->ring_base_vaddr + srng->u.dst_ring.tp;
> 
> 	4# accessing desc
> 		ath11k_hal_desc_reo_parse_err(... desc, ...)
> 
> Clearly each step depends on the results of previous steps. In this case the compiler/CPU
> is expected to be smart enough to not do any reordering, isn't it?

Steps 3 and 4 can be done speculatively before the load in step 1 is
complete as long as the result is discarded if it turns out not to be
needed.

Johan