Skip to contentSkip to conversation

Modprobe local root exploit

DISCUSSION Gregory Maxwell 18 participants 27 messages Nov 13, 2000 – Nov 16, 2000

lore
Share & tools
lore

Gregory Maxwell

author
#0 lore raw

After seeing the modprobe local root exploit today, I asked myself why kmod executes modprobe with full root and doesn't drop some capabilities first.

Why? It wouldn't close the hole, but it would narrow it down. -
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

#1 lore raw
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Gregory" == Gregory Maxwell <[email protected]> writes:

Gregory> After seeing the modprobe local root exploit today, I asked Gregory> myself why kmod executes modprobe with full root and doesn't Gregory> drop some capabilities first.

Gregory> Why? It wouldn't close the hole, but it would narrow it down.

This might also be a good idea; but my suggestion is to not allow arbitrary strings as module names in the first place. As far as I can see, all valid strings for KMOD requests consist of alphanumeric chars plus dash and underscore. Anybody with autoloaded modules that don't fit this pattern even after /etc/modules.conf translation please object !

Here's the patch...

Torsten

kernel/kmod.c renamed from kernel/kmod.c.orig +16 -0
@@ -168,6 +168,22 @@
 	static atomic_t kmod_concurrent = ATOMIC_INIT(0);
 #define MAX_KMOD_CONCURRENT 50	/* Completely arbitrary value - KAO */
 	static int kmod_loop_msg;
+	const char * p;
+
+	/* For security reasons ensure the requested name consists
+	 * only of allowed characters. Especially whitespace and
+	 * shell metacharacters might confuse modprobe.
+	 */
+	for (p = module_name; *p; p++)
+	{
+	  if ((*p & 0xdf) >= 'a' && (*p & 0xdf) <= 'z')
+	    continue;
+	  if (*p >= '0' && *p <= '9')
+	    continue;
+	  if (*p == '_' || *p == '-')
+	    continue;
+	  return -EINVAL;
+	}
 
 	/* Don't allow request_module() before the root fs is mounted!  */
 	if ( ! current->fs->root ) {

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

#2 lore raw
Quoted 14 lines · Torsten Duwe
kernel/kmod.c renamed from kernel/kmod.c.orig +7 -0
@@ -168,6 +168,22 @@
 	static atomic_t kmod_concurrent = ATOMIC_INIT(0);
 #define MAX_KMOD_CONCURRENT 50	/* Completely arbitrary value - KAO */
 	static int kmod_loop_msg;
+	const char * p;
+
+	/* For security reasons ensure the requested name consists
+	 * only of allowed characters. Especially whitespace and
+	 * shell metacharacters might confuse modprobe.
+	 */
+	for (p = module_name; *p; p++)

kernel/kmod.c line 187

Quoted 10 lines
kernel/kmod.c renamed from kernel/kmod.c.orig +9 -0
+	{
+	  if ((*p & 0xdf) >= 'a' && (*p & 0xdf) <= 'z')
+	    continue;
+	  if (*p >= '0' && *p <= '9')
+	    continue;
+	  if (*p == '_' || *p == '-')
+	    continue;
+	  return -EINVAL;
+	}

Just in case... Some modules have uppercase letters too :)

Signature · 9 lines
Francis Galiegue, [email protected]
"Programming is a race between programmers, who try and make more and more
idiot-proof software, and universe, which produces more and more remarkable
idiots. Until now, universe leads the race"  -- R. Cook

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/
#3 lore raw
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Francis" == Francis Galiegue <[email protected]> writes:

+ if ((*p & 0xdf) >= 'a' && (*p & 0xdf) <= 'z') continue;

Francis> Just in case... Some modules have uppercase letters too :)

That's what the &0xdf is intended for...

Torsten
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

#4 lore raw
Quoted 7 lines · Torsten Duwe
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Francis" == Francis Galiegue <[email protected]> writes:

+ if ((*p & 0xdf) >= 'a' && (*p & 0xdf) <= 'z') continue;

Francis> Just in case... Some modules have uppercase letters too :)

That's what the &0xdf is intended for...

Code in a security sensitive area needs to be crystal clear.

What's wrong with isalnum() ?

Cheers
Chris

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

#5 lore raw
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Chris" == Chris Evans <[email protected]> writes:

Chris> What's wrong with isalnum() ?

Hm, must admit that I wasn't 100% sure if that's in the kernel lib or an evil gcc expands it inline to some static array lookup. Now I see that it's already in the kernel. Do some of you also sometimes wonder why the kernel is so big ;-) ?

OK, so let's go for the isalnum() version, and don't forget to #include <linux/ctype.h>
above...

Torsten

For those who joined the program late here's the complete patch:

kernel/kmod.c renamed from kernel/kmod.c.orig +14 -0
@@ -18,6 +18,7 @@
 #include <linux/config.h>
 #include <linux/sched.h>
 #include <linux/unistd.h>
+#include <linux/ctype.h>
 #include <linux/smp_lock.h>
 
 #include <asm/uaccess.h>

@@ -168,6 +169,19 @@
 	static atomic_t kmod_concurrent = ATOMIC_INIT(0);
 #define MAX_KMOD_CONCURRENT 50	/* Completely arbitrary value - KAO */
 	static int kmod_loop_msg;
+	const char * p;
+
+	/* For security reasons ensure the requested name consists
+	 * only of allowed characters. Especially whitespace and
+	 * shell metacharacters might confuse modprobe.
+	 */
+	for (p = module_name; *p; p++)
+	{
+	  if (isalnum(*p) || *p == '_' || *p == '-')
+	    continue;
+
+	  return -EINVAL;
+	}
 
 	/* Don't allow request_module() before the root fs is mounted!  */
 	if ( ! current->fs->root ) {

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

#6 lore raw
In reply to Chris Evans
Quoted 10 lines · Chris Evans
Quoted 7 lines · Torsten Duwe
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Francis" == Francis Galiegue <[email protected]> writes:

+ if ((*p & 0xdf) >= 'a' && (*p & 0xdf) <= 'z') continue;

Francis> Just in case... Some modules have uppercase letters too :)

That's what the &0xdf is intended for...

Code in a security sensitive area needs to be crystal clear.

Nodz!

Quoted 1 line

What's wrong with isalnum() ?

Too efficient? ;)
--
Horst von Brand [email protected]
Casilla 9G, Vin~a del Mar, Chile +56 32 672616
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

#7 lore raw
In reply to Torsten Duwe
Quoted 7 lines · Chris Evans

On Mon, 13 Nov 2000, Torsten Duwe wrote:

Code in a security sensitive area needs to be crystal clear.

What's wrong with isalnum() ?

What about this then ?

kmod.c renamed from kmod.c.orig +14 -0
@@ -169,6 +169,20 @@
 #define MAX_KMOD_CONCURRENT 50	/* Completely arbitrary value - KAO */
 	static int kmod_loop_msg;
 
+	const char * p;
+
+	/* For security reasons ensure the requested name consists
+	* only of allowed characters. Especially whitespace and
+	* shell metacharacters might confuse modprobe.
+	*/
+	for (p = module_name; *p; p++)
+	{
+		if (isalnum(*p) || *p=='_' || *p=='-')
+			continue;
+
+		return -EINVAL;
+	}
+
 	/* Don't allow request_module() before the root fs is mounted!  */
 	if ( ! current->fs->root ) {
 		printk(KERN_ERR "request_module[%s]: Root fs not mounted\n",
#8 lore raw
In reply to Torsten Duwe

[Torsten Duwe]

kernel/kmod.c line 186

Quoted 7 lines
kernel/kmod.c renamed from kernel/kmod.c.orig +7 -0
+	for (p = module_name; *p; p++)
+	{
+	  if (isalnum(*p) || *p == '_' || *p == '-')
+	    continue;
+
+	  return -EINVAL;
+	}

I think you just broke at least some versions of devfs. I don't remember if the feature is still around, but I know it *used* to be possible to request_module("/dev/foobar"), which requires '/' in the name.

Peter
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

#9 lore raw
Quoted 14 lines · Peter Samuelson

[Torsten Duwe]

kernel/kmod.c line 186

Quoted 7 lines
kernel/kmod.c renamed from kernel/kmod.c.orig +7 -0
+	for (p = module_name; *p; p++)
+	{
+	  if (isalnum(*p) || *p == '_' || *p == '-')
+	    continue;
+
+	  return -EINVAL;
+	}

I think you just broke at least some versions of devfs. I don't remember if the feature is still around, but I know it *used* to be possible to request_module("/dev/foobar"), which requires '/' in the name.

That feature of devfs is definitely still around, it is critical to devfs.

All these patches against request_module are attacking the problem at the wrong point. The kernel can request any module name it likes, using any string it likes, as long as the kernel generates the name. The real problem is when the kernel blindly accepts some user input and passes it straight to modprobe, then the kernel is acting like a setuid wrapper for a program that was never designed to run setuid.

At the very least, interface names which are taken directly from the user should be prefixed with "user-interface-" before being passed to modprobe. The sysadmin can set "alias user-interface-eth0 eth0" if they want to use this feature. Passing the interface name unchanged is asking for trouble, as Chris Evans pointed out, you can abuse this "feature" to load any module.

I have nearly finished the security review of modutils, 2.3.20 will be out later tonight. It treats the kmod environment as tainted, forces the last parameter to be treated as a module name even if it starts with '-', only accepts one module name and refuses 'variable=value' strings. I find it rather ironic to be treating kernel supplied data as tainted.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

#10 lore raw
Quoted 2 lines · Keith Owens

All these patches against request_module are attacking the problem at the wrong point.

Agreed.

Quoted 5 lines

The kernel can request any module name it likes, using any string it likes, as long as the kernel generates the name. The real problem is when the kernel blindly accepts some user input and passes it straight to modprobe, then the kernel is acting like a setuid wrapper for a program that was never designed to run setuid.

I don't think it's a good idea to distribute such stuff over the whole kernel. Better control it at a single place, either when passing the parameter down to modprobe, or in modprobe itself. Everything else is too error-prone.

Signature · 7 lines
Florian Weimer 	                  [email protected]
University of Stuttgart           http://cert.uni-stuttgart.de/
RUS-CERT                          +49-711-685-5973/fax +49-711-685-5898
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/
#11 lore raw
In reply to Peter Samuelson
Quoted 6 lines · Keith Owens

All these patches against request_module are attacking the problem at the wrong point. The kernel can request any module name it likes, using any string it likes, as long as the kernel generates the name. The real problem is when the kernel blindly accepts some user input and passes it straight to modprobe, then the kernel is acting like a setuid wrapper for a program that was never designed to run setuid.

Rather than add sanity checking to modprobe, it would be a lot easier and safer from a security audit point of view to have the kernel call /sbin/kmodprobe instead of /sbin/modprobe. Then kmodprobe can sanitise all the data and exec the real modprobe. That way the only thing that needs auditing is a string munging/sanitising program.

--Malcolm

Signature · 7 lines
Malcolm Beattie <[email protected]>
Unix Systems Programmer
Oxford University Computing Services
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/
#12 lore raw
Quoted 8 lines · Malcolm Beattie
Quoted 6 lines · Keith Owens

All these patches against request_module are attacking the problem at the wrong point. The kernel can request any module name it likes, using any string it likes, as long as the kernel generates the name. The real problem is when the kernel blindly accepts some user input and passes it straight to modprobe, then the kernel is acting like a setuid wrapper for a program that was never designed to run setuid.

Quoted 5 lines

Rather than add sanity checking to modprobe, it would be a lot easier and safer from a security audit point of view to have the kernel call /sbin/kmodprobe instead of /sbin/modprobe. Then kmodprobe can sanitise all the data and exec the real modprobe. That way the only thing that needs auditing is a string munging/sanitising program.

<spock>Insufficient data, Captain</spock>. By the time request_module is called, the code has no idea if the text is valid or not because all information about where the string came from is in the higher levels. Using kmodprobe instead of modprobe cannot change that fact. As long as the kernel passes user input to modprobe unchanged, any user can load any module. Post validation has no way of catching that.

The kernel is allowed to request module names containing '_', '-', '/' (see devfs) and possibly ' '. That kills a lot of the sanitisation attempts. OTOH, modprobe can detect that its input came from the kernel and treat it as tainted, forcing the last parameter to be treated as a module name, even if it contains special characters and suppressing meta expansion for this "name". AFAICT, that makes all kernel input to modprobe safe. Wait for modutils 2.3.20 then hunt for any loopholes.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

#13 lore raw
In reply to Peter Samuelson
Quoted 8 lines · Malcolm Beattie
Quoted 6 lines · Keith Owens

All these patches against request_module are attacking the problem at the wrong point. The kernel can request any module name it likes, using any string it likes, as long as the kernel generates the name. The real problem is when the kernel blindly accepts some user input and passes it straight to modprobe, then the kernel is acting like a setuid wrapper for a program that was never designed to run setuid.

Quoted 5 lines

Rather than add sanity checking to modprobe, it would be a lot easier and safer from a security audit point of view to have the kernel call /sbin/kmodprobe instead of /sbin/modprobe. Then kmodprobe can sanitise all the data and exec the real modprobe. That way the only thing that needs auditing is a string munging/sanitising program.

Well, no matter what kernel needs auditing as well, the fact that dev_load will without any check load any module the user wants is already problematic and no munging helps with it at all, especially loading old ISA drivers might not be a good idea.

Jakub
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

#14 lore raw
Quoted 10 lines · Jakub Jelinek
Quoted 5 lines

Rather than add sanity checking to modprobe, it would be a lot easier and safer from a security audit point of view to have the kernel call /sbin/kmodprobe instead of /sbin/modprobe. Then kmodprobe can sanitise all the data and exec the real modprobe. That way the only thing that needs auditing is a string munging/sanitising program.

Well, no matter what kernel needs auditing as well, the fact that dev_load will without any check load any module the user wants is already problematic and no munging helps with it at all, especially loading old ISA drivers might not be a good idea.

FWIW: A quick look at the kernel source, and dev_load() seems to be the only place that does this. Other places apply prefixes to user supplied names.

Cheers
Chris

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

#15 lore raw
In reply to Torsten Duwe
Quoted 7 lines · [email protected] (Torsten Duwe) writes:
Quoted 1 line · Chris Evans

What's wrong with isalnum() ?

Hm, must admit that I wasn't 100% sure if that's in the kernel lib or an evil gcc expands it inline to some static array lookup. Now I see that it's already in the kernel. Do some of you also sometimes wonder why the kernel is so big ;-) ?

Someone could make it a bit smaller by patching fs/jffs/interp.c and arch/ppc/xmon/xmon.c to use the kernel lib, rather than their own versions.

Signature · 6 lines
 `O O'  | [email protected]
// ^ \\ | http://www.pyrites.org.uk/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/
#16 lore raw
Quoted 3 lines · [email protected] said:

Someone could make it a bit smaller by patching fs/jffs/interp.c and
arch/ppc/xmon/xmon.c to use the kernel lib, rather than their own versions.

Makes sense to me. Patch attached. As an added bonus, this patch (not the ctype change) also speeds up JFFS mounting by about an order of magnitude - by reading from the flash 4KiB at a time into a RAM cache, rather than scanning it a word at a time. Yeah, alright - I was looking for an excuse to update intrep.c anyway :)

Index: intrep.c
===================================================================
RCS file: /inst/cvs/linux/fs/jffs/Attic/intrep.c,v retrieving revision 1.1.2.4

intrep.c +83 -55
diff -u -r1.1.2.4 intrep.c
@@ -10,7 +10,8 @@
  * the Free Software Foundation; either version 2 of the License, or
  * (at your option) any later version.
  *
- * $Id: intrep.c,v 1.69 2000/08/24 09:35:47 dwmw2 Exp $
+ * - Based on Id: intrep.c,v 1.71 2000/10/27 16:51:29 dwmw2 Exp
+ * - With the ctype() changes from v1.77.
  *
  * Ported to Linux 2.3.x and MTD:
  * Copyright (C) 2000  Alexander Larsson ([email protected]), Cendio Systems AB

@@ -68,15 +69,11 @@
 #include <linux/version.h>
 #include <linux/smp_lock.h>
 #include <linux/sched.h>
+#include <linux/ctype.h>
 
-
 #include "intrep.h"
 #include "jffs_fm.h"
 
-#if LINUX_VERSION_CODE < 0x20300
-#define set_current_state(x) do{current->state = x;} while (0)
-#endif
-
 #if defined(JFFS_MEMORY_DEBUG) && JFFS_MEMORY_DEBUG
 long no_jffs_file = 0;
 long no_jffs_node = 0;

@@ -94,48 +91,7 @@
 static __u8 flash_read_u8(struct mtd_info *mtd, loff_t from);
 
 #if 1
-#define _U      01
-#define _L      02
-#define _N      04
-#define _S      010
-#define _P      020
-#define _C      040
-#define _X      0100
-#define _B      0200
-
-const unsigned char jffs_ctype_[1 + 256] = {
-	0,
-	_C,     _C,     _C,     _C,     _C,     _C,     _C,     _C,
-	_C,     _C|_S,  _C|_S,  _C|_S,  _C|_S,  _C|_S,  _C,     _C,
-	_C,     _C,     _C,     _C,     _C,     _C,     _C,     _C,
-	_C,     _C,     _C,     _C,     _C,     _C,     _C,     _C,
-	_S|_B,  _P,     _P,     _P,     _P,     _P,     _P,     _P,
-	_P,     _P,     _P,     _P,     _P,     _P,     _P,     _P,
-	_N,     _N,     _N,     _N,     _N,     _N,     _N,     _N,
-	_N,     _N,     _P,     _P,     _P,     _P,     _P,     _P,
-	_P,     _U|_X,  _U|_X,  _U|_X,  _U|_X,  _U|_X,  _U|_X,  _U,
-	_U,     _U,     _U,     _U,     _U,     _U,     _U,     _U,
-	_U,     _U,     _U,     _U,     _U,     _U,     _U,     _U,
-	_U,     _U,     _U,     _P,     _P,     _P,     _P,     _P,
-	_P,     _L|_X,  _L|_X,  _L|_X,  _L|_X,  _L|_X,  _L|_X,  _L,
-	_L,     _L,     _L,     _L,     _L,     _L,     _L,     _L,
-	_L,     _L,     _L,     _L,     _L,     _L,     _L,     _L,
-	_L,     _L,     _L,     _P,     _P,     _P,     _P,     _C
-};
-
-#define jffs_isalpha(c)      ((jffs_ctype_+1)[(int)c]&(_U|_L))
-#define jffs_isupper(c)      ((jffs_ctype_+1)[(int)c]&_U)
-#define jffs_islower(c)      ((jffs_ctype_+1)[(int)c]&_L)
-#define jffs_isdigit(c)      ((jffs_ctype_+1)[(int)c]&_N)
-#define jffs_isxdigit(c)     ((jffs_ctype_+1)[(int)c]&(_X|_N))
-#define jffs_isspace(c)      ((jffs_ctype_+1)[(int)c]&_S)
-#define jffs_ispunct(c)      ((jffs_ctype_+1)[(int)c]&_P)
-#define jffs_isalnum(c)      ((jffs_ctype_+1)[(int)c]&(_U|_L|_N))
-#define jffs_isprint(c)      ((jffs_ctype_+1)[(int)c]&(_P|_U|_L|_N|_B))
-#define jffs_isgraph(c)      ((jffs_ctype_+1)[(int)c]&(_P|_U|_L|_N))
-#define jffs_iscntrl(c)      ((jffs_ctype_+1)[(int)c]&_C)
-
-void
+static void
 jffs_hexdump(struct mtd_info *mtd, loff_t pos, int size)
 {
 	char line[16];

@@ -169,7 +125,7 @@
 		printk("  ");
 
 		for (i = 0; i < j; i++) {
-			if (jffs_isgraph(line[i])) {
+			if (isgraph(line[i])) {
 				printk("%c", line[i]);
 			}
 			else {

@@ -193,9 +149,12 @@
 	size_t retlen;
 	int res;
 
+	D3(printk(KERN_NOTICE "flash_safe_read(%p, %08x, %p, %08x)\n",
+		  mtd, from, buf, count));
+
 	res = MTD_READ(mtd, from, count, &retlen, buf);
 	if (retlen != count) {
-		printk("Didn't read all bytes in flash_safe_read(). Returned %d\n", res);
+		panic("Didn't read all bytes in flash_safe_read(). Returned %d\n", res);
 	}
 	return res?res:retlen;
 }

@@ -367,9 +326,37 @@
 {
 	__u32 sum = 0;
 	loff_t ptr = start;
-	while (size-- > 0) {
-		sum += flash_read_u8(mtd, ptr++);
+	__u8 *read_buf;
+	int i, length;
+
+	/* Allocate read buffer */
+	read_buf = (__u8 *) kmalloc (sizeof(__u8) * 4096, GFP_KERNEL);
+
+	/* Loop until checksum done */
+	while (size) {
+		/* Get amount of data to read */
+		if (size < 4096)
+			length = size;
+		else
+			length = 4096;
+
+		/* Perform flash read */
+		D3(printk(KERN_NOTICE "jffs_checksum_flash\n"));
+		flash_safe_read(mtd, ptr, &read_buf[0], length);
+
+		/* Compute checksum */
+		for (i=0; i < length ; i++)
+			sum += read_buf[i];
+
+		/* Update pointer and size */
+		size -= length;
+		ptr += length;
 	}
+
+	/* Free read buffer */
+	kfree (read_buf);
+
+	/* Return result */
 	D3(printk("checksum result: 0x%08x\n", sum));
 	return sum;
 }

@@ -609,12 +596,17 @@
 	loff_t pos = fmc->flash_start;
 	loff_t start;
 	loff_t end = fmc->flash_start + fmc->flash_size;
+	__u8 *read_buf;
+	int i, len, retlen;
 
 	D1(printk("jffs_scan_flash(): start pos = 0x%lx, end = 0x%lx\n",
 		  (long)pos, (long)end));
 
 	flash_safe_acquire(fmc->mtd);
 
+	/* Allocate read buffer */
+	read_buf = (__u8 *) kmalloc (sizeof(__u8) * 4096, GFP_KERNEL);
+
 	/* Start the scan.  */
 	while (pos < end) {
 		deleted_file = 0;

@@ -629,9 +621,22 @@
 			   something else than 0xff is found.  */
 			D1(printk("jffs_scan_flash(): 0xff at pos 0x%lx.\n",
 				  (long)pos));
-			for (; pos < end
-			       && JFFS_EMPTY_BITMASK == flash_read_u32(fmc->mtd, pos);
-			     pos += 4);
+
+			len = end - pos < 4096 ? end - pos : 4096;
+
+			retlen = flash_safe_read(fmc->mtd, pos,
+						 &read_buf[0], len);
+
+			retlen &= ~3;
+
+			for (i=0 ; i < retlen ; i+=4, pos += 4) {
+				if(*((__u32 *) &read_buf[i]) !=
+						JFFS_EMPTY_BITMASK)
+					break;
+			}
+			if (i == retlen)
+				continue;
+
 			D1(printk("jffs_scan_flash(): 0xff ended at "
 				  "pos 0x%lx.\n", (long)pos));
 

@@ -748,7 +753,12 @@
 			if (!(node = (struct jffs_node *)
 				     kmalloc(sizeof(struct jffs_node),
 					     GFP_KERNEL))) {
+				/* Free read buffer */
+				kfree (read_buf);
+
+				/* Release the flash device */
 				flash_safe_release(fmc->mtd);
+	
 				return -ENOMEM;
 			}
 			DJM(no_jffs_node++);

@@ -893,7 +903,13 @@
 				D(printk("jffs_scan_flash(): !node->fm\n"));
 				kfree(node);
 				DJM(no_jffs_node--);
+
+				/* Free read buffer */
+				kfree (read_buf);
+
+				/* Release the flash device */
 				flash_safe_release(fmc->mtd);
+
 				return -ENOMEM;
 			}
 			if ((err = jffs_insert_node(c, 0, &raw_inode,

@@ -911,7 +927,13 @@
 					D(printk("jffs_scan_flash: !dl\n"));
 					kfree(node);
 					DJM(no_jffs_node--);
+
+					/* Release the flash device */
 					flash_safe_release(fmc->flash_part);
+
+					/* Free read buffer */
+					kfree (read_buf);
+
 					return -ENOMEM;
 				}
 				dl->ino = deleted_file;

@@ -936,6 +958,11 @@
 		DJM(no_jffs_node--);
 	}
 	jffs_build_end(fmc);
+
+	/* Free read buffer */
+	kfree (read_buf);
+
+	/* Return happy */
 	D3(printk("jffs_scan_flash(): Leaving...\n"));
 	flash_safe_release(fmc->mtd);
 	return 0;

@@ -1598,6 +1625,7 @@
 		  f->name, node->ino, node->version, node_offset));
 
 	r = jffs_min(avail, max_size);
+	D3(printk(KERN_NOTICE "jffs_get_node_data\n"));
 	flash_safe_read(fmc->mtd, pos, buf, r);
 
 	D3(printk("  jffs_get_node_data(): Read %u byte%s.\n",

--
dwmw2

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

#17 lore raw
In reply to Torsten Duwe
Quoted 5 lines
Quoted 1 line
Quoted 1 line

+ if ((*p & 0xdf) >= 'a' && (*p & 0xdf) <= 'z') continue;

Francis> Just in case... Some modules have uppercase letters too :)

That's what the &0xdf is intended for...

That looks wrong for UTF8 which is technically what the kernel uses 8) -
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

#18 lore raw
Quoted 3 lines
Quoted 3 lines
Quoted 2 lines
Quoted 2 lines

+ if ((*p & 0xdf) >= 'a' && (*p & 0xdf) <= 'z') continue;

Francis> Just in case... Some modules have uppercase letters too :) >> That's what the &0xdf is intended for...

Jah, Bummer from my side; use "|0x20" instead. But as discussed, isalnum() does the perfect job, for readability and efficiency.

Alan> That looks wrong for UTF8 which is technically what the kernel uses Alan> 8)

Hmm, haf-a-amiley. Are module names to be localized or are they considered "international code" like the sources, limiting them to 7-Bit ASCII.

What's your opinion, Alan ? Linus ?

I'd consider it "system internal", not visible to the user and hence 7-Bit must suffice. I also strongly agree with Keith: treating strings that come from the kernel as tainted is weird at least.

I suggest to stick with [A-Za-z0-9_-]*, adding a check for the first char not being '-', maybe modifying devfs do use dashes ("dev-") and auditing the rest of the kernel BTW. The M$-FSes look a little suspicious to me with their "nls_*" stuff. CAP_SYS_MOUNT (once established) might then be turned into CAP_SYS_MODULE this way (mount -t vfat -o conv="; chmod ...") ???

Keith: what about the good-ole' "--" to specify end-of-options ? Every word after that could be treated as a simple module name.

Torsten

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

#19 lore raw
Quoted 3 lines

I'd consider it "system internal", not visible to the user and hence 7-Bit must suffice. I also strongly agree with Keith: treating strings that come from the kernel as tainted is weird at least.

I would consider it to be an arbitary 8bit bytesequence. Fix the user space

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

#20 lore raw
In reply to Torsten Duwe
Followup to:  <[email protected]>
By author:    Alan Cox <[email protected]>
In newsgroup: linux.dev.kernel
Quoted 9 lines
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines

+ if ((*p & 0xdf) >= 'a' && (*p & 0xdf) <= 'z') continue;

Francis> Just in case... Some modules have uppercase letters too :)

That's what the &0xdf is intended for...

That looks wrong for UTF8 which is technically what the kernel uses 8)

No, it's correct, actually, but probably not what you want. It will include all letters [A-Za-z], but if a module named "ärlig"...

-hpa

Signature · 7 lines
<[email protected]> at work, <[email protected]> in private!
"Unix gives you enough rope to shoot yourself in the foot."
http://www.zytor.com/~hpa/puzzle.txt
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/
#21 lore raw
Quoted 2 lines · H. Peter Anvin

No, it's correct, actually, but probably not what you want. It will include all letters [A-Za-z], but if a module named "ärlig"...

Trying to sanitise the module name in request_module is the wrong fix anyway, the kernel can ask for any module name it likes. What it must not do is treat user supplied input _unchanged_ as a module name.

modutils 2.3.20 (just released) fixes all the known local root exploits, without kernel changes. However 2.3.20 does nothing about this problem: "ping6 -I module_name" which lets any user load any module. That problem exists because the kernel passes user supplied data unchanged to request_module. The only fix is to add a prefix to user supplied input (say 'user-interface-') before passing the text to request_module. This has to be fixed in the higher layers of the kernel, it cannot be fixed in request_module or modprobe.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

#22 lore raw
Quoted 10 lines · Keith Owens
Quoted 2 lines · H. Peter Anvin

No, it's correct, actually, but probably not what you want. It will include all letters [A-Za-z], but if a module named "ärlig"...

Trying to sanitise the module name in request_module is the wrong fix anyway, the kernel can ask for any module name it likes. What it must not do is treat user supplied input _unchanged_ as a module name.

Quoted 9 lines

modutils 2.3.20 (just released) fixes all the known local root exploits, without kernel changes. However 2.3.20 does nothing about this problem: "ping6 -I module_name" which lets any user load any module. That problem exists because the kernel passes user supplied data unchanged to request_module. The only fix is to add a prefix to user supplied input (say 'user-interface-') before passing the text to request_module. This has to be fixed in the higher layers of the kernel, it cannot be fixed in request_module or modprobe.

Sure, but if you have to change the kernel anyway you ought to pass the "--" option so modprobe knows that regardless what the string is, it's a module name and not an option.

-hpa

Signature · 7 lines
<[email protected]> at work, <[email protected]> in private!
"Unix gives you enough rope to shoot yourself in the foot."
http://www.zytor.com/~hpa/puzzle.txt
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/
#23 lore raw
In reply to Torsten Duwe

[Torsten Duwe]

Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Francis" == Francis Galiegue <[email protected]> writes:

+ if ((*p & 0xdf) >= 'a' && (*p & 0xdf) <= 'z') continue;

Francis> Just in case... Some modules have uppercase letters too :)

That's what the &0xdf is intended for...

It's wrong, then: you've converted to uppercase, not lowercase.

request_module is not a fast path. Do it the obvious, unoptimized way:

  if ((*p < 'a' || *p > 'z') &&
      (*p < 'A' || *p > 'Z') &&
      (*p < '0' || *p > '9') &&
      *p != '-' && *p != '_')
    return -EINVAL;

Peter
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

#24 lore raw
In reply to Torsten Duwe
Quoted 12 lines · Peter Samuelson

[Torsten Duwe]

Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Francis" == Francis Galiegue <[email protected]> writes:

+ if ((*p & 0xdf) >= 'a' && (*p & 0xdf) <= 'z') continue;

Francis> Just in case... Some modules have uppercase letters too :)

That's what the &0xdf is intended for...

It's wrong, then: you've converted to uppercase, not lowercase.

Quoted 7 lines

request_module is not a fast path. Do it the obvious, unoptimized way:

  if ((*p < 'a' || *p > 'z') &&
      (*p < 'A' || *p > 'Z') &&
      (*p < '0' || *p > '9') &&
      *p != '-' && *p != '_')
    return -EINVAL;

Heading in the right direction, but this is equivalent to:

  if (isalnum(*p) && *p != '-' && *p != '_') return -EINVAL;

which is faster, smaller and easier to read.

--
Daniel
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

#25 lore raw
In reply to Torsten Duwe
Quoted 18 lines · David Relson
Quoted 5 lines · Daniel Phillips

Heading in the right direction, but this is equivalent to:

  if (isalnum(*p) && *p != '-' && *p != '_') return -EINVAL;

which is faster, smaller and easier to read.

Almost right, but you forgot to negate isalnum(). Should be:

         if (!isalnum(*p) && *p != '-' && *p != '_') return -EINVAL;

or

         if (! (isalnum(*p) || *p == '-' || *p == '_')) return -EINVAL;

I think I prefer the older version with "continue" as I don't have to think about all the negatives ("!"), i.e.

Quoted 7 lines
         for ( ... )
         {
                 if ( isalnum(*p) || *p == '-' || *p == '_' )
                         continue;
                 return -EINVAL;
         }

I reserve the right to make coding errors, thanks for not letting it get written into history :-)

How about:

  for ( ... ) if (!isalnum(*p) && !strchr("-_", *p)) return -EINVAL;
Signature · 5 lines
Daniel
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/
#26 lore raw

Daniel,

At 09:23 AM 11/14/00, you wrote:
I reserve the right to make coding errors, thanks for not letting it get written into history :-)

I'm not going to give up my right to make errors until I'm ready to give up my keyboard. I'll probably be pushing up daisies at that point in my life.

How about:

  for ( ... ) if (!isalnum(*p) && !strchr("-_", *p)) return -EINVAL;

I think that is correct. However, it fails the "easy to understand" criterion, so I don't like it.]

David
--------------------------------------------------------

David Relson                   Osage Software Systems, Inc.
[email protected]       514 W. Keech Ave.
www.osagesoftware.com          Ann Arbor, MI 48103
voice: 734.821.8800            fax: 734.821.8800

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/