Skip to contentSkip to conversation

knfsd: Don't allow bad file handles to cause extX to go readonly

PATCH In review NeilBrown 2 participants 4 messages Jul 28, 2006

Subsystems (3)
lore
Share & tools
lore

NeilBrown

author
#0 lore raw

PATCH 1/1 knfsd: Don't allow bad file handles to cause extX to go readonly

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

author
#1 lore raw

PATCH 1/1 knfsd: Have ext2 reject file handles with bad inode numbers early.

This prevents bad inode numbers from triggering errors in ext2_get_inode.

### Diffstat output

 ./fs/ext2/super.c |   41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)
fs/ext2/super.c +41 -0
diff .prev/fs/ext2/super.c ./fs/ext2/super.c
@@ -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)
author
#2 lore raw

PATCH 1/1 knfsd: Make ext3 reject filehandles referring to invalid inode numbers

In reply to 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.

### Diffstat output

 ./fs/ext3/super.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)
fs/ext3/super.c +43 -0
diff .prev/fs/ext3/super.c ./fs/ext3/super.c
@@ -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 {

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

#3 lore raw
In reply to NeilBrown
Quoted 11 lines · 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.

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.