Re: [PATCH] fat: check for hidden entries before overwriting end-of-directory marker

[OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>]

Hengyu Liang <hengyul@cs.unc.edu> writes:

> Since commit 6a2875517c77 ("fat: stop reading directory entries past the
> end-of-directory marker"), lookup and readdir stop at a slot whose first
> name byte is 0, and fat_add_entries() reports corruption when its scan
> finds an allocated entry after that slot.
>
> The scan stops as soon as it has enough free slots, though. When those
> slots include the marker, the new entries overwrite it and the allocated
> entries after it become visible again without being checked. A name that
> lookup could not find can then appear twice in the directory, and removing
> one copy leaves readdir listing a name that stat cannot find.

Yes, it is true. However, first of all that state is already corrupted
FAT filesystem.  And there would be many cases like this on corrupted
image. If we want to detect as possible as many corruptions, I guess it
will become finally the something like online fsck.

So I think we should not add additional sanity check has overhead
(especially to normal path) if it doesn't crash the kernel. Simple
sanity check is ok though.

Thanks.

> For example:
>
>   mkfs.fat -F 16 -C fat.img 32768
>   mount -t msdos -o loop fat.img /mnt
>   touch /mnt/aaa.txt /mnt/bbb.txt /mnt/file1
>   umount /mnt
>   off=$(grep -obUa 'BBB     TXT' fat.img | cut -d: -f1)
>   dd if=/dev/zero of=fat.img bs=1 seek=$off count=32 conv=notrunc
>   mount -t msdos -o loop fat.img /mnt
>   touch /mnt/file1
>   ls /mnt
>
> lists file1 twice.
>
> Before using free slots that include the marker, scan the slots up to the
> next marker and take the existing corruption path if one is allocated.
>
> Fixes: 6a2875517c77 ("fat: stop reading directory entries past the end-of-directory marker")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hengyu Liang <hengyul@cs.unc.edu>
> ---
> Tested on x86-64 KVM with KASAN, comparing mainline at d89e1d186ec9 with
> this patch:
> - The example above: the base kernel creates a second file1, which is
>   still there after a remount. With the patch, touch fails with EIO, the
>   error is logged and fsck.fat only finds the dirty bit.
> - vfat, three-slot create whose free run ends exactly at the marker:
>   the base kernel exposes the hidden file, the patch returns EIO.
> - vfat, one-slot create that leaves a zero slot after the new entry:
>   succeeds on both and the hidden file stays hidden.
> - Valid directories on FAT16 and FAT32, msdos and vfat (creates, deletes,
>   renames and mkdirs, then filling the root): the same entry counts, ENOSPC
>   on a full FAT16 root and no FAT-fs messages on both; fsck.fat -n exits 0.
>
>  fs/fat/dir.c | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/fs/fat/dir.c b/fs/fat/dir.c
> index 35bdb62944a2..ab99d53a719a 100644
> --- a/fs/fat/dir.c
> +++ b/fs/fat/dir.c
> @@ -1318,6 +1318,32 @@ static int fat_add_new_entries(struct inode *dir, void *slots, int nr_slots,
>  	return err;
>  }
>  
> +/*
> + * The free slots chosen by fat_add_entries() include the end-of-directory
> + * marker.  Once the new entries are written over it, the slots up to the
> + * next marker become visible, so make sure that none of them is allocated.
> + */
> +static int fat_check_after_eod(struct inode *dir, loff_t pos)
> +{
> +	struct buffer_head *bh = NULL;
> +	struct msdos_dir_entry *de = NULL;
> +	int err = 0;
> +
> +	while (fat_get_entry(dir, &pos, &bh, &de) > -1) {
> +		if (de->name[0] == 0)
> +			break;
> +		if (!IS_FREE(de->name)) {
> +			fat_fs_error_ratelimit(dir->i_sb,
> +				"allocated dir entry found after end-of-directory marker (i_pos %lld)",
> +				MSDOS_I(dir)->i_pos);
> +			err = -EIO;
> +			break;
> +		}
> +	}
> +	brelse(bh);
> +	return err;
> +}
> +
>  int fat_add_entries(struct inode *dir, void *slots, int nr_slots,
>  		    struct fat_slot_info *sinfo)
>  {
> @@ -1351,8 +1377,14 @@ int fat_add_entries(struct inode *dir, void *slots, int nr_slots,
>  				nr_bhs++;
>  			}
>  			free_slots++;
> -			if (free_slots == nr_slots)
> +			if (free_slots == nr_slots) {
> +				if (saw_eod) {
> +					err = fat_check_after_eod(dir, pos);
> +					if (err)
> +						goto error;
> +				}
>  				goto found;
> +			}
>  		} else {
>  			if (saw_eod) {
>  				fat_fs_error_ratelimit(sb,
>
> base-commit: d89e1d186ec9271bf78e62771779e9fc03bf0fc3

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