linux-kernel - Re: [PATCH] wifi: ath12k: fix 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] [day] [month] [year] [list]

Message-ID: <aDcpzTI-Bjry144Z@hovoldconsulting.com>
Date: Wed, 28 May 2025 17:20:45 +0200
From: Johan Hovold <johan@...nel.org>
To: Remi Pommarel <repk@...plefau.lt>
Cc: Johan Hovold <johan+linaro@...nel.org>,
	Jeff Johnson <jjohnson@...nel.org>,
	Miaoqing Pan <quic_miaoqing@...cinc.com>,
	Steev Klimaszewski <steev@...i.org>,
	Clayton Craft <clayton@...ftyguy.net>,
	Jens Glathe <jens.glathe@...schoolsolutions.biz>,
	Nicolas Escande <nico.escande@...il.com>,
	ath12k@...ts.infradead.org, linux-kernel@...r.kernel.org,
	stable@...r.kernel.org
Subject: Re: [PATCH] wifi: ath12k: fix ring-buffer corruption

On Mon, May 26, 2025 at 02:58:51PM +0200, Remi Pommarel wrote:
> On Mon, May 26, 2025 at 01:35:02PM +0200, Johan Hovold wrote:
> > On Thu, May 22, 2025 at 05:11:21PM +0200, Remi Pommarel wrote:

> > > Why not move the dma_rmb() in ath12k_hal_srng_access_begin() as below,
> > > that would look to me as a good place to do it.

> > We only need the read barrier for dest rings so the barrier would go in
> > the else branch, but I prefer keeping it in the caller so that it is
> > more obvious when it is needed and so that we can skip the barrier when
> > the ring is empty (e.g. as done above).
> 
> Thanks for taking time to clarify this.
> 
> Yes I messed up doing the patch by hand sorry, internally I test with
> the dma_rmb() in the else part. I tend to prefer having it in
> ath12k_hal_srng_access_begin() as caller does not have to take care of
> the barrier itself. Which for me seems a little bit risky if further
> refactoring (or adding other ring processing) is done in the future;
> the barrier could easily be forgotten don't you think ?

Yeah, that would be the argument for putting in the helper. Big hammer
vs adding it where needed after reviewing the code.

There actually is a new ring being added for 6.16-rc1 I noticed after I
posted the latest series. That would require a follow-up fix with the
barrier-in-caller approach.

> > > dma_rmb() acting also as a compiler barrier why the need for both
> > > READ_ONCE() ?
> > 
> > Yeah, I was being overly cautious here and it should be fine with plain
> > accesses when reading the descriptor after the barrier, but the memory
> > model seems to require READ_ONCE() when fetching the head pointer.
> > Currently, hp_addr is marked as volatile so READ_ONCE() could be
> > dropped for that reason, but I'd rather keep it here explicitly (e.g. in
> > case someone decides to drop the volatile).
> 
> Yes actually after more thinking, the READ_ONCE for fetching hp does make
> sense and is also in the patch I am currently testing.
> 
> Also for source rings don't we need a dma_wmb()/WRITE_ONCE before
> modifying the tail pointer (see ath12k_hal_srng_access_end()) for quite
> the same reason (updates of the descriptor have to be visible before
> write to tail pointer) ?

Yep, the source rings need explicit barriers for the LMAC case, but
there are further issues here too.

And that may also suggest adding the barriers in the start/end helpers
for consistency (i.e. use the big hammer).

I'll try to find some more time to fix the remaining bits next week.

Johan