[PATCH] fat: check whether fs size exceeds device size

[PATCH] fat: check whether fs size exceeds device size

[Zheng Lv]

The original code did not check for size of device. A truncated device
would mount, I/O failures would occur when "attempt to access beyond end
of device", leading to data lost.

Fix this by comparing total-sectors field in BPB with size of device.

This commit also prints a KERN_INFO message if there are extra sectors
at end of device (ie. total sectors < device sectors).

Signed-off-by: Zheng Lv <lv.zheng.2015@gmail.com>
---
 fs/fat/inode.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index 3bcf579..211f7bb 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -1583,6 +1583,7 @@ int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
 	struct msdos_sb_info *sbi;
 	u16 logical_sector_size;
 	u32 total_sectors, total_clusters, fat_clusters, rootdir_sectors;
+	u64 device_sectors;
 	int debug;
 	long error;
 	char buf[50];
@@ -1738,6 +1739,17 @@ int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
 	if (total_sectors == 0)
 		total_sectors = bpb.fat_total_sect;
 
+	device_sectors = sb->s_bdev->bd_inode->i_size / logical_sector_size;
+	if (device_sectors && total_sectors > device_sectors) {
+		fat_msg(sb, KERN_ERR, "total sectors %u "
+			"exceeds size of device (%llu sectors)",
+			total_sectors, device_sectors);
+		goto out_invalid;
+	} else if (device_sectors && total_sectors < device_sectors) {
+		fat_msg(sb, KERN_INFO, "%llu unused sectors at end of device",
+			device_sectors - total_sectors);
+	}
+
 	total_clusters = (total_sectors - sbi->data_start) / sbi->sec_per_clus;
 
 	if (sbi->fat_bits != 32)
-- 
2.7.4

Re: [PATCH] fat: check whether fs size exceeds device size

[OGAWA Hirofumi]

Zheng Lv <lv.zheng.2015@gmail.com> writes:

> The original code did not check for size of device. A truncated device
> would mount, I/O failures would occur when "attempt to access beyond end
> of device", leading to data lost.
>
> Fix this by comparing total-sectors field in BPB with size of device.
>
> This commit also prints a KERN_INFO message if there are extra sectors
> at end of device (ie. total sectors < device sectors).

[...]

> +	device_sectors = sb->s_bdev->bd_inode->i_size / logical_sector_size;
> +	if (device_sectors && total_sectors > device_sectors) {
> +		fat_msg(sb, KERN_ERR, "total sectors %u "
> +			"exceeds size of device (%llu sectors)",
> +			total_sectors, device_sectors);
> +		goto out_invalid;

Hm, it is a bit hard to think what to do. But I guess it is better to
allow access to rescue some files. (Yes, it may lost new data. But
read-only or in-place update is safe.)

> +	} else if (device_sectors && total_sectors < device_sectors) {
> +		fat_msg(sb, KERN_INFO, "%llu unused sectors at end of device",
> +			device_sectors - total_sectors);

This is legal setup. So, probably, should not pollute log for each mount.

> +	}
> +
>  	total_clusters = (total_sectors - sbi->data_start) / sbi->sec_per_clus;
>  
>  	if (sbi->fat_bits != 32)

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

Re: [PATCH] fat: check whether fs size exceeds device size

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 982 bytes --]

Hi,

[auto build test ERROR on v4.7-rc4]
[also build test ERROR on next-20160624]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Zheng-Lv/fat-check-whether-fs-size-exceeds-device-size/20160624-170317
config: arm-shannon_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 5.3.1-8) 5.3.1 20160205
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   fs/built-in.o: In function `fat_fill_super':
>> compr_zlib.c:(.text+0x6f880): undefined reference to `__divdi3'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 12199 bytes --]

Re: [PATCH] fat: check whether fs size exceeds device size

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 906 bytes --]

Hi,

[auto build test ERROR on v4.7-rc4]
[also build test ERROR on next-20160624]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Zheng-Lv/fat-check-whether-fs-size-exceeds-device-size/20160624-170317
config: sh-sh03_defconfig (attached as .config)
compiler: sh4-linux-gnu-gcc (Debian 5.3.1-8) 5.3.1 20160205
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sh 

All errors (new ones prefixed by >>):

>> ERROR: "__divdi3" [fs/fat/fat.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 12214 bytes --]

Re: [PATCH] fat: check whether fs size exceeds device size

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1495 bytes --]

Hi,

[auto build test ERROR on v4.7-rc4]
[also build test ERROR on next-20160624]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Zheng-Lv/fat-check-whether-fs-size-exceeds-device-size/20160624-170317
config: arm-u8500_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 5.3.1-8) 5.3.1 20160205
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   fs/built-in.o: In function `fat_fill_super':
>> fs/fat/inode.c:1742: undefined reference to `__aeabi_ldivmod'

vim +1742 fs/fat/inode.c

  1736			* sizeof(struct msdos_dir_entry) / sb->s_blocksize;
  1737		sbi->data_start = sbi->dir_start + rootdir_sectors;
  1738		total_sectors = bpb.fat_sectors;
  1739		if (total_sectors == 0)
  1740			total_sectors = bpb.fat_total_sect;
  1741	
> 1742		device_sectors = sb->s_bdev->bd_inode->i_size / logical_sector_size;
  1743		if (device_sectors && total_sectors > device_sectors) {
  1744			fat_msg(sb, KERN_ERR, "total sectors %u "
  1745				"exceeds size of device (%llu sectors)",

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 21066 bytes --]

Re: [PATCH] fat: check whether fs size exceeds device size

[Zheng Lv]

On Fri, Jun 24, 2016 at 06:34:25PM +0900, OGAWA Hirofumi wrote:
> > +	device_sectors = sb->s_bdev->bd_inode->i_size / logical_sector_size;
> > +	if (device_sectors && total_sectors > device_sectors) {
> > +		fat_msg(sb, KERN_ERR, "total sectors %u "
> > +			"exceeds size of device (%llu sectors)",
> > +			total_sectors, device_sectors);
> > +		goto out_invalid;
> 
> Hm, it is a bit hard to think what to do. But I guess it is better to
> allow access to rescue some files. (Yes, it may lost new data. But
> read-only or in-place update is safe.)

I would like to list the reasons it's better not to allow mounting.

 - The "attempt to access beyond end of device" error would fill the kernel
   log. It's drivers' business to prevent such kind of errors.
 - Clever data rescuers won't mount the damaged device. They would instead
   mount a copy of the broken image. It isn't much work to "truncate" the file
   to larger after they receive the message. 
 - It's at least unsafe to allow mounting rw a truncated device.
 - ext4 driver forbids mounting a truncated device, too.

In fact, the code was basically copied from ext4 fs driver. :-)

Reference: v4.7-rc4 fs/ext4/super.c:3605
>	blocks_count = sb->s_bdev->bd_inode->i_size >> sb->s_blocksize_bits;
>	if (blocks_count && ext4_blocks_count(es) > blocks_count) {
>		ext4_msg(sb, KERN_WARNING, "bad geometry: block count %llu "
>		       "exceeds size of device (%llu blocks)",
>		       ext4_blocks_count(es), blocks_count);
>		goto failed_mount;
>	}

On Fri, Jun 24, 2016 at 06:34:25PM +0900, OGAWA Hirofumi wrote:
> > +	} else if (device_sectors && total_sectors < device_sectors) {
> > +		fat_msg(sb, KERN_INFO, "%llu unused sectors at end of device",
> > +			device_sectors - total_sectors);
> 
> This is legal setup. So, probably, should not pollute log for each mount.

I totally agree with you.

Zheng Lv

Re: [PATCH] fat: check whether fs size exceeds device size

[Zheng Lv]

The 64-bit division seems to cause build failure on 32-bit ARM, we must play
some tricks to solve this.

[PATCH] fat: check whether fs size exceeds device size

[Zheng Lv]

The original code did not check for size of device. A truncated device
would mount, I/O failures would occur when "attempt to access beyond end
of device", leading to data lost.

Fix this by comparing total-sectors field in BPB with size of device.

Signed-off-by: Zheng Lv <lv.zheng.2015@gmail.com>
---
This patch remove the unwanted "else" block and eliminate use of division.


 fs/fat/inode.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index 3bcf579..d852d51 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -1583,6 +1583,7 @@ int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
 	struct msdos_sb_info *sbi;
 	u16 logical_sector_size;
 	u32 total_sectors, total_clusters, fat_clusters, rootdir_sectors;
+	u64 device_sectors;
 	int debug;
 	long error;
 	char buf[50];
@@ -1738,6 +1739,15 @@ int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
 	if (total_sectors == 0)
 		total_sectors = bpb.fat_total_sect;
 
+	device_sectors = sb->s_bdev->bd_inode->i_size >>
+			(ffs(logical_sector_size) - 1);
+	if (device_sectors && total_sectors > device_sectors) {
+		fat_msg(sb, KERN_ERR, "total sectors %u "
+			"exceeds size of device (%llu sectors)",
+			total_sectors, device_sectors);
+		goto out_invalid;
+	}
+
 	total_clusters = (total_sectors - sbi->data_start) / sbi->sec_per_clus;
 
 	if (sbi->fat_bits != 32)
-- 
2.7.4

Re: [PATCH] fat: check whether fs size exceeds device size

[OGAWA Hirofumi]

Zheng Lv <lv.zheng.2015@gmail.com> writes:

>> Hm, it is a bit hard to think what to do. But I guess it is better to
>> allow access to rescue some files. (Yes, it may lost new data. But
>> read-only or in-place update is safe.)
>
> I would like to list the reasons it's better not to allow mounting.
>
>  - The "attempt to access beyond end of device" error would fill the kernel
>    log. It's drivers' business to prevent such kind of errors.
>  - Clever data rescuers won't mount the damaged device. They would instead
>    mount a copy of the broken image. It isn't much work to "truncate" the file
>    to larger after they receive the message. 
>  - It's at least unsafe to allow mounting rw a truncated device.
>  - ext4 driver forbids mounting a truncated device, too.
>
> In fact, the code was basically copied from ext4 fs driver. :-)

I see. But a big difference with ext4 is, there is many bad
implementations of FAT.

My concern is formatted usb thumb (or mmc etc.) by bad formatter. Yes,
it is illegal format, but if user used only top of device (e.g. used to
move small file to move other machine), user would not notice bad
format, and works well until this patch applied.

So, windows also refuse to read/write? And dosfsck can fix? (and some
embedded device may not have to run fsck or mkfs).

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