linux-kernel - Re: [PATCH V2] fat: Prevent the race of read/write the 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] [day] [month] [year] [list]

Message-ID: <20250729100635.GH222315@ZenIV>
Date: Tue, 29 Jul 2025 11:06:35 +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 V2] fat: Prevent the race of read/write the FAT32 entry

On Tue, Jul 29, 2025 at 10:35:58AM +0100, Al Viro wrote:
> On Tue, Jul 29, 2025 at 02:17:10PM +0800, Edward Adam Davis wrote:
> > syzbot reports data-race in fat32_ent_get/fat32_ent_put. 
> > 
> > 	CPU0(Task A)			CPU1(Task B)
> > 	====				====
> > 	vfs_write
> > 	new_sync_write
> > 	generic_file_write_iter
> > 	fat_write_begin
> > 	block_write_begin		vfs_statfs
> > 	fat_get_block			statfs_by_dentry
> > 	fat_add_cluster			fat_statfs

Sorry, no - you've missed an intermediate fat_chain_add() in the call chain
here.  And that makes your race a non-issue.

> > 	fat_ent_write			fat_count_free_clusters
> > 	fat32_ent_put			fat32_ent_get

fat_count_free_clusters() doesn't care about exact value of entry;
the only thing that matters is whether it's equal to FAT_ENT_FREE.

Actualy changes of that predicate (i.e. allocating and freeing of
clusters) still happens under fat_lock() - nothing has changed in
that area.  *That* is not happening simultaneously with reads in
fat_count_free_clusters().  It's attaching the new element to the
end of chain that is outside of fat_lock().

And that operation does not affect that predicate at all; it changes
the value of the entry for last cluster of file (FAT_ENT_EOF) to the
number of cluster being added to the file.  Neither is equal to
FAT_ENT_FREE, so there's no problem - value you get ->ent_get()
is affected by that store, the value of
	ops->ent_get(&fatent) == FAT_ENT_FREE
isn't.  Probably worth a comment in fat_chain_add() re "this store
is safe outside of fat_lock() because of <list of reasons>".
Changes to this chain (both extending and truncating it) are
excluded by <lock> and as far as allocator is concerned, we are
not changing the state of any cluster.

Basically, FAT encodes both the free block map (entry[block] == FAT_ENT_FREE
<=> block is not in use) and linked lists of blocks for individual files -
they store the number of file's first block within directory entry and
use FAT for forwards pointers in the list.  FAT_ENT_EOF is used for "no
more blocks, it's the end of file".  Allocator and fat_count_free_clusters
care only about the free block map part of that thing; access to file
contents - the linked list for that file.