[PATCH 1/6] fs/minix: check return value of sb_getblk()

[PATCH 1/6] fs/minix: check return value of sb_getblk()

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

sb_getblk() can fail, so check its return value.

This fixes a NULL pointer dereference.

Reported-by: syzbot+4a88b2b9dc280f47baf4@syzkaller.appspotmail.com
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Originally-from: Qiujun Huang <anenbupt@gmail.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/minix/itree_common.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/minix/itree_common.c b/fs/minix/itree_common.c
index 043c3fdbc8e7..446148792f41 100644
--- a/fs/minix/itree_common.c
+++ b/fs/minix/itree_common.c
@@ -75,6 +75,7 @@ static int alloc_branch(struct inode *inode,
 	int n = 0;
 	int i;
 	int parent = minix_new_block(inode);
+	int err = -ENOSPC;
 
 	branch[0].key = cpu_to_block(parent);
 	if (parent) for (n = 1; n < num; n++) {
@@ -85,6 +86,11 @@ static int alloc_branch(struct inode *inode,
 			break;
 		branch[n].key = cpu_to_block(nr);
 		bh = sb_getblk(inode->i_sb, parent);
+		if (!bh) {
+			minix_free_block(inode, nr);
+			err = -ENOMEM;
+			break;
+		}
 		lock_buffer(bh);
 		memset(bh->b_data, 0, bh->b_size);
 		branch[n].bh = bh;
@@ -103,7 +109,7 @@ static int alloc_branch(struct inode *inode,
 		bforget(branch[i].bh);
 	for (i = 0; i < n; i++)
 		minix_free_block(inode, block_to_cpu(branch[i].key));
-	return -ENOSPC;
+	return err;
 }
 
 static inline int splice_branch(struct inode *inode,
-- 
2.27.0

[PATCH 2/6] fs/minix: don't allow getting deleted inodes

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

If an inode has no links, we need to mark it bad rather than allowing it
to be accessed.  This avoids WARNINGs in inc_nlink() and drop_nlink()
when doing directory operations on a fuzzed filesystem.

Reported-by: syzbot+a9ac3de1b5de5fb10efc@syzkaller.appspotmail.com
Reported-by: syzbot+df958cf5688a96ad3287@syzkaller.appspotmail.com
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/minix/inode.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/fs/minix/inode.c b/fs/minix/inode.c
index 7cb5fd38eb14..2bca95abe8f4 100644
--- a/fs/minix/inode.c
+++ b/fs/minix/inode.c
@@ -468,6 +468,13 @@ static struct inode *V1_minix_iget(struct inode *inode)
 		iget_failed(inode);
 		return ERR_PTR(-EIO);
 	}
+	if (raw_inode->i_nlinks == 0) {
+		printk("MINIX-fs: deleted inode referenced: %lu\n",
+		       inode->i_ino);
+		brelse(bh);
+		iget_failed(inode);
+		return ERR_PTR(-ESTALE);
+	}
 	inode->i_mode = raw_inode->i_mode;
 	i_uid_write(inode, raw_inode->i_uid);
 	i_gid_write(inode, raw_inode->i_gid);
@@ -501,6 +508,13 @@ static struct inode *V2_minix_iget(struct inode *inode)
 		iget_failed(inode);
 		return ERR_PTR(-EIO);
 	}
+	if (raw_inode->i_nlinks == 0) {
+		printk("MINIX-fs: deleted inode referenced: %lu\n",
+		       inode->i_ino);
+		brelse(bh);
+		iget_failed(inode);
+		return ERR_PTR(-ESTALE);
+	}
 	inode->i_mode = raw_inode->i_mode;
 	i_uid_write(inode, raw_inode->i_uid);
 	i_gid_write(inode, raw_inode->i_gid);
-- 
2.27.0

[PATCH 3/6] fs/minix: reject too-large maximum file size

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

If the minix filesystem tries to map a very large logical block number
to its on-disk location, block_to_path() can return offsets that are too
large, causing out-of-bounds memory accesses when accessing indirect
index blocks.  This should be prevented by the check against the maximum
file size, but this doesn't work because the maximum file size is read
directly from the on-disk superblock and isn't validated itself.

Fix this by validating the maximum file size at mount time.

Reported-by: syzbot+c7d9ec7a1a7272dd71b3@syzkaller.appspotmail.com
Reported-by: syzbot+3b7b03a0c28948054fb5@syzkaller.appspotmail.com
Reported-by: syzbot+6e056ee473568865f3e6@syzkaller.appspotmail.com
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/minix/inode.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/fs/minix/inode.c b/fs/minix/inode.c
index 2bca95abe8f4..0dd929346f3f 100644
--- a/fs/minix/inode.c
+++ b/fs/minix/inode.c
@@ -150,6 +150,23 @@ static int minix_remount (struct super_block * sb, int * flags, char * data)
 	return 0;
 }
 
+static bool minix_check_superblock(struct minix_sb_info *sbi)
+{
+	if (sbi->s_imap_blocks == 0 || sbi->s_zmap_blocks == 0)
+		return false;
+
+	/*
+	 * s_max_size must not exceed the block mapping limitation.  This check
+	 * is only needed for V1 filesystems, since V2/V3 support an extra level
+	 * of indirect blocks which places the limit well above U32_MAX.
+	 */
+	if (sbi->s_version == MINIX_V1 &&
+	    sbi->s_max_size > (7 + 512 + 512*512) * BLOCK_SIZE)
+		return false;
+
+	return true;
+}
+
 static int minix_fill_super(struct super_block *s, void *data, int silent)
 {
 	struct buffer_head *bh;
@@ -228,11 +245,12 @@ static int minix_fill_super(struct super_block *s, void *data, int silent)
 	} else
 		goto out_no_fs;
 
+	if (!minix_check_superblock(sbi))
+		goto out_illegal_sb;
+
 	/*
 	 * Allocate the buffer map to keep the superblock small.
 	 */
-	if (sbi->s_imap_blocks == 0 || sbi->s_zmap_blocks == 0)
-		goto out_illegal_sb;
 	i = (sbi->s_imap_blocks + sbi->s_zmap_blocks) * sizeof(bh);
 	map = kzalloc(i, GFP_KERNEL);
 	if (!map)
-- 
2.27.0

Re: [PATCH 1/6] fs/minix: check return value of sb_getblk()

[Andrew Morton]

On Sat, 27 Jun 2020 23:08:40 -0700 Eric Biggers <ebiggers@kernel.org> wrote:

> From: Eric Biggers <ebiggers@google.com>
> 
> sb_getblk() can fail, so check its return value.
> 
> This fixes a NULL pointer dereference.
> 
> Reported-by: syzbot+4a88b2b9dc280f47baf4@syzkaller.appspotmail.com
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Originally-from: Qiujun Huang <anenbupt@gmail.com>

Originally-from: isn't really a thing.  Did the original come with a
signed-off-by:?

> Signed-off-by: Eric Biggers <ebiggers@google.com>
>
> ...
>

Re: [PATCH 1/6] fs/minix: check return value of sb_getblk()

[Eric Biggers]

On Tue, Jul 07, 2020 at 12:26:12PM -0700, Andrew Morton wrote:
> On Sat, 27 Jun 2020 23:08:40 -0700 Eric Biggers <ebiggers@kernel.org> wrote:
> 
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > sb_getblk() can fail, so check its return value.
> > 
> > This fixes a NULL pointer dereference.
> > 
> > Reported-by: syzbot+4a88b2b9dc280f47baf4@syzkaller.appspotmail.com
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Cc: stable@vger.kernel.org
> > Originally-from: Qiujun Huang <anenbupt@gmail.com>
> 
> Originally-from: isn't really a thing.  Did the original come with a
> signed-off-by:?
> 

Yes it did.  Qiujun's patch was
https://lkml.kernel.org/lkml/20200323125700.7512-1-hqjagain@gmail.com
But I basically started from scratch anyway and my patch ended up different,
so I didn't leave the original "Author:".  Feel free to adjust the patch.

- Eric