Re: [PATCH v2 7/8] jump_label: annotate entries that operate on __init code earlier

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jul 2, 2018 at 11:11 AM, Ard Biesheuvel
<ard.biesheuvel@xxxxxxxxxx> wrote:
> Jump table entries are mostly read-only, with the exception of the
> init and module loader code that defuses entries that point into init
> code when the code being referred to is freed.
>
> For robustness, it would be better to move these entries into the
> ro_after_init section, but clearing the 'code' member of each jump
> table entry referring to init code at module load time races with the
> module_enable_ro() call that remaps the ro_after_init section read
> only, so we'd like to do it earlier.
>
> So given that whether such an entry refers to init code can be decided
> much earlier, we can pull this check forward. Since we may still need
> the code entry at this point, let's switch to setting a low bit in the
> 'key' member just like we do to annotate the default state of a jump
> table entry.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
> ---
>  include/linux/jump_label.h | 11 ++---
>  init/main.c                |  1 -
>  kernel/jump_label.c        | 48 ++++++--------------
>  3 files changed, 18 insertions(+), 42 deletions(-)

Net reduction in code, too! :)

Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>

-Kees

>
> diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
> index 871826fd0c3b..feee8abc96be 100644
> --- a/include/linux/jump_label.h
> +++ b/include/linux/jump_label.h
> @@ -141,7 +141,7 @@ static inline unsigned long jump_entry_target(const struct jump_entry *entry)
>
>  static inline struct static_key *jump_entry_key(const struct jump_entry *entry)
>  {
> -       long offset = entry->key & ~1L;
> +       long offset = entry->key & ~3L;
>
>         return (struct static_key *)((unsigned long)&entry->key + offset);
>  }
> @@ -160,7 +160,7 @@ static inline unsigned long jump_entry_target(const struct jump_entry *entry)
>
>  static inline struct static_key *jump_entry_key(const struct jump_entry *entry)
>  {
> -       return (struct static_key *)((unsigned long)entry->key & ~1UL);
> +       return (struct static_key *)((unsigned long)entry->key & ~3UL);
>  }
>
>  #endif
> @@ -172,12 +172,12 @@ static inline bool jump_entry_is_branch(const struct jump_entry *entry)
>
>  static inline bool jump_entry_is_init(const struct jump_entry *entry)
>  {
> -       return entry->code == 0;
> +       return (unsigned long)entry->key & 2UL;
>  }
>
>  static inline void jump_entry_set_init(struct jump_entry *entry)
>  {
> -       entry->code = 0;
> +       entry->key |= 2;
>  }
>
>  #endif
> @@ -213,7 +213,6 @@ extern struct jump_entry __start___jump_table[];
>  extern struct jump_entry __stop___jump_table[];
>
>  extern void jump_label_init(void);
> -extern void jump_label_invalidate_initmem(void);
>  extern void jump_label_lock(void);
>  extern void jump_label_unlock(void);
>  extern void arch_jump_label_transform(struct jump_entry *entry,
> @@ -261,8 +260,6 @@ static __always_inline void jump_label_init(void)
>         static_key_initialized = true;
>  }
>
> -static inline void jump_label_invalidate_initmem(void) {}
> -
>  static __always_inline bool static_key_false(struct static_key *key)
>  {
>         if (unlikely(static_key_count(key) > 0))
> diff --git a/init/main.c b/init/main.c
> index e59a01f163d6..d1a6b8a896e5 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -1062,7 +1062,6 @@ static int __ref kernel_init(void *unused)
>         /* need to finish all async __init code before freeing the memory */
>         async_synchronize_full();
>         ftrace_free_init_mem();
> -       jump_label_invalidate_initmem();
>         free_initmem();
>         mark_readonly();
>         system_state = SYSTEM_RUNNING;
> diff --git a/kernel/jump_label.c b/kernel/jump_label.c
> index d424e1d22d63..7cdd49aeaf6a 100644
> --- a/kernel/jump_label.c
> +++ b/kernel/jump_label.c
> @@ -373,14 +373,15 @@ static enum jump_label_type jump_label_type(struct jump_entry *entry)
>
>  static void __jump_label_update(struct static_key *key,
>                                 struct jump_entry *entry,
> -                               struct jump_entry *stop)
> +                               struct jump_entry *stop,
> +                               bool init)
>  {
>         for (; (entry < stop) && (jump_entry_key(entry) == key); entry++) {
>                 /*
>                  * An entry->code of 0 indicates an entry which has been
>                  * disabled because it was in an init text area.
>                  */
> -               if (!jump_entry_is_init(entry)) {
> +               if (init || !jump_entry_is_init(entry)) {
>                         if (kernel_text_address(jump_entry_code(entry)))
>                                 arch_jump_label_transform(entry, jump_label_type(entry));
>                         else
> @@ -420,6 +421,9 @@ void __init jump_label_init(void)
>                 if (jump_label_type(iter) == JUMP_LABEL_NOP)
>                         arch_jump_label_transform_static(iter, JUMP_LABEL_NOP);
>
> +               if (init_section_contains((void *)jump_entry_code(iter), 1))
> +                       jump_entry_set_init(iter);
> +
>                 iterk = jump_entry_key(iter);
>                 if (iterk == key)
>                         continue;
> @@ -432,19 +436,6 @@ void __init jump_label_init(void)
>         cpus_read_unlock();
>  }
>
> -/* Disable any jump label entries in __init/__exit code */
> -void __init jump_label_invalidate_initmem(void)
> -{
> -       struct jump_entry *iter_start = __start___jump_table;
> -       struct jump_entry *iter_stop = __stop___jump_table;
> -       struct jump_entry *iter;
> -
> -       for (iter = iter_start; iter < iter_stop; iter++) {
> -               if (init_section_contains((void *)jump_entry_code(iter), 1))
> -                       jump_entry_set_init(iter);
> -       }
> -}
> -
>  #ifdef CONFIG_MODULES
>
>  static enum jump_label_type jump_label_init_type(struct jump_entry *entry)
> @@ -524,7 +515,8 @@ static void __jump_label_mod_update(struct static_key *key)
>                         stop = __stop___jump_table;
>                 else
>                         stop = m->jump_entries + m->num_jump_entries;
> -               __jump_label_update(key, mod->entries, stop);
> +               __jump_label_update(key, mod->entries, stop,
> +                                   m->state == MODULE_STATE_COMING);
>         }
>  }
>
> @@ -570,6 +562,9 @@ static int jump_label_add_module(struct module *mod)
>         for (iter = iter_start; iter < iter_stop; iter++) {
>                 struct static_key *iterk;
>
> +               if (within_module_init(jump_entry_code(iter), mod))
> +                       jump_entry_set_init(iter);
> +
>                 iterk = jump_entry_key(iter);
>                 if (iterk == key)
>                         continue;
> @@ -605,7 +600,7 @@ static int jump_label_add_module(struct module *mod)
>
>                 /* Only update if we've changed from our initial state */
>                 if (jump_label_type(iter) != jump_label_init_type(iter))
> -                       __jump_label_update(key, iter, iter_stop);
> +                       __jump_label_update(key, iter, iter_stop, true);
>         }
>
>         return 0;
> @@ -661,19 +656,6 @@ static void jump_label_del_module(struct module *mod)
>         }
>  }
>
> -/* Disable any jump label entries in module init code */
> -static void jump_label_invalidate_module_init(struct module *mod)
> -{
> -       struct jump_entry *iter_start = mod->jump_entries;
> -       struct jump_entry *iter_stop = iter_start + mod->num_jump_entries;
> -       struct jump_entry *iter;
> -
> -       for (iter = iter_start; iter < iter_stop; iter++) {
> -               if (within_module_init(jump_entry_code(iter), mod))
> -                       jump_entry_set_init(iter);
> -       }
> -}
> -
>  static int
>  jump_label_module_notify(struct notifier_block *self, unsigned long val,
>                          void *data)
> @@ -695,9 +677,6 @@ jump_label_module_notify(struct notifier_block *self, unsigned long val,
>         case MODULE_STATE_GOING:
>                 jump_label_del_module(mod);
>                 break;
> -       case MODULE_STATE_LIVE:
> -               jump_label_invalidate_module_init(mod);
> -               break;
>         }
>
>         jump_label_unlock();
> @@ -767,7 +746,8 @@ static void jump_label_update(struct static_key *key)
>         entry = static_key_entries(key);
>         /* if there are no users, entry can be NULL */
>         if (entry)
> -               __jump_label_update(key, entry, stop);
> +               __jump_label_update(key, entry, stop,
> +                                   system_state < SYSTEM_RUNNING);
>  }
>
>  #ifdef CONFIG_STATIC_KEYS_SELFTEST
> --
> 2.17.1
>



-- 
Kees Cook
Pixel Security



[Index of Archives]     [Linux Kernel]     [Kernel Newbies]     [x86 Platform Driver]     [Netdev]     [Linux Wireless]     [Netfilter]     [Bugtraq]     [Linux Filesystems]     [Yosemite Discussion]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]

  Powered by Linux