[PATCH v2] fat: calculate data area start without overflow

[PATCH v2] fat: calculate data area start without overflow

[hengyul]

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>
---
Changes in v2:
- Use u32 temporaries with check_*_overflow() instead of u64 arithmetic.
- Check the FAT multiplication and both additions before using the layout.

 fs/fat/inode.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index f775a004cae1..020138ce35a6 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,27 @@ 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))
+		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;

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

[OGAWA Hirofumi]

hengyul@cs.unc.edu writes:

> -	sbi->data_start = sbi->dir_start + rootdir_sectors;
> +	if (check_mul_overflow(sbi->fats, sbi->fat_length,
> +			       &dir_start) ||

Looks like unreadable "\n".

	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))
> +		goto out_invalid;

Maybe, better to add the error detail?

	    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;
	}

Thanks.

>  	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>