| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Pine.LNX.4.64.0811271643250.9618@blonde.site>
Date: Thu, 27 Nov 2008 17:34:27 +0000 (GMT)
From: Hugh Dickins <hugh@...itas.com>
To: Pekka Enberg <penberg@...helsinki.fi>
cc: "Rafael J. Wysocki" <rjw@...k.pl>,
Miles Lane <miles.lane@...il.com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Christoph Lameter <cl@...ux-foundation.org>,
Ingo Molnar <mingo@...e.hu>, Tejun Heo <htejun@...il.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Vegard Nossum <vegard.nossum@...il.com>,
Steven Rostedt <rostedt@...dmis.org>,
Arjan van de Ven <arjan@...radead.org>
Subject: Re: 2.6.28-rc6-git1 -- BUG: unable to handle kernel paging request
at ffff8800be8b0019
On Thu, 27 Nov 2008, Pekka Enberg wrote:
> On Thu, Nov 27, 2008 at 3:54 PM, Hugh Dickins <hugh@...itas.com> wrote:
> > I think you're looking at a 2.6.28-rc5 sprint_symbol() there:
> > the world has moved on since those days. I changed it to use the
> > supplied "buffer" instead of local "namebuf" in 2.6.28-rc6, so we
> > have to wonder if my patch is to blame - though I don't see it.
>
> Oh, right. I think I see where this is going. The buffer is coming
> from sysfs and is PAGE_SIZE long. In SLUB, we do check for overflows
> but list_locations() allows us to get as close as 100 bytes bytes from
> the edge of the page. Unfortunately, kallsyms_lookup() does:
>
> const char *kallsyms_lookup(unsigned long addr,
> unsigned long *symbolsize,
> unsigned long *offset,
> char **modname, char *namebuf)
> {
> namebuf[KSYM_NAME_LEN - 1] = 0;
> namebuf[0] = 0;
>
> where KSYM_NAME_LEN is 128 which will tip us over the page edge
> potentially triggering an oops.
Thanks for working that out.
Good find (yes, it was a find!) by Miles.
> The proper fix is probably to pass a
> length of namebuf to kallsyms_lookup() but it's probably best to
> revert the patch this late in the release cycle?
Several possibilities, I don't know which is the right one for now,
hope Andrew can judge.
I don't see passing the length of namebuf to kallsyms_lookup()
as called for: it could have been designed that way from the start,
but it wasn't, and I don't see this bug as any reason to change that.
Reverting my patch for now: that's certainly a reasonable possibility,
but leaves us with several other such bugs. Suggested patch below,
but the ftrace part of it worries me a little, since it's within a
structure and maybe it's a bad idea to enlarge that at this point;
I've also not _really_ done the arithmetic needed for the slub one.
An alternative quick just-for-now fix might be to remove that
namebuf[KSYM_NAME_LEN - 1] = 0;
from kallsyms_lookup(): as I understand it (please check), that
could only make sense in cases where the symbol is KSYM_NAME_LEN
long or longer - in which case, all of the places fixed in the
patch below would be causing corruption already, even without my
patch. I think. Maybe that "= 0" even serves no purpose at all?
Hugh
[PATCH] KSYM_SYMBOL_LEN fixes
Miles Lane tailing /sys files hit a BUG which Pekka Enberg has tracked to
my 966c8c12dc9e77f931e2281ba25d2f0244b06949 sprint_symbol(): use less stack
exposing a bug in slub's list_locations() - kallsyms_lookup() writes a 0
to namebuf[KSYM_NAME_LEN-1], but that was beyond the end of page provided.
The 100 slop which list_locations() allows at end of page looks roughly
enough for all the other stuff it might print after the symbol before
it checks again: break out KSYM_SYMBOL_LEN earlier than before.
Latencytop and ftrace and are using KSYM_NAME_LEN buffers where they need
KSYM_SYMBOL_LEN buffers, and vmallocinfo a 2*KSYM_NAME_LEN buffer where
it wants a KSYM_SYMBOL_LEN buffer: fix those before anyone copies them.
Signed-off-by: Hugh Dickins <hugh@...itas.com>
---
fs/proc/base.c | 2 +-
include/linux/ftrace.h | 2 +-
kernel/latencytop.c | 2 +-
mm/slub.c | 2 +-
mm/vmalloc.c | 2 +-
5 files changed, 5 insertions(+), 5 deletions(-)
--- 2.6.28-rc6/fs/proc/base.c 2008-10-24 09:28:19.000000000 +0100
+++ linux/fs/proc/base.c 2008-11-27 16:39:26.000000000 +0000
@@ -371,7 +371,7 @@ static int lstats_show_proc(struct seq_f
task->latency_record[i].time,
task->latency_record[i].max);
for (q = 0; q < LT_BACKTRACEDEPTH; q++) {
- char sym[KSYM_NAME_LEN];
+ char sym[KSYM_SYMBOL_LEN];
char *c;
if (!task->latency_record[i].backtrace[q])
break;
--- 2.6.28-rc6/include/linux/ftrace.h 2008-11-02 23:17:56.000000000 +0000
+++ linux/include/linux/ftrace.h 2008-11-27 16:39:26.000000000 +0000
@@ -231,7 +231,7 @@ ftrace_init_module(unsigned long *start,
struct boot_trace {
pid_t caller;
- char func[KSYM_NAME_LEN];
+ char func[KSYM_SYMBOL_LEN];
int result;
unsigned long long duration; /* usecs */
ktime_t calltime;
--- 2.6.28-rc6/kernel/latencytop.c 2008-07-13 22:51:29.000000000 +0100
+++ linux/kernel/latencytop.c 2008-11-27 16:39:26.000000000 +0000
@@ -191,7 +191,7 @@ static int lstats_show(struct seq_file *
latency_record[i].time,
latency_record[i].max);
for (q = 0; q < LT_BACKTRACEDEPTH; q++) {
- char sym[KSYM_NAME_LEN];
+ char sym[KSYM_SYMBOL_LEN];
char *c;
if (!latency_record[i].backtrace[q])
break;
--- 2.6.28-rc6/mm/slub.c 2008-10-24 09:28:26.000000000 +0100
+++ linux/mm/slub.c 2008-11-27 16:39:27.000000000 +0000
@@ -3595,7 +3595,7 @@ static int list_locations(struct kmem_ca
for (i = 0; i < t.count; i++) {
struct location *l = &t.loc[i];
- if (len > PAGE_SIZE - 100)
+ if (len > PAGE_SIZE - KSYM_SYMBOL_LEN - 100)
break;
len += sprintf(buf + len, "%7ld ", l->count);
--- 2.6.28-rc6/mm/vmalloc.c 2008-11-21 11:09:24.000000000 +0000
+++ linux/mm/vmalloc.c 2008-11-27 16:39:27.000000000 +0000
@@ -1705,7 +1705,7 @@ static int s_show(struct seq_file *m, vo
v->addr, v->addr + v->size, v->size);
if (v->caller) {
- char buff[2 * KSYM_NAME_LEN];
+ char buff[KSYM_SYMBOL_LEN];
seq_putc(m, ' ');
sprint_symbol(buff, (unsigned long)v->caller);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists