misc fixes from 2.4.33-ow1

Willy and all,

Attached are 7 small changes from 2.4.33-ow1, as separate patches. I do not feel that these warrant separate messages.

linux-2.4.33-ow1-BAD_ADDR.diff

This one is a one-liner, in binfmt_elf.c:

(quoted diff) +1 -1

-#define BAD_ADDR(x)	((unsigned long)(x) > TASK_SIZE)
+#define BAD_ADDR(x)	((unsigned long)(x) >= TASK_SIZE)

I feel that it is more logical to have BAD_ADDR() defined in this way:
indeed, the first kernel-space address is unusable for userspace. I don't think this change affects anything, and we had it in -ow for a couple of years.

The proper fix would then be :

fs/binfmt_elf.c +1 -1

-#define BAD_ADDR(x)	((unsigned long)(x) > TASK_SIZE)
+#define BAD_ADDR(x)	((unsigned long)(x) >= TASK_SIZE)

fs/binfmt_elf.c +1 -1

-	    if (k > TASK_SIZE || eppnt->p_filesz > eppnt->p_memsz ||
+	    if (BAD_ADDR(k) || eppnt->p_filesz > eppnt->p_memsz ||

And even then, I'm not happy with this test :

   TASK_SIZE - elf_ppnt->p_memsz < k

because it means that we only raise the error when

   k + elf_ppnt->p_memsz > TASK_SIZE

I really think that we want to check this instead :

   k + elf_ppnt->p_memsz >= TASK_SIZE

Otherwise we leave a window where this is undetected :

   load_addr + eppnt->p_vaddr == TASK_SIZE - eppnt->p_memsz

This will later lead to last_bss getting readjusted to TASK_SIZE, which I don't think is expected at all :

            k = load_addr + eppnt->p_memsz + eppnt->p_vaddr;
            if (k > last_bss)
                last_bss = k;

Then I think we should change this at both places :

Quoted 1 line · Willy Tarreau

The proper fix would then be :

[...]

Quoted 2 lines

fs/binfmt_elf.c +1 -1

-#define BAD_ADDR(x)	((unsigned long)(x) > TASK_SIZE)
+#define BAD_ADDR(x)	((unsigned long)(x) >= TASK_SIZE)

[...]

Quoted 2 lines

fs/binfmt_elf.c +1 -1

-	    if (k > TASK_SIZE || eppnt->p_filesz > eppnt->p_memsz ||
+	    if (BAD_ADDR(k) || eppnt->p_filesz > eppnt->p_memsz ||

[...]

fs/binfmt_elf.c #endif · line 775

Quoted 2 lines

fs/binfmt_elf.c +1 -1

-		if (k > TASK_SIZE || elf_ppnt->p_filesz > elf_ppnt->p_memsz ||
+		if (BAD_ADDR(k) || elf_ppnt->p_filesz > elf_ppnt->p_memsz ||

Looks OK to me.

Quoted 25 lines

And even then, I'm not happy with this test :

   TASK_SIZE - elf_ppnt->p_memsz < k

because it means that we only raise the error when

   k + elf_ppnt->p_memsz > TASK_SIZE

I really think that we want to check this instead :

   k + elf_ppnt->p_memsz >= TASK_SIZE

Otherwise we leave a window where this is undetected :

   load_addr + eppnt->p_vaddr == TASK_SIZE - eppnt->p_memsz

This will later lead to last_bss getting readjusted to TASK_SIZE, which I don't think is expected at all :

            k = load_addr + eppnt->p_memsz + eppnt->p_vaddr;
            if (k > last_bss)
                last_bss = k;

Then I think we should change this at both places :

Review

Quoted 2 lines

(quoted diff) +1 -1

- 		    TASK_SIZE - elf_ppnt->p_memsz < k) {
+		    BAD_ADDR(k + elf_ppnt->p_memsz)) {

I am not sure about these re-arrangements - I'd need to review them in full context to make sure that we don't inadvertently change things as it relates to behavior on integer overflows, which I presently do not have the time for. I'll trust that you do such a review with integer overflows and variable type differences (size, signedness) in mind now that I've mentioned this potential danger. ;-) Alternatively, you can simply change the comparisons from < to <= and from > to >= rather than use the BAD_ADDR() macro.

Thanks,

Alexander

Quoted 6 lines · Solar Designer

Quoted 1 line · Willy Tarreau

The proper fix would then be :

[...]

Quoted 2 lines

fs/binfmt_elf.c +1 -1

-#define BAD_ADDR(x)	((unsigned long)(x) > TASK_SIZE)
+#define BAD_ADDR(x)	((unsigned long)(x) >= TASK_SIZE)

[...]

Quoted 7 lines

Quoted 2 lines

fs/binfmt_elf.c +1 -1

-	    if (k > TASK_SIZE || eppnt->p_filesz > eppnt->p_memsz ||
+	    if (BAD_ADDR(k) || eppnt->p_filesz > eppnt->p_memsz ||

[...]

fs/binfmt_elf.c #endif · line 775

Quoted 2 lines

fs/binfmt_elf.c +1 -1

-		if (k > TASK_SIZE || elf_ppnt->p_filesz > elf_ppnt->p_memsz ||
+		if (BAD_ADDR(k) || elf_ppnt->p_filesz > elf_ppnt->p_memsz ||

Looks OK to me.

Quoted 8 lines

Quoted 8 lines

And even then, I'm not happy with this test :

   TASK_SIZE - elf_ppnt->p_memsz < k

because it means that we only raise the error when

   k + elf_ppnt->p_memsz > TASK_SIZE

Quoted 7 lines

Quoted 7 lines

I really think that we want to check this instead :

   k + elf_ppnt->p_memsz >= TASK_SIZE

Otherwise we leave a window where this is undetected :

   load_addr + eppnt->p_vaddr == TASK_SIZE - eppnt->p_memsz

Quoted 6 lines

Quoted 6 lines

This will later lead to last_bss getting readjusted to TASK_SIZE, which I don't think is expected at all :

            k = load_addr + eppnt->p_memsz + eppnt->p_vaddr;
            if (k > last_bss)
                last_bss = k;

[...] But before this, I'd like to get comments from
the people who discussed the subject recently.

Quoted 10 lines · Willy Tarreau

Quoted 9 lines · Solar Designer

Quoted 1 line · Willy Tarreau

The proper fix would then be :

[...]

Quoted 2 lines

fs/binfmt_elf.c +1 -1

-#define BAD_ADDR(x)	((unsigned long)(x) > TASK_SIZE)
+#define BAD_ADDR(x)	((unsigned long)(x) >= TASK_SIZE)

[...]

Quoted 2 lines

fs/binfmt_elf.c +1 -1

-	    if (k > TASK_SIZE || eppnt->p_filesz > eppnt->p_memsz ||
+	    if (BAD_ADDR(k) || eppnt->p_filesz > eppnt->p_memsz ||

[...]

Quoted 4 lines

Quoted 4 lines

fs/binfmt_elf.c #endif · line 775

Quoted 2 lines

fs/binfmt_elf.c +1 -1

-		if (k > TASK_SIZE || elf_ppnt->p_filesz > elf_ppnt->p_memsz ||
+		if (BAD_ADDR(k) || elf_ppnt->p_filesz > elf_ppnt->p_memsz ||

Looks OK to me.

These are all correct.

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

And even then, I'm not happy with this test :

   TASK_SIZE - elf_ppnt->p_memsz < k

because it means that we only raise the error when

   k + elf_ppnt->p_memsz > TASK_SIZE

I really think that we want to check this instead :

   k + elf_ppnt->p_memsz >= TASK_SIZE

Otherwise we leave a window where this is undetected :

Quoted 1 line

Quoted 1 line

Quoted 1 line

   load_addr + eppnt->p_vaddr == TASK_SIZE - eppnt->p_memsz

The reason I did not propose changing these is because these are end-point checks (as opposed to starting address checks). I think that the following "equals" condition is conceptually valid:

	(starting-address + region-size == TASK_SIZE)

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

This will later lead to last_bss getting readjusted to TASK_SIZE, which I don't think is expected at all :

            k = load_addr + eppnt->p_memsz + eppnt->p_vaddr;
            if (k > last_bss)
                last_bss = k;

This is an interesting case, but I think the error checking works okay.

After the ELF phdr loop, the resulting "last_bss" is used as follows:

	/* Map the last of the bss segment */
	if (last_bss > elf_bss) {
		down_write(&current->mm->mmap_sem);
		error = do_brk(elf_bss, last_bss - elf_bss);
		up_write(&current->mm->mmap_sem);
		if (BAD_ADDR(error))
			goto out_close;
	}

The variable "last_bss" is used to compute the size argument in the call to do_brk(). If the section extends beyond TASK_SIZE, then do_brk() will return -EINVAL. If the do_brk() call succeeds but "elf_bss" is itself exactly at TASK_SIZE, then the BAD_ADDR() call above will catch it.

Quoted 2 lines

[...] But before this, I'd like to get comments from
the people who discussed the subject recently.

Thus, I think that both 2.4.33 and 2.6.<latest> are okay without any further changes.

Cheers. -ernie

Quoted 5 lines

Quoted 2 lines

[...] But before this, I'd like to get comments from
the people who discussed the subject recently.

Thus, I think that both 2.4.33 and 2.6.<latest> are okay without any further changes.

At least 2.4 needs the fix to use the correct BAD_ADDR (which is not OK in 2.4.33 yet).

fs/binfmt_elf.c static int load_elf_binary(struct linux_ · line 826

Quoted 2 lines · Ernie Petrides

fs/binfmt_elf.c renamed from fs/binfmt_elf.c.orig +0 -2

-			printk(KERN_ERR "Unable to load interpreter %.128s\n",
-				elf_interpreter);

I'd rather have this message rate-limited, not dropped completely.

Another long-time concern that I had is that we've got some printk()s of user-supplied string data. What about embedded linefeeds - can this be used to produce fake kernel messages with arbitrary log level (syslog priority)? It certainly seems so.

Also, there are terminal controls...

Quoted 5 lines · Solar Designer

fs/binfmt_elf.c static int load_elf_binary(struct linux_ · line 826

Quoted 2 lines · Ernie Petrides

fs/binfmt_elf.c renamed from fs/binfmt_elf.c.orig +0 -2

-			printk(KERN_ERR "Unable to load interpreter %.128s\n",
-				elf_interpreter);

I'd rather have this message rate-limited, not dropped completely.

I consider any printk() that can be arbitrarily triggered by an unprivileged user to be inappropriate, rate-limited or not. I recommend that it be removed entirely.

Quoted 6 lines

Another long-time concern that I had is that we've got some printk()s of user-supplied string data. What about embedded linefeeds - can this be used to produce fake kernel messages with arbitrary log level (syslog priority)? It certainly seems so.

Also, there are terminal controls...

These are valid concerns. Allowing the kernel to print user-fabricated strings is a terrible idea.

Quoted 4 lines · Solar Designer

fs/binfmt_elf.c static int load_elf_binary(struct linux_ · line 826

Quoted 2 lines

fs/binfmt_elf.c renamed from fs/binfmt_elf.c.orig +0 -2

-			printk(KERN_ERR "Unable to load interpreter %.128s\n",
-				elf_interpreter);

I'd rather have this message rate-limited, not dropped completely.

I consider any printk() that can be arbitrarily triggered by an unprivileged user to be inappropriate, rate-limited or not. I recommend that it be removed entirely.

Quoted 6 lines

Another long-time concern that I had is that we've got some printk()s of user-supplied string data. What about embedded linefeeds - can this be used to produce fake kernel messages with arbitrary log level (syslog priority)? It certainly seems so.

Also, there are terminal controls...

These are valid concerns. Allowing the kernel to print user-fabricated strings is a terrible idea.

Quoted 9 lines · Ernie Petrides

Quoted 4 lines · Solar Designer

fs/binfmt_elf.c static int load_elf_binary(struct linux_ · line 826

Quoted 2 lines

fs/binfmt_elf.c renamed from fs/binfmt_elf.c.orig +0 -2

-			printk(KERN_ERR "Unable to load interpreter %.128s\n",
-				elf_interpreter);

I'd rather have this message rate-limited, not dropped completely.

I consider any printk() that can be arbitrarily triggered by an unprivileged user to be inappropriate, rate-limited or not. I recommend that it be removed entirely.

Some of them are quite useful. For example, the presence of this one in dmesg and the logs typically (although not necessarily) indicates that there was an OOM condition - and the interpreter name is useful to know that the message indeed applied to /lib/ld-linux.so.2 rather than to some user-supplied ELF interpreter (that could simply be non-existent). Alan has recently suggested that another rate-limited user triggerable message be introduced for set*uid() failing on transient errors. Then, what about warnings of emulated unaligned accesses on Alpha? Those were useful to me on many occasions. There can be many other examples.

Also, for some printk()s it is difficult to say whether they're user triggerable. A printk() indicating a "machine check" or an ECC error may well be user triggerable on a particular system.

Quoted 9 lines

Quoted 6 lines

Another long-time concern that I had is that we've got some printk()s of user-supplied string data. What about embedded linefeeds - can this be used to produce fake kernel messages with arbitrary log level (syslog priority)? It certainly seems so.

Also, there are terminal controls...

These are valid concerns. Allowing the kernel to print user-fabricated strings is a terrible idea.

Yet there are lots of such printk()s - and I suggest that we make a determination on all of them before we possibly start fixing individual ones. In fact, perhaps there are too many of them to be fixing any in 2.4, unless we determine to somehow harden printk() itself.

Even current->comm is untrusted user input, but there are at least 58 printk()s of it in 2.4.33. (58 is the number spotted by a grep that would only match those that have current->comm on the same line with printk itself.)

Quoted 5 lines · Solar Designer

Yet there are lots of such printk()s - and I suggest that we make a determination on all of them before we possibly start fixing individual ones. In fact, perhaps there are too many of them to be fixing any in 2.4, unless we determine to somehow harden printk() itself.

Quoted 4 lines

Even current->comm is untrusted user input, but there are at least 58 printk()s of it in 2.4.33. (58 is the number spotted by a grep that would only match those that have current->comm on the same line with printk itself.)

I still fail to imagine a useful case for printk("%s") to output non-printable chars verbatim. I really think that the right solution would be for printk() to output all non-printable chars escaped (eg: \n, \r, \xXX).

It would
definitely fix the problem without removing useful information. I remember having had the problem a long time ago with a name extracted from a string supplied by the BIOS which mangled the dmesg at early boot.

It would be too much work (and too risky) to expect from all drivers to check their strings for correctness, so the hardening way would be the best one IMHO.

Quoted 6 lines · Willy Tarreau

Quoted 5 lines · Solar Designer

Yet there are lots of such printk()s - and I suggest that we make a determination on all of them before we possibly start fixing individual ones. In fact, perhaps there are too many of them to be fixing any in 2.4, unless we determine to somehow harden printk() itself.

Quoted 8 lines

Quoted 4 lines

Even current->comm is untrusted user input, but there are at least 58 printk()s of it in 2.4.33. (58 is the number spotted by a grep that would only match those that have current->comm on the same line with printk itself.)

I still fail to imagine a useful case for printk("%s") to output non-printable chars verbatim. I really think that the right solution would be for printk() to output all non-printable chars escaped (eg: \n, \r, \xXX).

Well, this would be ASCII codes 0 through 0x1f and 0x7f through 0x9f. Unfortunately, some of the latter correspond to national letters and other visible characters with weird charsets:

http://en.wikipedia.org/wiki/Windows_code_page

but perhaps it is OK to not support them in kernel logs - syslogd's printline() would escape them anyway, so we can do it earlier to also protect the console.

Would you escape backslashes themselves, though? Perhaps not. syslogd (as maintained in Linux sysklogd) doesn't. Yes, this escaping is not reliably reversible in that case.

Quoted 7 lines

It would
definitely fix the problem without removing useful information. I remember having had the problem a long time ago with a name extracted from a string supplied by the BIOS which mangled the dmesg at early boot.

It would be too much work (and too risky) to expect from all drivers to check their strings for correctness, so the hardening way would be the best one IMHO.

I agree.

Alexander

I think that we are trying to protect against :
1) local users faking kernel messages. (eg: disk errors, oopses, ...)
2) local users changing console settings (eg: black on black)
3) local users corrupting the log reader's terminal

1) is relatively easy to do, basically if we escape \b, \r and \n, it will become hard to produce fake logs.

2) should be as easy. I'm not aware of any way to change a local console setting with non-control chars (>= 0x20)

3) is a problem of interpretation between the program storing the logs on disk and the one retrieving them and showing them to the user. It's by no way a kernel problem.

Thus, 1 and 2 could be merged by escaping ...

I don't think we will have to escape the escape char itself. I know that this is dirty and will not make it reliable to reverse the string, but we need to keep in mind that what we are trying to do is not preventing users from hiding their programs' exact names, but preventing them from faking logs. Moreover, sysklogd does not escape the escape char either, so we are not introducing a new weakness by doing this.

Quoted 7 lines · Willy Tarreau

I think that we are trying to protect against :
1) local users faking kernel messages. (eg: disk errors, oopses, ...)
2) local users changing console settings (eg: black on black)
3) local users corrupting the log reader's terminal

1) is relatively easy to do, basically if we escape \b, \r and \n, it will become hard to produce fake logs.

I think it's just '\n'; other characters that you've mentioned achieve
(1) by exploiting (2).

Quoted 2 lines

2) should be as easy. I'm not aware of any way to change a local console setting with non-control chars (>= 0x20)

...except that there are control chars above 0x20:

	http://archives.neohapsis.com/archives/linux/lsap/2000-q2/0151.html

(scroll down to the end). One thing that I did not mention in that older posting is that, IIRC, according to my vt420 manual, macro recording and playback may be invoked via the 8-bit controls (in some terminal modes).

This is not limited to real (hardware) terminals; of the 8-bit controls, at least CSI (0x9b) is typically supported by terminal emulators. Even our linux/drivers/char/console.c supports it.

Although most terminal emulators don't appear to allow for macro recording/playback to be requested with control chars, they do support status requests - to which they respond with certain easy-to-guess strings (an attack would be to create a script in PATH under that name - yes, this has multiple prerequisites).

So I think that we should escape chars in the 0x7f to 0x9f range as well.

Quoted 3 lines

3) is a problem of interpretation between the program storing the logs on disk and the one retrieving them and showing them to the user. It's by no way a kernel problem.

I'd agree, but what about dmesg? It reads directly from the kernel and it is commonly run with output to a tty. OK, maybe dmesg (the userspace program) should be fixed.

Quoted 1 line

Thus, 1 and 2 could be merged by escaping ...

Yes.

In fact, if you do the escaping right when substituting values for "%s", then you'll cover (3) as well.

You probably don't want to modify the behavior of vsnprintf() for all of the kernel; you only want to affect its behavior when called from printk(). So you might need to be calling a private function instead, which would accept an additional argument, and make vsnprintf() a wrapper around that function. Yes, that would be a hack.

Alternatively, you can do the escaping right after the vsnprintf() call, but then it will affect more than just "%s". I am not sure whether this is acceptable.

Thanks,

Alexander

I didn't want to affect anything but "%s". Unfortunately, it seems that it was already too much because there are several users in the kernel which first construct their multi-line strings then call printk("%s") with it :-(

Serial driver version 5.05c (2001-07-08) with MANY_PORTS SHARE_IRQ SERIAL_PCI ISAPNP enabled^J<6>ttyS00 at 0x03f8 (irq = 4) is a 16550A

IP Protocols: ICMP, UDP, TCP, IGMP^J<6>IP: routing cache hash table of 8192 buckets, 64Kbytes

Another idea I had was to add a new format specifier to vsnprintf() to explicitly escape the string (eg: "%S"). But there are so many users of printk() to fix then that I'm not sure we would find them all. However, it would be the real fix and not a hack because what we're trying to do is to enforce controls on some data type, which is exactly the point of this solution.

Quoted 3 lines · Willy Tarreau

I didn't want to affect anything but "%s". Unfortunately, it seems that it was already too much because there are several users in the kernel which first construct their multi-line strings then call printk("%s") with it :-(

I've tried looking up the printk() calls corresponding to the example output that you provided. Those that I've checked do not use "%s" on multi-line strings; rather, they merely embed the final '\n' that is meant to end the line within the last "%s" string. Perhaps we could allow for this special case (that is, not escape the '\n' if it is the last character to be output by the printk() call).

Yes, there will be a few
printk("%s", ...) calls that we won't protect - namely, those that are meant to output user-supplied strings in the middle of a line, yet are invoked separately from the printk() that is meant to complete the line. Those would need to be spotted and fixed individually.

Quoted 1 line

Serial driver version 5.05c (2001-07-08) with MANY_PORTS SHARE_IRQ SERIAL_PCI ISAPNP enabled^J<6>ttyS00 at 0x03f8 (irq = 4) is a 16550A

The first line (before the ^J) is output with:

printk(KERN_INFO "%s version %s%s (%s) with%s", serial_name,
serial_version, LOCAL_VERSTRING, serial_revdate, serial_options);

The linefeed is embedded in serial_options.

Quoted 1 line

IP Protocols: ICMP, UDP, TCP, IGMP^J<6>IP: routing cache hash table of 8192 buckets, 64Kbytes

	printk(KERN_INFO "IP Protocols: ");
	for (p = inet_protocol_base; p != NULL;) {
		struct inet_protocol *tmp = (struct inet_protocol *) p->next;
		inet_add_protocol(p);
		printk("%s%s",p->name,tmp?", ":"\n");
		p = tmp;
	}

As you can see, this one is similar - a single line is formed with multiple printk()s and the linefeed is embedded in the last "%s" string.

Quoted 6 lines

Another idea I had was to add a new format specifier to vsnprintf() to explicitly escape the string (eg: "%S"). But there are so many users of printk() to fix then that I'm not sure we would find them all. However, it would be the real fix and not a hack because what we're trying to do is to enforce controls on some data type, which is exactly the point of this solution.

Yes, I had this thought, too. This would be the cleanest solution, but I'm afraid that it will fail in practice. People will continue introducing printk() calls with plain "%s" where the new "%S" would be needed.

Another problem is that there will be cases where it is difficult to make a determination whether a given string is untrusted user input.

A solution would be to normally use "%S" and only use
"%s" where "%S" wouldn't work. In that case, we could as well swap "%s" and "%S", though - hardening the existing "%s" and introducing "%S" for those callers that depend on the old behavior.

Alexander

Well, I'm not sure about this. Nearly all patches which get merged pass through a public review first, and when you see how many replies you get for and 'else' and and 'if' on two different lines, I expect lots of spontaneous replies such as "use %S for user-supplied strings".

Quoted 4 lines

A solution would be to normally use "%S" and only use
"%s" where "%S" wouldn't work. In that case, we could as well swap "%s" and "%S", though - hardening the existing "%s" and introducing "%S" for those callers that depend on the old behavior.

I'd rather not change "%s" semantics if we introduce another specifier which does exactly what we would expect "%s" to do.

I will try your proposal to retain the trailing '\n' unescaped.

Quoted 4 lines · Willy Tarreau

Well, I'm not sure about this. Nearly all patches which get merged pass through a public review first, and when you see how many replies you get for and 'else' and and 'if' on two different lines, I expect lots of spontaneous replies such as "use %S for user-supplied strings".

I wouldn't rely on that.

Quoted 4 lines

Quoted 4 lines

A solution would be to normally use "%S" and only use
"%s" where "%S" wouldn't work. In that case, we could as well swap "%s" and "%S", though - hardening the existing "%s" and introducing "%S" for those callers that depend on the old behavior.

I think it's the way to go.

Quoted 2 lines

I'd rather not change "%s" semantics if we introduce another specifier which does exactly what we would expect "%s" to do.

Both would be equivalent in most cases. It's better to use "%s" for most cases (either secured or not) and leave "%S" for the bunch of special cases whose authors better know what are they doing.

Quoted 1 line

I will try your proposal to retain the trailing '\n' unescaped.

I think with "%s" and "%S" this is no longer needed.

Krzysztof Halasa

Quoted 1 line

Serial driver version 5.05c (2001-07-08) with MANY_PORTS SHARE_IRQ SERIAL_P

The first line (before the ^J) is output with:

printk(KERN_INFO "%s version %s%s (%s) with%s", serial_name,
serial_version, LOCAL_VERSTRING, serial_revdate, serial_options);

The linefeed is embedded in serial_options.

Quoted 7 lines

Another idea I had was to add a new format specifier to vsnprintf() to explicitly escape the string (eg: "%S"). But there are so many users of printk() to fix then that I'm not sure we would find them all. However, it would be the real fix and not a hack because what we're trying to do is to enforce controls on some data type, which is exactly the point of this solution.

Yes, I had this thought, too. This would be the cleanest solution, but I'm afraid that it will fail in practice. People will continue

Quoted 8 lines · Willy Tarreau

Quoted 5 lines

Quoted 2 lines

[...] But before this, I'd like to get comments from
the people who discussed the subject recently.

Thus, I think that both 2.4.33 and 2.6.<latest> are okay without any further changes.

At least 2.4 needs the fix to use the correct BAD_ADDR (which is not OK in 2.4.33 yet).

Ah, right. (Sorry, I was verifying the change in a RHEL3 tree.)

In that case, I support your patch as posted. But the whole point of that investigation was to fix an exec() vulnerability with a bad ELF entry address. This is addressed in the final hunk of the 2.4.33-based patch below, which includes the changes that you previously posted.

Cheers. -ernie

Signed-off-by Ernie Petrides <[email protected]>

fs/binfmt_elf.c renamed from fs/binfmt_elf.c.orig +11 -6

@@ -77,7 +77,7 @@ static struct linux_binfmt elf_format = 
 	NULL, THIS_MODULE, load_elf_binary, load_elf_library, elf_core_dump, ELF_EXEC_PAGESIZE
 };
 
-#define BAD_ADDR(x)	((unsigned long)(x) > TASK_SIZE)
+#define BAD_ADDR(x)	((unsigned long)(x) >= TASK_SIZE)
 
 static int set_brk(unsigned long start, unsigned long end)
 {

@@ -345,7 +345,7 @@ elf_type, total_size);
 	     * <= p_memsize so it is only necessary to check p_memsz.
 	     */
 	    k = load_addr + eppnt->p_vaddr;
-	    if (k > TASK_SIZE || eppnt->p_filesz > eppnt->p_memsz ||
+	    if (BAD_ADDR(k) || eppnt->p_filesz > eppnt->p_memsz ||
 		eppnt->p_memsz > TASK_SIZE || TASK_SIZE - eppnt->p_memsz < k) {
 	        error = -ENOMEM;
 		goto out_close;

@@ -772,7 +772,7 @@ static int load_elf_binary(struct linux_
 		 * allowed task size. Note that p_filesz must always be
 		 * <= p_memsz so it is only necessary to check p_memsz.
 		 */
-		if (k > TASK_SIZE || elf_ppnt->p_filesz > elf_ppnt->p_memsz ||
+		if (BAD_ADDR(k) || elf_ppnt->p_filesz > elf_ppnt->p_memsz ||
 		    elf_ppnt->p_memsz > TASK_SIZE ||
 		    TASK_SIZE - elf_ppnt->p_memsz < k) {
 			/* set_brk can never work.  Avoid overflows.  */

@@ -822,10 +822,9 @@ static int load_elf_binary(struct linux_
 						    interpreter,
 						    &interp_load_addr);
 		if (BAD_ADDR(elf_entry)) {
-			printk(KERN_ERR "Unable to load interpreter %.128s\n",
-				elf_interpreter);
 			force_sig(SIGSEGV, current);
-			retval = IS_ERR((void *)elf_entry) ? PTR_ERR((void *)elf_entry) : -ENOEXEC;
+			retval = IS_ERR((void *)elf_entry) ?
+					(int)elf_entry : -EINVAL;
 			goto out_free_dentry;
 		}
 		reloc_func_desc = interp_load_addr;

@@ -833,6 +832,12 @@ static int load_elf_binary(struct linux_
 		allow_write_access(interpreter);
 		fput(interpreter);
 		kfree(elf_interpreter);
+	} else {
+		if (BAD_ADDR(elf_entry)) {
+			force_sig(SIGSEGV, current);
+			retval = -EINVAL;
+			goto out_free_dentry;
+		}
 	}
 
 	kfree(elf_phdata);