Re: [PATCH v3] fat: calculate data area start without overflow

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

hengyul@cs.unc.edu writes:

> From: Hengyu Liang <hengyul@cs.unc.edu>
>
> On 32-bit architectures, sbi->fat_length, sbi->dir_start and
> sbi->data_start are unsigned long. The number of FATs is an 8-bit BPB
> field, while the FAT32 length is a 32-bit BPB field. Therefore, the
> calculation
>
>     sbi->fat_start + sbi->fats * sbi->fat_length
>
> can wrap before data_start is checked against total_sectors.
>
> For example, with fat_start=32, fats=2 and fat_length=0x80000001,
> the unwrapped data area start is 0x100000022 (4294967330), but the
> calculation wraps to 34 on i386. With total_sectors=36, the
> validation then incorrectly passes.
>
> The following script creates an image that demonstrates the problem:
>
>     python3 - <<'PY'
>     import struct
>
>     S = 512
>     b = bytearray(36 * S)
>
>     def p(off, fmt, value):
>         struct.pack_into(fmt, b, off, value)
>
>     # FAT32 BPB
>     b[0:3] = b'\xeb\x58\x90'
>     b[3:11] = b'MSWIN4.1'
>     p(11, '<H', 512)
>     b[13] = 1
>     p(14, '<H', 32)             # reserved sectors
>     b[16] = 2                   # number of FATs
>     p(17, '<H', 0)
>     p(19, '<H', 0)
>     b[21] = 0xf8
>     p(22, '<H', 0)
>     p(24, '<H', 1)
>     p(26, '<H', 1)
>     p(32, '<I', 36)             # total sectors
>     p(36, '<I', 0x80000001)     # FAT32 length
>     p(44, '<I', 2)              # root cluster
>     p(48, '<H', 1)              # FSINFO sector
>     b[64] = 0x80
>     b[66] = 0x29
>     p(67, '<I', 0x12345678)
>     b[71:82] = b'OVERFLOW   '
>     b[82:90] = b'FAT32   '
>     b[510:512] = b'\x55\xaa'
>
>     # FSINFO
>     p(S, '<I', 0x41615252)
>     p(S + 484, '<I', 0x61417272)
>     p(S + 488, '<I', 0xffffffff)
>     p(S + 492, '<I', 0xffffffff)
>
>     # Wrapped FAT starts at sector 32
>     fat = 32 * S
>     p(fat + 8, '<I', 0x0fffffff)   # FAT[2]
>     p(fat + 12, '<I', 0x0fffffff)  # FAT[3]
>
>     # Wrapped data_start == 34
>     root = 34 * S
>     b[root:root + 11] = b'ESCAPE  TXT'
>     b[root + 11] = 0x20
>     p(root + 26, '<H', 3)
>     p(root + 28, '<I', 4)
>     b[35 * S:35 * S + 4] = b'OOB!'
>
>     open('fat-overflow.img', 'wb').write(b)
>     PY
>
> Attach fat-overflow.img as /dev/sdb and run:
>
>     mount -t vfat -o ro /dev/sdb /mnt
>     cat /mnt/ESCAPE.TXT
>
> On an unpatched i386 kernel, the mount succeeds and reading the file
> returns:
>
>     OOB!
>
> On an x86-64 kernel and on a patched i386 kernel, mounting is rejected
> with:
>
>     mount: mounting /dev/sdb on /mnt failed: Invalid argument
>
> Calculate dir_start and data_start in u32 using
> check_mul_overflow() and check_add_overflow(). Since total_sectors is
> also a u32 value, any result that overflows u32 cannot describe a valid
> volume. Reject such layouts before storing the values in the existing
> unsigned long fields. The existing total_sectors check handles values
> that fit in u32 but still lie beyond the volume.
>
> Signed-off-by: Hengyu Liang <hengyul@cs.unc.edu>

Thank you. Looks good.

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

> ---
> Changes in v3:
> - Keep the multiplication check on one line for readability.
> - Report overflow before rejecting the volume.
>
>  fs/fat/inode.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/fs/fat/inode.c b/fs/fat/inode.c
> index f775a004cae1..0b0bbe777842 100644
> --- a/fs/fat/inode.c
> +++ b/fs/fat/inode.c
> @@ -20,6 +20,7 @@
>  #include <linux/blkdev.h>
>  #include <linux/backing-dev.h>
>  #include <linux/unaligned.h>
> +#include <linux/overflow.h>
>  #include <linux/random.h>
>  #include <linux/iversion.h>
>  #include <linux/fs_struct.h>
> @@ -1577,6 +1578,7 @@ int fat_fill_super(struct super_block *sb, struct fs_context *fc,
>  	struct msdos_sb_info *sbi;
>  	u16 logical_sector_size;
>  	u32 total_sectors, total_clusters, fat_clusters, rootdir_sectors;
> +	u32 dir_start, data_start;
>  	long error;
>  	char buf[50];
>  	struct timespec64 ts;
> @@ -1752,7 +1754,6 @@ int fat_fill_super(struct super_block *sb, struct fs_context *fc,
>  	sbi->dir_per_block = sb->s_blocksize / sizeof(struct msdos_dir_entry);
>  	sbi->dir_per_block_bits = ffs(sbi->dir_per_block) - 1;
>  
> -	sbi->dir_start = sbi->fat_start + sbi->fats * sbi->fat_length;
>  	sbi->dir_entries = bpb.fat_dir_entries;
>  	if (sbi->dir_entries & (sbi->dir_per_block - 1)) {
>  		if (!silent)
> @@ -1763,20 +1764,30 @@ int fat_fill_super(struct super_block *sb, struct fs_context *fc,
>  
>  	rootdir_sectors = sbi->dir_entries
>  		* sizeof(struct msdos_dir_entry) / sb->s_blocksize;
> -	sbi->data_start = sbi->dir_start + rootdir_sectors;
> +	if (check_mul_overflow(sbi->fats, sbi->fat_length, &dir_start) ||
> +	    check_add_overflow(sbi->fat_start, dir_start, &dir_start) ||
> +	    check_add_overflow(dir_start, rootdir_sectors, &data_start)) {
> +		if (!silent)
> +			fat_msg(sb, KERN_ERR,
> +				"overflow of root dir or data layout");
> +		goto out_invalid;
> +	}
> +
>  	total_sectors = bpb.fat_sectors;
>  	if (total_sectors == 0)
>  		total_sectors = bpb.fat_total_sect;
>  
> -	if (total_sectors < sbi->data_start) {
> +	if (total_sectors < data_start) {
>  		if (!silent)
>  			fat_msg(sb, KERN_ERR,
> -				"data area starts beyond volume (%lu > %u)",
> -				sbi->data_start, total_sectors);
> +				"data area starts beyond volume (%u > %u)",
> +				data_start, total_sectors);
>  		goto out_invalid;
>  	}
>  
> -	total_clusters = (total_sectors - sbi->data_start) / sbi->sec_per_clus;
> +	sbi->dir_start = dir_start;
> +	sbi->data_start = data_start;
> +	total_clusters = (total_sectors - data_start) / sbi->sec_per_clus;
>  
>  	if (!is_fat32(sbi))
>  		sbi->fat_bits = (total_clusters > MAX_FAT12) ? 16 : 12;
>

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