Re: FAT: Operating on broken FAT FS causes the write syscall to return negative number not equal to -1

From: OGAWA Hirofumi
Date: Sun Jul 15 2018 - 10:20:23 EST


Anatoly Trosinenko <anatoly.trosinenko@xxxxxxxxx> writes:

> How to reproduce:
> 1) Compile v4.18-rc4 kernel with the attached config
> 1) Unpack the attached FS image (128 Mb) and mount it as vfat to /mnt
> 2) Compile and run vfat-bug.c
>
> What is expected:
> `write` returns either -1 or small positive number.
>
> What happens:
> The -13619152 aka 0xffffffffff303030 is returned.

This patch returns better error (-EIO) for me. (But note, the corrupted
FS image doesn't guarantee POSIX behavior.)

Thanks.


[PATCH] fat: Validate ->i_start before using

On corrupted FATfs may have invalid ->i_start. To handle it, this
checks ->i_start before using, and return proper error code.

Signed-off-by: OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx>
---

fs/fat/cache.c | 19 ++++++++++++-------
fs/fat/fat.h | 7 +++++++
fs/fat/fatent.c | 6 +++---
3 files changed, 22 insertions(+), 10 deletions(-)

diff -puN fs/fat/cache.c~fat-validate-i_start fs/fat/cache.c
--- linux/fs/fat/cache.c~fat-validate-i_start 2018-07-15 23:03:25.167171670 +0900
+++ linux-hirofumi/fs/fat/cache.c 2018-07-15 23:03:25.171171666 +0900
@@ -225,7 +225,8 @@ static inline void cache_init(struct fat
int fat_get_cluster(struct inode *inode, int cluster, int *fclus, int *dclus)
{
struct super_block *sb = inode->i_sb;
- const int limit = sb->s_maxbytes >> MSDOS_SB(sb)->cluster_bits;
+ struct msdos_sb_info *sbi = MSDOS_SB(sb);
+ const int limit = sb->s_maxbytes >> sbi->cluster_bits;
struct fat_entry fatent;
struct fat_cache_id cid;
int nr;
@@ -234,6 +235,12 @@ int fat_get_cluster(struct inode *inode,

*fclus = 0;
*dclus = MSDOS_I(inode)->i_start;
+ if (!fat_valid_entry(sbi, *dclus)) {
+ fat_fs_error_ratelimit(sb,
+ "%s: invalid start cluster (i_pos %lld, start %08x)",
+ __func__, MSDOS_I(inode)->i_pos, *dclus);
+ return -EIO;
+ }
if (cluster == 0)
return 0;

@@ -250,9 +257,8 @@ int fat_get_cluster(struct inode *inode,
/* prevent the infinite loop of cluster chain */
if (*fclus > limit) {
fat_fs_error_ratelimit(sb,
- "%s: detected the cluster chain loop"
- " (i_pos %lld)", __func__,
- MSDOS_I(inode)->i_pos);
+ "%s: detected the cluster chain loop (i_pos %lld)",
+ __func__, MSDOS_I(inode)->i_pos);
nr = -EIO;
goto out;
}
@@ -262,9 +268,8 @@ int fat_get_cluster(struct inode *inode,
goto out;
else if (nr == FAT_ENT_FREE) {
fat_fs_error_ratelimit(sb,
- "%s: invalid cluster chain (i_pos %lld)",
- __func__,
- MSDOS_I(inode)->i_pos);
+ "%s: invalid cluster chain (i_pos %lld)",
+ __func__, MSDOS_I(inode)->i_pos);
nr = -EIO;
goto out;
} else if (nr == FAT_ENT_EOF) {
diff -puN fs/fat/fat.h~fat-validate-i_start fs/fat/fat.h
--- linux/fs/fat/fat.h~fat-validate-i_start 2018-07-15 23:03:25.168171670 +0900
+++ linux-hirofumi/fs/fat/fat.h 2018-07-15 23:03:25.171171666 +0900
@@ -348,6 +348,13 @@ static inline void fatent_brelse(struct
fatent->fat_inode = NULL;
}

+static inline bool fat_valid_entry(struct msdos_sb_info *sbi, int entry)
+{
+ if (entry < FAT_START_ENT || sbi->max_cluster <= entry)
+ return false;
+ return true;
+}
+
extern void fat_ent_access_init(struct super_block *sb);
extern int fat_ent_read(struct inode *inode, struct fat_entry *fatent,
int entry);
diff -puN fs/fat/fatent.c~fat-validate-i_start fs/fat/fatent.c
--- linux/fs/fat/fatent.c~fat-validate-i_start 2018-07-15 23:03:25.169171668 +0900
+++ linux-hirofumi/fs/fat/fatent.c 2018-07-15 23:03:25.171171666 +0900
@@ -24,7 +24,7 @@ static void fat12_ent_blocknr(struct sup
{
struct msdos_sb_info *sbi = MSDOS_SB(sb);
int bytes = entry + (entry >> 1);
- WARN_ON(entry < FAT_START_ENT || sbi->max_cluster <= entry);
+ WARN_ON(!fat_valid_entry(sbi, entry));
*offset = bytes & (sb->s_blocksize - 1);
*blocknr = sbi->fat_start + (bytes >> sb->s_blocksize_bits);
}
@@ -34,7 +34,7 @@ static void fat_ent_blocknr(struct super
{
struct msdos_sb_info *sbi = MSDOS_SB(sb);
int bytes = (entry << sbi->fatent_shift);
- WARN_ON(entry < FAT_START_ENT || sbi->max_cluster <= entry);
+ WARN_ON(!fat_valid_entry(sbi, entry));
*offset = bytes & (sb->s_blocksize - 1);
*blocknr = sbi->fat_start + (bytes >> sb->s_blocksize_bits);
}
@@ -354,7 +354,7 @@ int fat_ent_read(struct inode *inode, st
int err, offset;
sector_t blocknr;

- if (entry < FAT_START_ENT || sbi->max_cluster <= entry) {
+ if (!fat_valid_entry(sbi, entry)) {
fatent_brelse(fatent);
fat_fs_error(sb, "invalid access to FAT (entry 0x%08x)", entry);
return -EIO;
_

--
OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx>