[PATCH 000 of 2] knfsd: Don't allow bad file handles to cause extX to go readonly

[PATCH 000 of 2] knfsd: Don't allow bad file handles to cause extX to go readonly

[NeilBrown]

Currently, and file handle with a bad inode number in it can cause
ext2 to go to readonly (as it looks like a corrupted filesystem)
and could allow remote access to ext3 special files like the journal.

These patches give ext2/3 their own get_dentry method which checks the
inode number early before other bits of the code can be freaked out by
it.

These are revised versions of earlier patches.  Rather than exporting
export_iget, we open code it and simplify it slightly.  This avoids
and extra module dependancy.

NeilBrown

To follow:
 [PATCH 001 of 2] knfsd: Have ext2 reject file handles with bad inode numbers early.
 [PATCH 002 of 2] knfsd: Make ext3 reject filehandles referring to invalid inode numbers

[PATCH 001 of 2] knfsd: Have ext2 reject file handles with bad inode numbers early.

[NeilBrown]

This prevents bad inode numbers from triggering errors in
ext2_get_inode.


Signed-off-by: Neil Brown <neilb@suse.de>

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

diff .prev/fs/ext2/super.c ./fs/ext2/super.c
--- .prev/fs/ext2/super.c	2006-07-28 10:12:55.000000000 +1000
+++ ./fs/ext2/super.c	2006-07-28 10:17:58.000000000 +1000
@@ -251,6 +251,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.
@@ -258,6 +298,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)

[PATCH 002 of 2] knfsd: Make ext3 reject filehandles referring to invalid inode numbers

[NeilBrown]

Inodes earlier than the 'first' inode (e.g. journal,
resize) should be rejected early - except the root inode.
Also inode numbers that are too big should be rejected early.


Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./fs/ext3/super.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff .prev/fs/ext3/super.c ./fs/ext3/super.c
--- .prev/fs/ext3/super.c	2006-07-28 10:18:55.000000000 +1000
+++ ./fs/ext3/super.c	2006-07-28 10:25:20.000000000 +1000
@@ -554,6 +554,48 @@ static int ext3_show_options(struct seq_
 	return 0;
 }
 
+
+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 will return a bad_inode if the inode had been deleted.
+	 * so we should be safe.
+	 *
+	 * Currently we don't know the generation for parent directory, so
+	 * a generation of 0 means "accept any"
+	 */
+	inode = iget(sb, ino);
+	if (inode == NULL)
+		return ERR_PTR(-ENOMEM);
+	if (is_bad_inode(inode)
+	    || (generation && inode->i_generation != generation)
+		) {
+		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;
+}
+
 #ifdef CONFIG_QUOTA
 #define QTYPE2NAME(t) ((t)==USRQUOTA?"user":"group")
 #define QTYPE2MOPT(on, t) ((t)==USRQUOTA?((on)##USRJQUOTA):((on)##GRPJQUOTA))
@@ -622,6 +664,7 @@ static struct super_operations ext3_sops
 
 static struct export_operations ext3_export_ops = {
 	.get_parent = ext3_get_parent,
+	.get_dentry = ext3_get_dentry,
 };
 
 enum {

Re: [NFS] [PATCH 000 of 2] knfsd: Don't allow bad file handles to cause extX to go readonly

[Christoph Hellwig]

On Fri, Jul 28, 2006 at 10:31:20AM +1000, NeilBrown wrote:
> Currently, and file handle with a bad inode number in it can cause
> ext2 to go to readonly (as it looks like a corrupted filesystem)
> and could allow remote access to ext3 special files like the journal.
> 
> These patches give ext2/3 their own get_dentry method which checks the
> inode number early before other bits of the code can be freaked out by
> it.
> 
> These are revised versions of earlier patches.  Rather than exporting
> export_iget, we open code it and simplify it slightly.  This avoids
> and extra module dependancy.

This looks much better, agreed.  Long-term we should switch ext2/ext2
to use iget_locked so we can propagate errors in finding the inode much
better.