Re: [PATCH 4.19 033/110] bfs: add sanity check at bfs_fill_super()
From: Tigran Aivazian
Date: Thu Nov 29 2018 - 10:23:17 EST
Hello,
Yes, of course I object to it. I ignored this version of the patch
being applied to the older Linux versions, but for the latest versions
surely the version that I have authored should be applied instead. I
have sent to Andrew Morton both the 4.20-rc1 and 4.19.2 versions of
the patch. The 4.20 was applied (as "linux-next", I don't know why it
is not in 4.20-rc4 yet), but 4.19.2 version was not applied yet, so
here it is attached again (with the proper changelog etc). It applies
to 4.19.5 cleanly as well, so please use this version (attached).
Kind regards,
Tigran
On Thu, 29 Nov 2018 at 14:29, Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> 4.19-stable review patch. If anyone has any objections, please let me know.
>
> ------------------
>
> From: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
>
> commit 9f2df09a33aa2c76ce6385d382693f98d7f2f07e upstream.
>
> syzbot is reporting too large memory allocation at bfs_fill_super() [1].
> Since file system image is corrupted such that bfs_sb->s_start == 0,
> bfs_fill_super() is trying to allocate 8MB of continuous memory. Fix
> this by adding a sanity check on bfs_sb->s_start, __GFP_NOWARN and
> printf().
>
> [1] https://syzkaller.appspot.com/bug?id=16a87c236b951351374a84c8a32f40edbc034e96
>
> Link: http://lkml.kernel.org/r/1525862104-3407-1-git-send-email-penguin-kernel@xxxxxxxxxxxxxxxxxxx
> Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> Reported-by: syzbot <syzbot+71c6b5d68e91149fc8a4@xxxxxxxxxxxxxxxxxxxxxxxxx>
> Reviewed-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: Tigran Aivazian <aivazian.tigran@xxxxxxxxx>
> Cc: Matthew Wilcox <willy@xxxxxxxxxxxxx>
> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>
> ---
> fs/bfs/inode.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> --- a/fs/bfs/inode.c
> +++ b/fs/bfs/inode.c
> @@ -350,7 +350,8 @@ static int bfs_fill_super(struct super_b
>
> s->s_magic = BFS_MAGIC;
>
> - if (le32_to_cpu(bfs_sb->s_start) > le32_to_cpu(bfs_sb->s_end)) {
> + if (le32_to_cpu(bfs_sb->s_start) > le32_to_cpu(bfs_sb->s_end) ||
> + le32_to_cpu(bfs_sb->s_start) < BFS_BSIZE) {
> printf("Superblock is corrupted\n");
> goto out1;
> }
> @@ -359,9 +360,11 @@ static int bfs_fill_super(struct super_b
> sizeof(struct bfs_inode)
> + BFS_ROOT_INO - 1;
> imap_len = (info->si_lasti / 8) + 1;
> - info->si_imap = kzalloc(imap_len, GFP_KERNEL);
> - if (!info->si_imap)
> + info->si_imap = kzalloc(imap_len, GFP_KERNEL | __GFP_NOWARN);
> + if (!info->si_imap) {
> + printf("Cannot allocate %u bytes\n", imap_len);
> goto out1;
> + }
> for (i = 0; i < BFS_ROOT_INO; i++)
> set_bit(i, info->si_imap);
>
>
>
From: Tigran Aivazian <aivazian.tigran@xxxxxxxxx>
Subject: bfs: extra sanity checking and static inode bitmap
Strengthen validation of BFS superblock against corruption.
Make in-core inode bitmap static part of superblock info structure.
Print a warning when mounting a BFS filesystem created with "-N 512"
option as only 510 files can be created in the root directory.
Make the kernel messages more uniform. Update the 'prefix' passed to
bfs_dump_imap() to match the current naming of operations.
White space and comments cleanup.
Signed-off-by: Tigran Aivazian <aivazian.tigran@xxxxxxxxx>
Reported-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
---
fs/bfs/bfs.h | 11 ++++++-
fs/bfs/dir.c | 4 +-
fs/bfs/file.c | 2 -
fs/bfs/inode.c | 66 ++++++++++++++++++++------------------------
include/uapi/linux/bfs_fs.h | 2 -
5 files changed, 43 insertions(+), 42 deletions(-)
--- include/uapi/linux/bfs_fs.h.0 2018-11-13 19:19:55.941267342 +0000
+++ include/uapi/linux/bfs_fs.h 2018-11-13 19:20:24.101182357 +0000
@@ -1,7 +1,7 @@
/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
/*
* include/linux/bfs_fs.h - BFS data structures on disk.
- * Copyright (C) 1999 Tigran Aivazian <tigran@xxxxxxxxxxx>
+ * Copyright (C) 1999-2018 Tigran Aivazian <aivazian.tigran@xxxxxxxxx>
*/
#ifndef _LINUX_BFS_FS_H
--- fs/bfs/bfs.h.0 2018-11-13 19:20:40.151161044 +0000
+++ fs/bfs/bfs.h 2018-11-13 19:21:32.929740656 +0000
@@ -1,13 +1,20 @@
/* SPDX-License-Identifier: GPL-2.0 */
/*
* fs/bfs/bfs.h
- * Copyright (C) 1999 Tigran Aivazian <tigran@xxxxxxxxxxx>
+ * Copyright (C) 1999-2018 Tigran Aivazian <aivazian.tigran@xxxxxxxxx>
*/
#ifndef _FS_BFS_BFS_H
#define _FS_BFS_BFS_H
#include <linux/bfs_fs.h>
+/* In theory BFS supports up to 512 inodes, numbered from 2 (for /) up to 513 inclusive.
+ In actual fact, attempting to create the 512th inode (i.e. inode No. 513 or file No. 511)
+ will fail with ENOSPC in bfs_add_entry(): the root directory cannot contain so many entries, counting '..'.
+ So, mkfs.bfs(8) should really limit its -N option to 511 and not 512. For now, we just print a warning
+ if a filesystem is mounted with such "impossible to fill up" number of inodes */
+#define BFS_MAX_LASTI 513
+
/*
* BFS file system in-core superblock info
*/
@@ -17,7 +24,7 @@
unsigned long si_freei;
unsigned long si_lf_eblk;
unsigned long si_lasti;
- unsigned long *si_imap;
+ DECLARE_BITMAP(si_imap, BFS_MAX_LASTI+1);
struct mutex bfs_lock;
};
--- fs/bfs/dir.c.0 2018-11-13 19:29:32.361259272 +0000
+++ fs/bfs/dir.c 2018-11-13 19:30:01.380683858 +0000
@@ -2,8 +2,8 @@
/*
* fs/bfs/dir.c
* BFS directory operations.
- * Copyright (C) 1999,2000 Tigran Aivazian <tigran@xxxxxxxxxxx>
- * Made endianness-clean by Andrew Stribblehill <ads@xxxxxxxxxx> 2005
+ * Copyright (C) 1999-2018 Tigran Aivazian <aivazian.tigran@xxxxxxxxx>
+ * Made endianness-clean by Andrew Stribblehill <ads@xxxxxxxxxx> 2005
*/
#include <linux/time.h>
--- fs/bfs/file.c.0 2018-11-13 19:30:11.760489957 +0000
+++ fs/bfs/file.c 2018-11-13 19:30:27.020214845 +0000
@@ -2,7 +2,7 @@
/*
* fs/bfs/file.c
* BFS file operations.
- * Copyright (C) 1999,2000 Tigran Aivazian <tigran@xxxxxxxxxxx>
+ * Copyright (C) 1999-2018 Tigran Aivazian <aivazian.tigran@xxxxxxxxx>
*
* Make the file block allocation algorithm understand the size
* of the underlying block device.
--- fs/bfs/inode.c.0 2018-11-13 19:21:46.089579726 +0000
+++ fs/bfs/inode.c 2018-11-13 19:29:22.521467104 +0000
@@ -1,10 +1,9 @@
/*
* fs/bfs/inode.c
* BFS superblock and inode operations.
- * Copyright (C) 1999-2006 Tigran Aivazian <aivazian.tigran@xxxxxxxxx>
+ * Copyright (C) 1999-2018 Tigran Aivazian <aivazian.tigran@xxxxxxxxx>
* From fs/minix, Copyright (C) 1991, 1992 Linus Torvalds.
- *
- * Made endianness-clean by Andrew Stribblehill <ads@xxxxxxxxxx>, 2005.
+ * Made endianness-clean by Andrew Stribblehill <ads@xxxxxxxxxx>, 2005.
*/
#include <linux/module.h>
@@ -118,12 +117,12 @@
{
struct bfs_sb_info *info = BFS_SB(inode->i_sb);
unsigned int ino = (u16)inode->i_ino;
- unsigned long i_sblock;
+ unsigned long i_sblock;
struct bfs_inode *di;
struct buffer_head *bh;
int err = 0;
- dprintf("ino=%08x\n", ino);
+ dprintf("ino=%08x\n", ino);
di = find_inode(inode->i_sb, ino, &bh);
if (IS_ERR(di))
@@ -144,7 +143,7 @@
di->i_atime = cpu_to_le32(inode->i_atime.tv_sec);
di->i_mtime = cpu_to_le32(inode->i_mtime.tv_sec);
di->i_ctime = cpu_to_le32(inode->i_ctime.tv_sec);
- i_sblock = BFS_I(inode)->i_sblock;
+ i_sblock = BFS_I(inode)->i_sblock;
di->i_sblock = cpu_to_le32(i_sblock);
di->i_eblock = cpu_to_le32(BFS_I(inode)->i_eblock);
di->i_eoffset = cpu_to_le32(i_sblock * BFS_BSIZE + inode->i_size - 1);
@@ -188,13 +187,13 @@
mark_buffer_dirty(bh);
brelse(bh);
- if (bi->i_dsk_ino) {
+ if (bi->i_dsk_ino) {
if (bi->i_sblock)
info->si_freeb += bi->i_eblock + 1 - bi->i_sblock;
info->si_freei++;
clear_bit(ino, info->si_imap);
- bfs_dump_imap("delete_inode", s);
- }
+ bfs_dump_imap("evict_inode", s);
+ }
/*
* If this was the last file, make the previous block
@@ -214,7 +213,6 @@
return;
mutex_destroy(&info->bfs_lock);
- kfree(info->si_imap);
kfree(info);
s->s_fs_info = NULL;
}
@@ -311,8 +309,7 @@
else
strcat(tmpbuf, "0");
}
- printf("BFS-fs: %s: lasti=%08lx <%s>\n",
- prefix, BFS_SB(s)->si_lasti, tmpbuf);
+ printf("%s: lasti=%08lx <%s>\n", prefix, BFS_SB(s)->si_lasti, tmpbuf);
free_page((unsigned long)tmpbuf);
#endif
}
@@ -322,7 +319,7 @@
struct buffer_head *bh, *sbh;
struct bfs_super_block *bfs_sb;
struct inode *inode;
- unsigned i, imap_len;
+ unsigned i;
struct bfs_sb_info *info;
int ret = -EINVAL;
unsigned long i_sblock, i_eblock, i_eoff, s_size;
@@ -341,8 +338,7 @@
bfs_sb = (struct bfs_super_block *)sbh->b_data;
if (le32_to_cpu(bfs_sb->s_magic) != BFS_MAGIC) {
if (!silent)
- printf("No BFS filesystem on %s (magic=%08x)\n",
- s->s_id, le32_to_cpu(bfs_sb->s_magic));
+ printf("No BFS filesystem on %s (magic=%08x)\n", s->s_id, le32_to_cpu(bfs_sb->s_magic));
goto out1;
}
if (BFS_UNCLEAN(bfs_sb, s) && !silent)
@@ -350,18 +346,19 @@
s->s_magic = BFS_MAGIC;
- if (le32_to_cpu(bfs_sb->s_start) > le32_to_cpu(bfs_sb->s_end)) {
- printf("Superblock is corrupted\n");
+ if (le32_to_cpu(bfs_sb->s_start) > le32_to_cpu(bfs_sb->s_end) ||
+ le32_to_cpu(bfs_sb->s_start) < sizeof(struct bfs_super_block) + sizeof(struct bfs_dirent)) {
+ printf("Superblock is corrupted on %s\n", s->s_id);
goto out1;
}
- info->si_lasti = (le32_to_cpu(bfs_sb->s_start) - BFS_BSIZE) /
- sizeof(struct bfs_inode)
- + BFS_ROOT_INO - 1;
- imap_len = (info->si_lasti / 8) + 1;
- info->si_imap = kzalloc(imap_len, GFP_KERNEL);
- if (!info->si_imap)
+ info->si_lasti = (le32_to_cpu(bfs_sb->s_start) - BFS_BSIZE) / sizeof(struct bfs_inode) + BFS_ROOT_INO - 1;
+ if (info->si_lasti == BFS_MAX_LASTI)
+ printf("WARNING: filesystem %s was created with 512 inodes, the real maximum is 511, mounting anyway\n", s->s_id);
+ else if (info->si_lasti > BFS_MAX_LASTI) {
+ printf("Impossible last inode number %lu > %d on %s\n", info->si_lasti, BFS_MAX_LASTI, s->s_id);
goto out1;
+ }
for (i = 0; i < BFS_ROOT_INO; i++)
set_bit(i, info->si_imap);
@@ -369,26 +366,25 @@
inode = bfs_iget(s, BFS_ROOT_INO);
if (IS_ERR(inode)) {
ret = PTR_ERR(inode);
- goto out2;
+ goto out1;
}
s->s_root = d_make_root(inode);
if (!s->s_root) {
ret = -ENOMEM;
- goto out2;
+ goto out1;
}
info->si_blocks = (le32_to_cpu(bfs_sb->s_end) + 1) >> BFS_BSIZE_BITS;
- info->si_freeb = (le32_to_cpu(bfs_sb->s_end) + 1
- - le32_to_cpu(bfs_sb->s_start)) >> BFS_BSIZE_BITS;
+ info->si_freeb = (le32_to_cpu(bfs_sb->s_end) + 1 - le32_to_cpu(bfs_sb->s_start)) >> BFS_BSIZE_BITS;
info->si_freei = 0;
info->si_lf_eblk = 0;
/* can we read the last block? */
bh = sb_bread(s, info->si_blocks - 1);
if (!bh) {
- printf("Last block not available: %lu\n", info->si_blocks - 1);
+ printf("Last block not available on %s: %lu\n", s->s_id, info->si_blocks - 1);
ret = -EIO;
- goto out3;
+ goto out2;
}
brelse(bh);
@@ -422,11 +418,11 @@
(i_eoff != le32_to_cpu(-1) && i_eoff > s_size) ||
i_sblock * BFS_BSIZE > i_eoff) {
- printf("Inode 0x%08x corrupted\n", i);
+ printf("Inode 0x%08x corrupted on %s\n", i, s->s_id);
brelse(bh);
ret = -EIO;
- goto out3;
+ goto out2;
}
if (!di->i_ino) {
@@ -442,14 +438,12 @@
}
brelse(bh);
brelse(sbh);
- bfs_dump_imap("read_super", s);
+ bfs_dump_imap("fill_super", s);
return 0;
-out3:
+out2:
dput(s->s_root);
s->s_root = NULL;
-out2:
- kfree(info->si_imap);
out1:
brelse(sbh);
out:
@@ -479,7 +473,7 @@
int err = init_inodecache();
if (err)
goto out1;
- err = register_filesystem(&bfs_fs_type);
+ err = register_filesystem(&bfs_fs_type);
if (err)
goto out;
return 0;