Skip to contentSkip to conversation

e2fsprogs: Avoid infinite loop in ext2fs_find_block_device()

PATCH Applied Niu 4 participants 6 messages Nov 3, 2011 – Nov 20, 2011

lore
Share & tools
lore

Niu

author
#0 lore raw

hello, Ted

In my rhel5 system, there are lots of loop links in the /dev/.udev/failed folder, which makes the e2fsprogs 'make check' stuck in 't_ext_jnl_rm'. I'm not sure if the loop links are generated by udev defect or misconfiguration, but anyway, I think the ext2fs_find_block_device() should do some sanity check to avoid infinite loop.

I made a simple patch which breaks the loop when depth reaching 5 in ext2fs_find_block_device(), any comments? Thank you.

Quoted 1 line

From 5ef3b82266e7fb5edbce4febc4924b0beaccacf1 Mon Sep 17 00:00:00 2001

From: Niu Yawei <[email protected]>
Date: Wed, 2 Nov 2011 04:31:11 +0800
Subject: [PATCH] Avoid infinite loop in ext2fs_find_block_device()

The ext2fs_find_block_device() should stop searching when the directory depth reaching a certain threshold, otherwise, it could run into infinite loop if there are loop links in the device directory.

Signed-off-by Niu Yawei <[email protected]>
---
 lib/ext2fs/finddev.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
lib/ext2fs/finddev.c +4 -0
index 13ef14b..61f9754 100644
@@ -128,6 +128,7 @@ char *ext2fs_find_block_device(dev_t device)
 	struct dir_list *list = 0, *new_list = 0;
 	struct dir_list *current;
 	char	*ret_path = 0;
+	int    level = 0;
 
 	/*
 	 * Add the starting directories to search...

@@ -154,6 +155,9 @@ char *ext2fs_find_block_device(dev_t device)
 		if (list == 0) {
 			list = new_list;
 			new_list = 0;
+			/* Avoid infinite loop */
+			if (++level > 5)
+				break;
 		}
 	}
 	free_dirlist(&list);
Signature · 1 line
1.7.1
#1 lore raw
Quoted 7 lines · Niu

hello, Ted

In my rhel5 system, there are lots of loop links in the /dev/.udev/failed folder, which makes the e2fsprogs 'make check' stuck in 't_ext_jnl_rm'. I'm not sure if the loop links are generated by udev defect or misconfiguration, but anyway, I think the ext2fs_find_block_device() should do some sanity check to avoid infinite loop.

I made a simple patch which breaks the loop when depth reaching 5 in ext2fs_find_block_device(), any comments? Thank you.

My only concern would be that depth 5 isn't totally unreasonable in real life, and this causes it to silently stop searching, right? Would there be much harm in making the limit much higher, to be fairly sure that it has wandered off into the weeds?

I guess this function isn't very frequently called outside of make check though, just from tune2fs' remove_journal_device() (for external journals I guess?) so I suppose it's not too worrisome.

-Eric

Quoted 37 lines

From 5ef3b82266e7fb5edbce4febc4924b0beaccacf1 Mon Sep 17 00:00:00 2001
From: Niu Yawei <[email protected]>
Date: Wed, 2 Nov 2011 04:31:11 +0800
Subject: [PATCH] Avoid infinite loop in ext2fs_find_block_device()

The ext2fs_find_block_device() should stop searching when the directory depth reaching a certain threshold, otherwise, it could run into infinite loop if there are loop links in the device directory.

Signed-off-by Niu Yawei <[email protected]>
---
 lib/ext2fs/finddev.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
lib/ext2fs/finddev.c +4 -0
index 13ef14b..61f9754 100644
@@ -128,6 +128,7 @@ char *ext2fs_find_block_device(dev_t device)
 	struct dir_list *list = 0, *new_list = 0;
 	struct dir_list *current;
 	char	*ret_path = 0;
+	int    level = 0;
 
 	/*
 	 * Add the starting directories to search...

@@ -154,6 +155,9 @@ char *ext2fs_find_block_device(dev_t device)
 		if (list == 0) {
 			list = new_list;
 			new_list = 0;
+			/* Avoid infinite loop */
+			if (++level > 5)
+				break;
 		}
 	}
 	free_dirlist(&list);
#2 lore raw
Quoted 3 lines · Eric Sandeen

My only concern would be that depth 5 isn't totally unreasonable in real life, and this causes it to silently stop searching, right? Would there be much harm in making the limit much higher, to be fairly sure that it has wandered off into the weeds?

Agreed, the kernel currently uses a limit of 8. And we should use a #define for this in lib/ext2fs/ext2fsP.h, and use it for both finddev.c and lib/ext2fs/namei.c.

-- Ted

author
#3 lore raw
Quoted 1 line

From 81bfd58b3980f940c23f87f891365a289df776ec Mon Sep 17 00:00:00 2001

From: Niu Yawei <[email protected]>
Date: Wed, 2 Nov 2011 04:31:11 +0800
Subject: [PATCH] e2fsprogs: maximum nested link count

Define EXT2FS_MAX_NESTED_LINKS as 8, and check the link count not exceeding it in ext2fs_find_block_device() and follow_link().

Signed-off-by Niu Yawei <[email protected]>
---
 lib/ext2fs/ext2fsP.h |    2 ++
 lib/ext2fs/finddev.c |    5 +++++
 lib/ext2fs/namei.c   |    3 ++-
 3 files changed, 9 insertions(+), 1 deletions(-)
lib/ext2fs/ext2fsP.h +2 -0
index b182d7f..82e1ba0 100644
@@ -11,6 +11,8 @@
 
 #include "ext2fs.h"
 
+#define EXT2FS_MAX_NESTED_LINKS  8
+
 /*
  * Badblocks list
  */
lib/ext2fs/finddev.c +5 -0
index 13ef14b..311608d 100644
@@ -34,6 +34,7 @@
 
 #include "ext2_fs.h"
 #include "ext2fs.h"
+#include "ext2fsP.h"
 
 struct dir_list {
 	char	*name;

@@ -128,6 +129,7 @@ char *ext2fs_find_block_device(dev_t device)
 	struct dir_list *list = 0, *new_list = 0;
 	struct dir_list *current;
 	char	*ret_path = 0;
+	int    level = 0;
 
 	/*
 	 * Add the starting directories to search...

@@ -154,6 +156,9 @@ char *ext2fs_find_block_device(dev_t device)
 		if (list == 0) {
 			list = new_list;
 			new_list = 0;
+			/* Avoid infinite loop */
+			if (++level >= EXT2FS_MAX_NESTED_LINKS)
+				break;
 		}
 	}
 	free_dirlist(&list);
lib/ext2fs/namei.c +2 -1
index 6bbb124..a936474 100644
@@ -20,6 +20,7 @@
 
 #include "ext2_fs.h"
 #include "ext2fs.h"
+#include "ext2fsP.h"
 
 static errcode_t open_namei(ext2_filsys fs, ext2_ino_t root, ext2_ino_t base,
 			    const char *pathname, size_t pathlen, int follow,

@@ -45,7 +46,7 @@ static errcode_t follow_link(ext2_filsys fs, ext2_ino_t root, ext2_ino_t dir,
 		*res_inode = inode;
 		return 0;
 	}
-	if (link_count++ > 5) {
+	if (link_count++ >= EXT2FS_MAX_NESTED_LINKS) {
 		return EXT2_ET_SYMLINK_LOOP;
 	}
 	/* FIXME-64: Actually, this is FIXME EXTENTS */
Signature · 1 line
1.7.1
Quoted 8 lines · Eric Sandeen
Quoted 3 lines

My only concern would be that depth 5 isn't totally unreasonable in real life, and this causes it to silently stop searching, right? Would there be much harm in making the limit much higher, to be fairly sure that it has wandered off into the weeds?

Agreed, the kernel currently uses a limit of 8. And we should use a #define for this in lib/ext2fs/ext2fsP.h, and use it for both finddev.c and lib/ext2fs/namei.c.

-- Ted

Applied
#4 lore raw
Quoted 9 lines · Niu

From 81bfd58b3980f940c23f87f891365a289df776ec Mon Sep 17 00:00:00 2001
From: Niu Yawei <[email protected]>
Date: Wed, 2 Nov 2011 04:31:11 +0800
Subject: [PATCH] e2fsprogs: maximum nested link count

Define EXT2FS_MAX_NESTED_LINKS as 8, and check the link count not exceeding it in ext2fs_find_block_device() and follow_link().

Signed-off-by Niu Yawei <[email protected]>

Thanks, applied.

- Ted

#5 lore raw
In reply to Niu
Quoted 8 lines · Niu
lib/ext2fs/ext2fsP.h +0 -0
index b182d7f..82e1ba0 100644
@@ -11,6 +11,8 @@
 
 #include "ext2fs.h"

[...]

lib/ext2fs/finddev.c line 37

Quoted 7 lines
lib/ext2fs/finddev.c +1 -0
@@ -34,6 +34,7 @@
 
 #include "ext2_fs.h"
 #include "ext2fs.h"
+#include "ext2fsP.h"

On the very real chance of making a fool of myself: if ext2fsP.h includes ext2fs.h and now finddev.c will include ext2fsP.h - shouldn't we omit the '#include "ext2fs.h"' in finddev.c (and namei.c) now?

I guess the compiler/preprocessor will figure this out, just wondering about code cleanliness. Then again we have names like 'ext2_fs.h' and 'ext2fs.h', I guess only an insider will know the difference :-)

Christian.

Signature · 3 lines
BOFH excuse #97:

Small animal kamikaze attack on power supplies