linux-kernel - Re: [PATCH] fat: Prevent the race of read/write the FAT16 and FAT32 entry

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: <20250729045323.GE222315@ZenIV>
Date: Tue, 29 Jul 2025 05:53:23 +0100
From: Al Viro <viro@...iv.linux.org.uk>
To: Edward Adam Davis <eadavis@...com>
Cc: hirofumi@...l.parknet.co.jp, linkinjeon@...nel.org,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	sj1557.seo@...sung.com,
	syzbot+d3c29ed63db6ddf8406e@...kaller.appspotmail.com,
	syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH] fat: Prevent the race of read/write the FAT16 and FAT32
 entry

On Tue, Jul 29, 2025 at 12:17:21PM +0800, Edward Adam Davis wrote:
> > >
> > > Could you be more specific?  "Incomplete" in which sense?
> Because ent32_p and ent12_p are in the same union [1], their addresses are
> the same, and they both have the "read/write race condition" problem, so I
> used the same method as [2] to add a spinlock to solve it.

What the hell?  ent32_p and ent12_p are _pointers_; whatever races there
might be are about the objects they are pointing to.

> > > Which race condition would that be?
> data-race in fat32_ent_get / fat32_ent_put, detail: see [3]

References to KCSAN output are worthless, unless you can explain what the
actual problem is (no, "tool is not quiet" is *NOT* a problem; it might
or might not be a symptom of one).

> > Note that FAT12 situation is really different - we not just have an inevitable
> > read-modify-write for stores (half-byte access), we are not even guaranteed that
> > byte and half-byte will be within the same cacheline, so cmpxchg is not an
> > option; we have to use a spinlock there.
> I think for FAT12 they are always in the same cacheline, the offset of the
> member ent12_p in struct fat_entry is 4 bytes, and no matter x86-64 or arm64,
> the regular 64-byte cacheline is enough to ensure that they are in the same
> cacheline.

Have you actually read what these functions are doing?  _Pointers_ are not
problematic at all; neither ..._ent_get nor ..._ent_put are changing those,
for crying out loud!

If KCSAN is warning about those (which I sincerely doubt), you need to report
a bug in KCSAN.  I would *really* recommend verifying that first.

FAT12 problem is that FAT entries being accessed there are 12-bit, packed in
pairs into an array of 3-byte values.  Have you actually read what the functions
are doing?  There we *must* serialize the access to bytes that have 4 bits
from one entry and 4 from another - there's no such thing as atomically
update half a byte; it has to be read, modified and stored back.  If two
threads try to do that to upper and lower halves of the same byte at the
same time, the value will be corrupted.

The same *might* happen to FAT16 on (sub)architectures we no longer support;
there is  no way in hell we run into anything of that sort for (aligned) 32bit
stores.  Never had been.  And neither aligned 16bit nor aligned 32bit ever
had a problem with read seeing a state with only a part of value having
been written.