CVE-2006-3468: which patch to use?

CVE-2006-3468: which patch to use?

[Adrian Bunk]

[-- Attachment #1: Type: text/plain, Size: 1343 bytes --]

While going through patches for 2.6.16.x, I stumbled over the following 
regarding the "NFS export of ext2/ext3" security vulnerabilities (the 
ext3 one is  CVE-2006-3468, I don't whether there's a number for the 
ext2 one):

There are three patches available:
have-ext2-reject-file-handles-with-bad-inode-numbers-early.patch
have-ext3-reject-file-handles-with-bad-inode-numbers-early.patch
ext3-avoid-triggering-ext3_error-on-bad-nfs-file-handle.patch

The first two patches are except for a s/ext2/ext3/ identical.

The two ext3 patches fix the same issue in slightly different ways.

It seems there was already some agreement that the first of the two ext3 
patches should be preferred due to being more the same as the ext2 patch
(see [1] and followups).

But the only patch that is applied in 2.6.18-rc4 (and in 2.6.17.9) is 
the ext3 patch that is _not_ identical to the ext2 one.

Is it the correct solution to revert this ext3 patch in both 2.6.18-rc 
and 2.6.17 and to apply the other two patches?

cu
Adrian

BTW: I've attached all three patches.

[1] http://lkml.org/lkml/2006/8/4/192

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed


[-- Attachment #2: have-ext2-reject-file-handles-with-bad-inode-numbers-early.patch --]
[-- Type: text/plain, Size: 2320 bytes --]

>From neilb@suse.de Thu Aug  3 17:53:52 2006
From: Neil Brown <neilb@suse.de>
To: Greg KH <greg@kroah.com>
Date: Fri, 4 Aug 2006 10:53:40 +1000
Message-ID: <17618.39572.764990.76181@cse.unsw.edu.au>
Cc: Marcel Holtmann <marcel@holtmann.org>,
	Linus Torvalds <torvalds@osdl.org>, linux-kernel@vger.kernel.org,
	stable@kernel.org
Subject: Have ext2 reject file handles with bad inode numbers early.

From: Neil Brown <neilb@suse.de>

This prevents bad inode numbers from triggering errors in
ext2_get_inode.


Signed-off-by: Neil Brown <neilb@suse.de>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 fs/ext2/super.c |   41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

--- linux-2.6.17.7.orig/fs/ext2/super.c
+++ linux-2.6.17.7/fs/ext2/super.c
@@ -252,6 +252,46 @@ static struct super_operations ext2_sops
 #endif
 };
 
+static struct dentry *ext2_get_dentry(struct super_block *sb, void *vobjp)
+{
+	__u32 *objp = vobjp;
+	unsigned long ino = objp[0];
+	__u32 generation = objp[1];
+	struct inode *inode;
+	struct dentry *result;
+
+	if (ino != EXT2_ROOT_INO && ino < EXT2_FIRST_INO(sb))
+		return ERR_PTR(-ESTALE);
+	if (ino > le32_to_cpu(EXT2_SB(sb)->s_es->s_inodes_count))
+		return ERR_PTR(-ESTALE);
+
+	/* iget isn't really right if the inode is currently unallocated!!
+	 * ext2_read_inode currently does appropriate checks, but
+	 * it might be "neater" to call ext2_get_inode first and check
+	 * if the inode is valid.....
+	 */
+	inode = iget(sb, ino);
+	if (inode == NULL)
+		return ERR_PTR(-ENOMEM);
+	if (is_bad_inode(inode)
+	    || (generation && inode->i_generation != generation)
+		) {
+		/* we didn't find the right inode.. */
+		iput(inode);
+		return ERR_PTR(-ESTALE);
+	}
+	/* now to find a dentry.
+	 * If possible, get a well-connected one
+	 */
+	result = d_alloc_anon(inode);
+	if (!result) {
+		iput(inode);
+		return ERR_PTR(-ENOMEM);
+	}
+	return result;
+}
+
+
 /* Yes, most of these are left as NULL!!
  * A NULL value implies the default, which works with ext2-like file
  * systems, but can be improved upon.
@@ -259,6 +299,7 @@ static struct super_operations ext2_sops
  */
 static struct export_operations ext2_export_ops = {
 	.get_parent = ext2_get_parent,
+	.get_dentry = ext2_get_dentry,
 };
 
 static unsigned long get_sb_block(void **data)

[-- Attachment #3: have-ext3-reject-file-handles-with-bad-inode-numbers-early.patch --]
[-- Type: text/plain, Size: 2570 bytes --]

>From stable-bounces@linux.kernel.org Fri Aug  4 08:36:24 2006
Message-ID: <44D36946.7020601@redhat.com>
Date: Fri, 04 Aug 2006 10:35:34 -0500
From: Eric Sandeen <esandeen@redhat.com>
To: Christoph Hellwig <hch@infradead.org>, Eric Sandeen <esandeen@redhat.com>,
        Greg KH <gregkh@suse.de>, linux-kernel@vger.kernel.org,
        stable@kernel.org, torvalds@osdl.org,
        Justin Forbes <jmforbes@linuxtx.org>,
        Zwane Mwaikambo <zwane@arm.linux.org.uk>,
        "Theodore Ts'o" <tytso@mit.edu>, Randy Dunlap <rdunlap@xenotime.net>,
        Dave Jones <davej@redhat.com>, Chuck Wolber <chuckw@quantumlinux.com>,
        Chris Wedgwood <reviews@ml.cw.f00f.org>, akpm@osdl.org,
        alan@lxorguk.ukuu.org.uk, jack@suse.cz, neilb@suse.de,
        Marcel Holtmann <marcel@holtmann.org>,
        "Stephen C. Tweedie" <sct@redhat.com>
Subject: Have ext3 reject file handles with bad inode numbers early

blatantly ripped off from Neil Brown's ext2 patch.


Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
Acked-by: "Theodore Ts'o" <tytso@mit.edu>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 fs/ext3/super.c |   40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

--- linux-2.6.17.8.orig/fs/ext3/super.c
+++ linux-2.6.17.8/fs/ext3/super.c
@@ -620,8 +620,48 @@ static struct super_operations ext3_sops
 #endif
 };
 
+static struct dentry *ext3_get_dentry(struct super_block *sb, void *vobjp)
+{
+	__u32 *objp = vobjp;
+	unsigned long ino = objp[0];
+	__u32 generation = objp[1];
+	struct inode *inode;
+	struct dentry *result;
+
+	if (ino != EXT3_ROOT_INO && ino < EXT3_FIRST_INO(sb))
+		return ERR_PTR(-ESTALE);
+	if (ino > le32_to_cpu(EXT3_SB(sb)->s_es->s_inodes_count))
+		return ERR_PTR(-ESTALE);
+
+	/* iget isn't really right if the inode is currently unallocated!!
+	 * ext3_read_inode currently does appropriate checks, but
+	 * it might be "neater" to call ext3_get_inode first and check
+	 * if the inode is valid.....
+	 */
+	inode = iget(sb, ino);
+	if (inode == NULL)
+		return ERR_PTR(-ENOMEM);
+	if (is_bad_inode(inode)
+	    || (generation && inode->i_generation != generation)
+		) {
+		/* we didn't find the right inode.. */
+		iput(inode);
+		return ERR_PTR(-ESTALE);
+	}
+	/* now to find a dentry.
+	 * If possible, get a well-connected one
+	 */
+	result = d_alloc_anon(inode);
+	if (!result) {
+		iput(inode);
+		return ERR_PTR(-ENOMEM);
+	}
+	return result;
+}
+
 static struct export_operations ext3_export_ops = {
 	.get_parent = ext3_get_parent,
+	.get_dentry = ext3_get_dentry,
 };
 
 enum {

[-- Attachment #4: ext3-avoid-triggering-ext3_error-on-bad-nfs-file-handle.patch --]
[-- Type: text/plain, Size: 3836 bytes --]

>From stable-bounces@linux.kernel.org Sun Jul 30 03:04:26 2006
Message-Id: <200607301003.k6UA31hC002485@shell0.pdx.osdl.net>
To: torvalds@osdl.org
From: akpm@osdl.org
Date: Sun, 30 Jul 2006 03:03:01 -0700
Cc: akpm@osdl.org, jack@suse.cz, esandeen@redhat.com, neilb@suse.de,
        sct@redhat.com, marcel@holtmann.org, stable@kernel.org
Subject: ext3: avoid triggering ext3_error on bad NFS file handle

From: Neil Brown <neilb@suse.de>

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 an error, then bad filehandles can have unpleasant
effect - ext3_error() will usually cause a forced read-only remount, or a
panic if `errors=panic' was used.

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

[akpm@osdl.org: fix off-by-one error]
Signed-off-by: Neil Brown <neilb@suse.de>
Signed-off-by: Jan Kara <jack@suse.cz>
Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: "Stephen C. Tweedie" <sct@redhat.com>
Cc: Eric Sandeen <esandeen@redhat.com>
Signed-off-by: Andrew Morton <akpm@osdl.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 fs/ext3/inode.c         |   13 +++++++------
 fs/ext3/namei.c         |   15 +++++++++++++--
 include/linux/ext3_fs.h |    9 +++++++++
 3 files changed, 29 insertions(+), 8 deletions(-)

--- linux-2.6.17.7.orig/fs/ext3/inode.c
+++ linux-2.6.17.7/fs/ext3/inode.c
@@ -2402,14 +2402,15 @@ static unsigned long ext3_get_inode_bloc
 	struct buffer_head *bh;
 	struct ext3_group_desc * gdp;
 
-
-	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);
+	if (!ext3_valid_inum(sb, ino)) {
+		/*
+		 * This error is already checked for in namei.c unless we are
+		 * looking at an NFS filehandle, in which case no error
+		 * report 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");
--- linux-2.6.17.7.orig/fs/ext3/namei.c
+++ linux-2.6.17.7/fs/ext3/namei.c
@@ -1000,7 +1000,12 @@ 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 (!ext3_valid_inum(dir->i_sb, ino)) {
+			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 +1033,13 @@ 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 (!ext3_valid_inum(child->d_inode->i_sb, ino)) {
+		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);
--- linux-2.6.17.7.orig/include/linux/ext3_fs.h
+++ linux-2.6.17.7/include/linux/ext3_fs.h
@@ -495,6 +495,15 @@ static inline struct ext3_inode_info *EX
 {
 	return container_of(inode, struct ext3_inode_info, vfs_inode);
 }
+
+static inline int ext3_valid_inum(struct super_block *sb, unsigned long ino)
+{
+	return 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));
+}
 #else
 /* Assume that user mode programs are passing in an ext3fs superblock, not
  * a kernel struct super_block.  This will allow us to call the feature-test

Re: CVE-2006-3468: which patch to use?

[Eric Sandeen]

Adrian Bunk wrote:
> While going through patches for 2.6.16.x, I stumbled over the following 
> regarding the "NFS export of ext2/ext3" security vulnerabilities (the 
> ext3 one is  CVE-2006-3468, I don't whether there's a number for the 
> ext2 one):
> 
> There are three patches available:
> have-ext2-reject-file-handles-with-bad-inode-numbers-early.patch
> have-ext3-reject-file-handles-with-bad-inode-numbers-early.patch
> ext3-avoid-triggering-ext3_error-on-bad-nfs-file-handle.patch
> 
> The first two patches are except for a s/ext2/ext3/ identical.
> 
> The two ext3 patches fix the same issue in slightly different ways.
> 
> It seems there was already some agreement that the first of the two ext3 
> patches should be preferred due to being more the same as the ext2 patch
> (see [1] and followups).
> 
> But the only patch that is applied in 2.6.18-rc4 (and in 2.6.17.9) is 
> the ext3 patch that is _not_ identical to the ext2 one.
> 
> Is it the correct solution to revert this ext3 patch in both 2.6.18-rc 
> and 2.6.17 and to apply the other two patches?
> 
> cu
> Adrian
> 
> BTW: I've attached all three patches.
> 
> [1] http://lkml.org/lkml/2006/8/4/192

IMO the first two should be used; i.e. those that add ext[23]_get_dentry().

-Eric

Re: CVE-2006-3468: which patch to use?

[Neil Brown]

On Sunday August 20, bunk@stusta.de wrote:
> While going through patches for 2.6.16.x, I stumbled over the following 
> regarding the "NFS export of ext2/ext3" security vulnerabilities (the 
> ext3 one is  CVE-2006-3468, I don't whether there's a number for the 
> ext2 one):
> 
> There are three patches available:
> have-ext2-reject-file-handles-with-bad-inode-numbers-early.patch
> have-ext3-reject-file-handles-with-bad-inode-numbers-early.patch
> ext3-avoid-triggering-ext3_error-on-bad-nfs-file-handle.patch
> 
> The first two patches are except for a s/ext2/ext3/ identical.
> 
> The two ext3 patches fix the same issue in slightly different ways.
> 
> It seems there was already some agreement that the first of the two ext3 
> patches should be preferred due to being more the same as the ext2 patch
> (see [1] and followups).
> 
> But the only patch that is applied in 2.6.18-rc4 (and in 2.6.17.9) is 
> the ext3 patch that is _not_ identical to the ext2 one.
> 
> Is it the correct solution to revert this ext3 patch in both 2.6.18-rc 
> and 2.6.17 and to apply the other two patches?

There is no point in reverting the ext3 patch.  It is a good and
proper patch to have.
Apply the ext2 patch is definitely a good idea.
Applying the other ext3 patch is also a good idea.

NeilBrown