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: <874iuwxsew.fsf@mail.parknet.co.jp>
Date: Tue, 29 Jul 2025 00:10:31 +0900
From: OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
To: Edward Adam Davis <eadavis@...com>
Cc: syzbot+d3c29ed63db6ddf8406e@...kaller.appspotmail.com,
        linkinjeon@...nel.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, sj1557.seo@...sung.com,
        syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH] fat: Prevent the race of read/write the FAT16 and FAT32
 entry

Edward Adam Davis <eadavis@...com> writes:

> The writer and reader access FAT32 entry without any lock, so the data
> obtained by the reader is incomplete.
>
> Add spin lock to solve the race condition that occurs when accessing
> FAT32 entry.
>
> FAT16 entry has the same issue and is handled together.

What is the real issue? Counting free entries doesn't care whether EOF
(0xffffff) or allocate (0x000068), so it should be same result on both
case.

We may want to use READ_ONCE/WRITE_ONCE though, I can't see the reason
to add spin_lock.

Thanks.

> Reported-by: syzbot+d3c29ed63db6ddf8406e@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=d3c29ed63db6ddf8406e
> Signed-off-by: Edward Adam Davis <eadavis@...com>
> ---
>  fs/fat/fatent.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/fs/fat/fatent.c b/fs/fat/fatent.c
> index a7061c2ad8e4..0e64875e932c 100644
> --- a/fs/fat/fatent.c
> +++ b/fs/fat/fatent.c
> @@ -19,6 +19,8 @@ struct fatent_operations {
>  };
>  
>  static DEFINE_SPINLOCK(fat12_entry_lock);
> +static DEFINE_SPINLOCK(fat16_entry_lock);
> +static DEFINE_SPINLOCK(fat32_entry_lock);
>  
>  static void fat12_ent_blocknr(struct super_block *sb, int entry,
>  			      int *offset, sector_t *blocknr)
> @@ -137,8 +139,13 @@ static int fat12_ent_get(struct fat_entry *fatent)
>  
>  static int fat16_ent_get(struct fat_entry *fatent)
>  {
> -	int next = le16_to_cpu(*fatent->u.ent16_p);
> +	int next;
> +
> +	spin_lock(&fat16_entry_lock);
> +	next = le16_to_cpu(*fatent->u.ent16_p);
>  	WARN_ON((unsigned long)fatent->u.ent16_p & (2 - 1));
> +	spin_unlock(&fat16_entry_lock);
> +
>  	if (next >= BAD_FAT16)
>  		next = FAT_ENT_EOF;
>  	return next;
> @@ -146,8 +153,13 @@ static int fat16_ent_get(struct fat_entry *fatent)
>  
>  static int fat32_ent_get(struct fat_entry *fatent)
>  {
> -	int next = le32_to_cpu(*fatent->u.ent32_p) & 0x0fffffff;
> +	int next;
> +
> +	spin_lock(&fat32_entry_lock);
> +	next = le32_to_cpu(*fatent->u.ent32_p) & 0x0fffffff;
>  	WARN_ON((unsigned long)fatent->u.ent32_p & (4 - 1));
> +	spin_unlock(&fat32_entry_lock);
> +
>  	if (next >= BAD_FAT32)
>  		next = FAT_ENT_EOF;
>  	return next;
> @@ -180,15 +192,21 @@ static void fat16_ent_put(struct fat_entry *fatent, int new)
>  	if (new == FAT_ENT_EOF)
>  		new = EOF_FAT16;
>  
> +	spin_lock(&fat16_entry_lock);
>  	*fatent->u.ent16_p = cpu_to_le16(new);
> +	spin_unlock(&fat16_entry_lock);
> +
>  	mark_buffer_dirty_inode(fatent->bhs[0], fatent->fat_inode);
>  }
>  
>  static void fat32_ent_put(struct fat_entry *fatent, int new)
>  {
>  	WARN_ON(new & 0xf0000000);
> +	spin_lock(&fat32_entry_lock);
>  	new |= le32_to_cpu(*fatent->u.ent32_p) & ~0x0fffffff;
>  	*fatent->u.ent32_p = cpu_to_le32(new);
> +	spin_unlock(&fat32_entry_lock);
> +
>  	mark_buffer_dirty_inode(fatent->bhs[0], fatent->fat_inode);
>  }

-- 
OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>