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.
Best regards,
Anatoly
Anatoly Trosinenko <[email protected]> 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 <[email protected]>
---
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 <[email protected]>
On Sun, Jul 15, 2018 at 11:20:06PM +0900, OGAWA Hirofumi wrote:
> +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;
> +}
Pet peeve: if (...) return false; return true; instead of return !....;
In this case,
return entry >= FAT_START_ENT && entry < sb->max_cluster;
Al Viro <[email protected]> writes:
> On Sun, Jul 15, 2018 at 11:20:06PM +0900, OGAWA Hirofumi wrote:
>> +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;
>> +}
>
> Pet peeve: if (...) return false; return true; instead of return !....;
>
> In this case,
> return entry >= FAT_START_ENT && entry < sb->max_cluster;
Fixed. Thanks.
[PATCH v2] 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 <[email protected]>
---
fs/fat/cache.c | 19 ++++++++++++-------
fs/fat/fat.h | 5 +++++
fs/fat/fatent.c | 6 +++---
3 files changed, 20 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:59:11.978489716 +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:59:58.896437024 +0900
@@ -348,6 +348,11 @@ static inline void fatent_brelse(struct
fatent->fat_inode = NULL;
}
+static inline bool fat_valid_entry(struct msdos_sb_info *sbi, int entry)
+{
+ return FAT_START_ENT <= entry && entry < sbi->max_cluster;
+}
+
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:59:12.036489650 +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 <[email protected]>
Thank you!
> This patch returns better error (-EIO) for me.
This works for me likewise.
> (But note, the corrupted FS image doesn't guarantee POSIX behavior.)
Oops, I was just doing some testing and thought that correct behavior
for crafted FS is to return arbitrary valid error code (like -EIO) or
some arbitrary data, say, not larger than FS (not disclosing the
kernel memory, of course). Please excuse me if I was wrong. If fixing
this would slow down some hot code path, then I am not insisting on
returning valid errno. :)
Meanwhile, how should be considered such discrepancies with man pages
for invalid FS images: should it be considered low priority bug,
not-a-bug or feature request (diagnostics)?
Thanks
Anatoly
вс, 15 июл. 2018 г. в 17:30, Al Viro <[email protected]>:
>
> On Sun, Jul 15, 2018 at 11:20:06PM +0900, OGAWA Hirofumi wrote:
> > +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;
> > +}
>
> Pet peeve: if (...) return false; return true; instead of return !....;
>
> In this case,
> return entry >= FAT_START_ENT && entry < sb->max_cluster;
Anatoly Trosinenko <[email protected]> writes:
>> This patch returns better error (-EIO) for me.
>
> This works for me likewise.
Thanks for testing.
>> (But note, the corrupted FS image doesn't guarantee POSIX behavior.)
>
> Oops, I was just doing some testing and thought that correct behavior
> for crafted FS is to return arbitrary valid error code (like -EIO) or
> some arbitrary data, say, not larger than FS (not disclosing the
> kernel memory, of course). Please excuse me if I was wrong. If fixing
> this would slow down some hot code path, then I am not insisting on
> returning valid errno. :)
>
> Meanwhile, how should be considered such discrepancies with man pages
> for invalid FS images: should it be considered low priority bug,
> not-a-bug or feature request (diagnostics)?
To handle the corrupted image _perfectly_, finally we will have to have
online fsck or similar.
For example, if the data block was shared between the regular file and
directory, user can mmap the directory data via (corrupted) regular
file.
Then locking of directory handler doesn't work, and handler has to have
directory data as volatile data (and validate data for each memory
load). And to verify this invalid shared data blocks, we will have to
read all inodes (i.e. almost fsck).
So, I may change the code if data verification is easy and lightweight
like in this case. But like said above, it will not be guaranteed.
Thanks.
--
OGAWA Hirofumi <[email protected]>
Thanks for explanation!
Best regards,
Anatoly
вс, 15 июл. 2018 г. в 18:26, OGAWA Hirofumi <[email protected]>:
>
> Anatoly Trosinenko <[email protected]> writes:
>
> >> This patch returns better error (-EIO) for me.
> >
> > This works for me likewise.
>
> Thanks for testing.
>
> >> (But note, the corrupted FS image doesn't guarantee POSIX behavior.)
> >
> > Oops, I was just doing some testing and thought that correct behavior
> > for crafted FS is to return arbitrary valid error code (like -EIO) or
> > some arbitrary data, say, not larger than FS (not disclosing the
> > kernel memory, of course). Please excuse me if I was wrong. If fixing
> > this would slow down some hot code path, then I am not insisting on
> > returning valid errno. :)
> >
> > Meanwhile, how should be considered such discrepancies with man pages
> > for invalid FS images: should it be considered low priority bug,
> > not-a-bug or feature request (diagnostics)?
>
> To handle the corrupted image _perfectly_, finally we will have to have
> online fsck or similar.
>
> For example, if the data block was shared between the regular file and
> directory, user can mmap the directory data via (corrupted) regular
> file.
>
> Then locking of directory handler doesn't work, and handler has to have
> directory data as volatile data (and validate data for each memory
> load). And to verify this invalid shared data blocks, we will have to
> read all inodes (i.e. almost fsck).
>
> So, I may change the code if data verification is easy and lightweight
> like in this case. But like said above, it will not be guaranteed.
>
> Thanks.
> --
> OGAWA Hirofumi <[email protected]>
> Oops, I was just doing some testing and thought that correct behavior
> for crafted FS is to return arbitrary valid error code (like -EIO) or
> some arbitrary data, say, not larger than FS (not disclosing the
> kernel memory, of course). Please excuse me if I was wrong. If fixing
> this would slow down some hot code path, then I am not insisting on
> returning valid errno. :)
>
> Meanwhile, how should be considered such discrepancies with man pages
> for invalid FS images: should it be considered low priority bug,
> not-a-bug or feature request (diagnostics)?
If you can crash the machine or exploit it with a carefully crafted disk
then its serious. If you get weird behaviour only it's not too serious.
It's nice (but often not possible) if a filesystem at least forces itself
R/O when it detects a corruption to avoid doing more damage.
Alan