fat: check whether fs size exceeds device size
diff mbox

Message ID 1466758690-12354-1-git-send-email-lv.zheng.2015@gmail.com
State New
Headers show

Commit Message

Zheng Lv June 24, 2016, 8:58 a.m. UTC
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(+)

Comments

OGAWA Hirofumi June 24, 2016, 9:34 a.m. UTC | #1
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)
kbuild test robot June 24, 2016, 9:38 a.m. UTC | #2
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
kbuild test robot June 24, 2016, 10:39 a.m. UTC | #3
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
kbuild test robot June 24, 2016, 10:53 a.m. UTC | #4
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
Zheng Lv June 24, 2016, 1:56 p.m. UTC | #5
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
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zheng Lv June 24, 2016, 2:02 p.m. UTC | #6
The 64-bit division seems to cause build failure on 32-bit ARM, we must play
some tricks to solve this.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
OGAWA Hirofumi June 25, 2016, 10:08 a.m. UTC | #7
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.

Patch
diff mbox

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)