Re: Bad ext3/nfs DoS bug

[Neil Brown]

On Wednesday July 19, jack@xxxxxxx wrote:
> > So what happens next? Is the ext3 maintainer on sabatical,
> > or am I expected to submit a patch to fix this?
>   I guess people are mostly busy with OLS and such so maybe they missed
> the discussion.. Giving CC to relevant people to catch their attention
> :)
>   Andrew, Stephen: James has come across a nasty bug (potentially remote
> DoS). NFS extracts inode number from a filehandle and the inode number
> eventually ends in ext3_read_inode(). Now if the inode number is bogus,
> ext3_get_inode_block() calls ext3_error() and filesystem is remounted
> RO or whatever else is configured. That is quite undesirable in this
> case.
>   Now the easy "fix" is to change ext3_error() to ext3_warning() but an
> attacker flooding your logs with warnings is probably not good either
> and in case the inode number comes from ext3 itself we should really do
> ext3_error() as there is some corruption in the fs.
>   Better fix would be to add a flag to read_inode() saying that the inode
> number is from untrusted source (but that means changing a prototype of a
> function every fs uses) and change export_iget() to pass this flag. Yet
> another solution would be to make ext3 implement its own get_dentry() export
> function and pass the flag internally...
>   What do you think is the best solution?

I think that a good solution (hard to say if it is the best) is to
remove that error message altogether, and put it where inode numbers
are read out of directories.  Something like the following patch -
compile tested only.

NeilBrown

Avoid triggering ext3_error on bad NFS file handle

The inode number out of an NFS file handle gets passed 
eventually to ext3_get_inode_block without any checking.
If ext3_get_inode_block allows it to trigger a error,
then bad filehandles can have unpleasant effect.

So remove the call to ext3_error there and put a matching
check in ext3/namei.c where inode numbers are read of storage.


Signed-off-by: Neil Brown <neilb@xxxxxxx>

### Diffstat output
 ./fs/ext3/inode.c |   10 ++++++----
 ./fs/ext3/namei.c |   19 +++++++++++++++++--
 2 files changed, 23 insertions(+), 6 deletions(-)

diff .prev/fs/ext3/inode.c ./fs/ext3/inode.c
--- .prev/fs/ext3/inode.c	2006-07-20 14:41:07.000000000 +1000
+++ ./fs/ext3/inode.c	2006-07-20 14:42:04.000000000 +1000
@@ -2405,11 +2405,13 @@ static ext3_fsblk_t ext3_get_inode_block
 
 	if ((ino != EXT3_ROOT_INO && ino != EXT3_JOURNAL_INO &&
 		ino != EXT3_RESIZE_INO && ino < EXT3_FIRST_INO(sb)) ||
-		ino > le32_to_cpu(EXT3_SB(sb)->s_es->s_inodes_count)) {
-		ext3_error(sb, "ext3_get_inode_block",
-			    "bad inode number: %lu", ino);
+		ino > le32_to_cpu(EXT3_SB(sb)->s_es->s_inodes_count))
+		/* This error already checked for in namei.c unless we
+		 * are looking at an NFS filehandle, in which case,
+		 * no error reported is needed
+		 */
 		return 0;
-	}
+
 	block_group = (ino - 1) / EXT3_INODES_PER_GROUP(sb);
 	if (block_group >= EXT3_SB(sb)->s_groups_count) {
 		ext3_error(sb,"ext3_get_inode_block","group >= groups count");

diff .prev/fs/ext3/namei.c ./fs/ext3/namei.c
--- .prev/fs/ext3/namei.c	2006-07-20 14:39:51.000000000 +1000
+++ ./fs/ext3/namei.c	2006-07-20 14:44:23.000000000 +1000
@@ -1000,7 +1000,14 @@ static struct dentry *ext3_lookup(struct
 	if (bh) {
 		unsigned long ino = le32_to_cpu(de->inode);
 		brelse (bh);
-		inode = iget(dir->i_sb, ino);
+		if ((ino != EXT3_ROOT_INO && ino != EXT3_JOURNAL_INO &&
+		     ino != EXT3_RESIZE_INO && ino < EXT3_FIRST_INO(dir->i_sb)) ||
+		    ino > le32_to_cpu(EXT3_SB(dir->i_sb)->s_es->s_inodes_count)) {
+			ext3_error(dir->i_sb, "ext3_lookup",
+				   "bad inode number: %lu", ino);
+			inode = NULL;
+		} else
+			inode = iget(dir->i_sb, ino);
 
 		if (!inode)
 			return ERR_PTR(-EACCES);
@@ -1028,7 +1035,15 @@ struct dentry *ext3_get_parent(struct de
 		return ERR_PTR(-ENOENT);
 	ino = le32_to_cpu(de->inode);
 	brelse(bh);
-	inode = iget(child->d_inode->i_sb, ino);
+
+	if ((ino != EXT3_ROOT_INO && ino != EXT3_JOURNAL_INO &&
+	     ino != EXT3_RESIZE_INO && ino < EXT3_FIRST_INO(child->d_inode->i_sb)) ||
+	    ino > le32_to_cpu(EXT3_SB(child->d_inode->i_sb)->s_es->s_inodes_count)) {
+		ext3_error(child->d_inode->i_sb, "ext3_get_parent",
+			   "bad inode number: %lu", ino);
+		inode = NULL;
+	} else
+		inode = iget(child->d_inode->i_sb, ino);
 
 	if (!inode)
 		return ERR_PTR(-EACCES);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/