| 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
| ||
|
Date: Mon, 23 May 2011 18:46:36 +0200
From: Jiri Olsa <jolsa@...hat.com>
To: rostedt@...dmis.org
Cc: Jason Baron <jbaron@...hat.com>, mingo@...e.hu,
linux-kernel@...r.kernel.org
Subject: Re: [PATCHv2] jump_label: check entries limit in __jump_label_update
hi,
any feedback?
thanks,
jirka
On Tue, May 10, 2011 at 10:30:45AM -0400, Jason Baron wrote:
> On Tue, May 10, 2011 at 12:43:46PM +0200, Jiri Olsa wrote:
> > On Mon, May 09, 2011 at 03:32:48PM -0400, Jason Baron wrote:
> > > On Wed, May 04, 2011 at 05:30:23PM +0200, Jiri Olsa wrote:
> > > > When iterating the jump_label entries array (core or modules),
> > > > the __jump_label_update function peeks over the last entry.
> > > >
> > > > The reason is that the end of the for loop depends on the key
> > > > value of the processed entry. Thus when going through the
> > > > last array entry, we will touch the memory behind the array
> > > > limit.
> > > >
> > > > This bug probably will never be triggered, since most likely the
> > > > memory behind the jump_label entries will be accesable and the
> > > > entry->key will be different than the expected value.
> > > >
> > > >
> > > > Signed-off-by: Jiri Olsa <jolsa@...hat.com>
> > > > ---
> > > > kernel/jump_label.c | 17 ++++++++++++-----
> > > > 1 files changed, 12 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/kernel/jump_label.c b/kernel/jump_label.c
> > > > index 74d1c09..b2ee97a 100644
> > > > --- a/kernel/jump_label.c
> > > > +++ b/kernel/jump_label.c
> > > > @@ -105,9 +105,12 @@ static int __jump_label_text_reserved(struct jump_entry *iter_start,
> > > > }
> > > >
> > > > static void __jump_label_update(struct jump_label_key *key,
> > > > - struct jump_entry *entry, int enable)
> > > > + struct jump_entry *entry,
> > > > + struct jump_entry *stop, int enable)
> > > > {
> > > > - for (; entry->key == (jump_label_t)(unsigned long)key; entry++) {
> > > > + for (; (entry < stop) &&
> > > > + (entry->key == (jump_label_t)(unsigned long)key);
> > > > + entry++) {
> > > > /*
> > > > * entry->code set to 0 invalidates module init text sections
> > > > * kernel_text_address() verifies we are not in core kernel
> > > > @@ -158,6 +161,7 @@ early_initcall(jump_label_init);
> > > > struct jump_label_mod {
> > > > struct jump_label_mod *next;
> > > > struct jump_entry *entries;
> > > > + struct jump_entry *entries_stop;
> > > > struct module *mod;
> > > > };
> > > >
> > > > @@ -181,7 +185,8 @@ static void __jump_label_mod_update(struct jump_label_key *key, int enable)
> > > > struct jump_label_mod *mod = key->next;
> > > >
> > > > while (mod) {
> > > > - __jump_label_update(key, mod->entries, enable);
> > > > + __jump_label_update(key, mod->entries, mod->entries_stop,
> > > > + enable);
> > > > mod = mod->next;
> > >
> > > hmmm. Instead of adding a new field to the 'struct jump_label_mod' data
> > > structure (and thus increasing its footprint), why not use:
> > > mod->jump_entries + mod->num_jump_entries here?
> >
> > yep, overlooked the struct module pointer inside jump_label_mod
> > attaching new patch
> >
> > thanks,
> > jirka
> >
> > ---
> > When iterating the jump_label entries array (core or modules),
> > the __jump_label_update function peeks over the last entry.
> >
> > The reason is that the end of the for loop depends on the key
> > value of the processed entry. Thus when going through the
> > last array entry, we will touch the memory behind the array
> > limit.
> >
> > This bug probably will never be triggered, since most likely the
> > memory behind the jump_label entries will be accesable and the
> > entry->key will be different than the expected value.
> >
> >
> > Signed-off-by: Jiri Olsa <jolsa@...hat.com>
> > ---
> > kernel/jump_label.c | 18 +++++++++++++-----
> > 1 files changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/kernel/jump_label.c b/kernel/jump_label.c
> > index 74d1c09..fa27e75 100644
> > --- a/kernel/jump_label.c
> > +++ b/kernel/jump_label.c
> > @@ -105,9 +105,12 @@ static int __jump_label_text_reserved(struct jump_entry *iter_start,
> > }
> >
> > static void __jump_label_update(struct jump_label_key *key,
> > - struct jump_entry *entry, int enable)
> > + struct jump_entry *entry,
> > + struct jump_entry *stop, int enable)
> > {
> > - for (; entry->key == (jump_label_t)(unsigned long)key; entry++) {
> > + for (; (entry < stop) &&
> > + (entry->key == (jump_label_t)(unsigned long)key);
> > + entry++) {
> > /*
> > * entry->code set to 0 invalidates module init text sections
> > * kernel_text_address() verifies we are not in core kernel
> > @@ -181,7 +184,11 @@ static void __jump_label_mod_update(struct jump_label_key *key, int enable)
> > struct jump_label_mod *mod = key->next;
> >
> > while (mod) {
> > - __jump_label_update(key, mod->entries, enable);
> > + struct module *m = mod->mod;
> > +
> > + __jump_label_update(key, mod->entries,
> > + m->jump_entries + m->num_jump_entries,
> > + enable);
> > mod = mod->next;
> > }
> > }
> > @@ -245,7 +252,8 @@ static int jump_label_add_module(struct module *mod)
> > key->next = jlm;
> >
> > if (jump_label_enabled(key))
> > - __jump_label_update(key, iter, JUMP_LABEL_ENABLE);
> > + __jump_label_update(key, iter, iter_stop,
> > + JUMP_LABEL_ENABLE);
> > }
> >
> > return 0;
> > @@ -371,7 +379,7 @@ static void jump_label_update(struct jump_label_key *key, int enable)
> >
> > /* if there are no users, entry can be NULL */
> > if (entry)
> > - __jump_label_update(key, entry, enable);
> > + __jump_label_update(key, entry, __stop___jump_table, enable);
> >
> > #ifdef CONFIG_MODULES
> > __jump_label_mod_update(key, enable);
> > --
> > 1.7.1
> >
>
> Looks good.
>
> Acked-by: Jason Baron <jbaron@...hat.com>
>
> Thanks,
>
> -Jason
--
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