| Message ID | 1466758690-12354-1-git-send-email-lv.zheng.2015@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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)
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
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
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
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
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
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.
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)
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(+)