We give up old_addr hint from the coming patch module in cases when kernel
load base has been randomized (as in such case, the coming module has no
idea about the exact randomization offset).
We are currently too pessimistic, and give up immediately as soon as
CONFIG_RANDOMIZE_BASE is set; this doesn't however directly imply that the
load base has actually been randomized. There are config options that
disable kASLR (such as hibernation), user could have disabled kaslr on
kernel command-line, etc.
The loader propagates the information whether kernel has been randomized
through bootparams. This allows us to have the condition more accurate.
On top of that, it seems unnecessary to give up old_addr hints even if
randomization is active. The relocation offset can be computed as
difference between _text start and __START_KERNEL, and therefore old_addr
can be adjusted accordingly.
Signed-off-by: Jiri Kosina <[email protected]>
---
arch/x86/include/asm/livepatch.h | 4 ++++
arch/x86/kernel/livepatch.c | 5 +++++
kernel/livepatch/core.c | 5 +++--
3 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
index 2d29197..3c339c0 100644
--- a/arch/x86/include/asm/livepatch.h
+++ b/arch/x86/include/asm/livepatch.h
@@ -23,8 +23,12 @@
#include <linux/module.h>
#include <linux/ftrace.h>
+#include <asm/setup.h>
#ifdef CONFIG_LIVEPATCH
+
+extern unsigned long kgr_vmlinux_relocation_offset(void);
+
static inline int klp_check_compiler_support(void)
{
#ifndef CC_USING_FENTRY
diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
index ff3c3101d..7a171c1 100644
--- a/arch/x86/kernel/livepatch.c
+++ b/arch/x86/kernel/livepatch.c
@@ -88,3 +88,8 @@ int klp_write_module_reloc(struct module *mod, unsigned long type,
return ret;
}
+
+unsigned long kgr_vmlinux_relocation_offset(void)
+{
+ return (unsigned long)&_text - __START_KERNEL;
+}
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 284e269..5e85dde 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -234,8 +234,9 @@ static int klp_find_verify_func_addr(struct klp_object *obj,
int ret;
#if defined(CONFIG_RANDOMIZE_BASE)
- /* KASLR is enabled, disregard old_addr from user */
- func->old_addr = 0;
+ /* If KASLR has been enabled, adjust old_addr accordingly */
+ if (kaslr_enabled())
+ func->old_addr += kgr_vmlinux_relocation_offset();
#endif
if (!func->old_addr || klp_is_module(obj))
--
Jiri Kosina
SUSE Labs
We give up old_addr hint from the coming patch module in cases when kernel load
base has been randomized (as in such case, the coming module has no idea about
the exact randomization offset).
We are currently too pessimistic, and give up immediately as soon as
CONFIG_RANDOMIZE_BASE is set; this doesn't however directly imply that the
load base has actually been randomized. There are config options that
disable kASLR (such as hibernation), user could have disabled kaslr on
kernel command-line, etc.
The loader propagates the information whether kernel has been randomized
through bootparams. This allows us to have the condition more accurate.
On top of that, it seems unnecessary to give up old_addr hints even if
randomization is active. The relocation offset can be computed as
difference between _text start and __START_KERNEL, and therefore old_addr
can be adjusted accordingly.
Signed-off-by: Jiri Kosina <[email protected]>
---
v1 -> v2: I accidentally used kgr_ suffix (which we use in kGraft) instead
of klp_ in v1.
arch/x86/include/asm/livepatch.h | 4 ++++
arch/x86/kernel/livepatch.c | 5 +++++
kernel/livepatch/core.c | 5 +++--
3 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
index 2d29197..84a3247 100644
--- a/arch/x86/include/asm/livepatch.h
+++ b/arch/x86/include/asm/livepatch.h
@@ -23,8 +23,12 @@
#include <linux/module.h>
#include <linux/ftrace.h>
+#include <asm/setup.h>
#ifdef CONFIG_LIVEPATCH
+
+extern unsigned long klp_vmlinux_relocation_offset(void);
+
static inline int klp_check_compiler_support(void)
{
#ifndef CC_USING_FENTRY
diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
index ff3c3101d..6df7902 100644
--- a/arch/x86/kernel/livepatch.c
+++ b/arch/x86/kernel/livepatch.c
@@ -88,3 +88,8 @@ int klp_write_module_reloc(struct module *mod, unsigned long type,
return ret;
}
+
+unsigned long klp_vmlinux_relocation_offset(void)
+{
+ return (unsigned long)&_text - __START_KERNEL;
+}
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 284e269..ff4c35c 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -234,8 +234,9 @@ static int klp_find_verify_func_addr(struct klp_object *obj,
int ret;
#if defined(CONFIG_RANDOMIZE_BASE)
- /* KASLR is enabled, disregard old_addr from user */
- func->old_addr = 0;
+ /* If KASLR has been enabled, adjust old_addr accordingly */
+ if (kaslr_enabled())
+ func->old_addr += klp_vmlinux_relocation_offset();
#endif
if (!func->old_addr || klp_is_module(obj))
--
Jiri Kosina
SUSE Labs
On Fri, Apr 24, 2015 at 09:59:03PM +0200, Jiri Kosina wrote:
> We give up old_addr hint from the coming patch module in cases when kernel load
> base has been randomized (as in such case, the coming module has no idea about
> the exact randomization offset).
>
> We are currently too pessimistic, and give up immediately as soon as
> CONFIG_RANDOMIZE_BASE is set; this doesn't however directly imply that the
> load base has actually been randomized. There are config options that
> disable kASLR (such as hibernation), user could have disabled kaslr on
> kernel command-line, etc.
>
> The loader propagates the information whether kernel has been randomized
> through bootparams. This allows us to have the condition more accurate.
>
> On top of that, it seems unnecessary to give up old_addr hints even if
> randomization is active. The relocation offset can be computed as
> difference between _text start and __START_KERNEL, and therefore old_addr
> can be adjusted accordingly.
>
> Signed-off-by: Jiri Kosina <[email protected]>
> ---
>
> v1 -> v2: I accidentally used kgr_ suffix (which we use in kGraft) instead
> of klp_ in v1.
>
> arch/x86/include/asm/livepatch.h | 4 ++++
> arch/x86/kernel/livepatch.c | 5 +++++
> kernel/livepatch/core.c | 5 +++--
> 3 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
> index 2d29197..84a3247 100644
> --- a/arch/x86/include/asm/livepatch.h
> +++ b/arch/x86/include/asm/livepatch.h
> @@ -23,8 +23,12 @@
>
> #include <linux/module.h>
> #include <linux/ftrace.h>
> +#include <asm/setup.h>
>
> #ifdef CONFIG_LIVEPATCH
> +
> +extern unsigned long klp_vmlinux_relocation_offset(void);
> +
> static inline int klp_check_compiler_support(void)
> {
> #ifndef CC_USING_FENTRY
> diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
> index ff3c3101d..6df7902 100644
> --- a/arch/x86/kernel/livepatch.c
> +++ b/arch/x86/kernel/livepatch.c
> @@ -88,3 +88,8 @@ int klp_write_module_reloc(struct module *mod, unsigned long type,
>
> return ret;
> }
> +
> +unsigned long klp_vmlinux_relocation_offset(void)
> +{
> + return (unsigned long)&_text - __START_KERNEL;
> +}
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 284e269..ff4c35c 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -234,8 +234,9 @@ static int klp_find_verify_func_addr(struct klp_object *obj,
> int ret;
>
> #if defined(CONFIG_RANDOMIZE_BASE)
> - /* KASLR is enabled, disregard old_addr from user */
> - func->old_addr = 0;
> + /* If KASLR has been enabled, adjust old_addr accordingly */
> + if (kaslr_enabled())
> + func->old_addr += klp_vmlinux_relocation_offset();
The old_addr field is optional, where a value of 0 means "lookup the
address in kallsyms". So its value should only be adjusted if old_addr
is already non-zero. Otherwise the zero value will be replaced with a
bad address and it will mistakenly call klp_verify_vmlinux_symbol() with
the bad address instead of klp_find_object_symbol().
--
Josh
On Fri, Apr 24, 2015 at 09:59:03PM +0200, Jiri Kosina wrote:
> We give up old_addr hint from the coming patch module in cases when kernel load
> base has been randomized (as in such case, the coming module has no idea about
> the exact randomization offset).
>
> We are currently too pessimistic, and give up immediately as soon as
> CONFIG_RANDOMIZE_BASE is set; this doesn't however directly imply that the
> load base has actually been randomized. There are config options that
> disable kASLR (such as hibernation), user could have disabled kaslr on
> kernel command-line, etc.
>
> The loader propagates the information whether kernel has been randomized
> through bootparams. This allows us to have the condition more accurate.
>
> On top of that, it seems unnecessary to give up old_addr hints even if
> randomization is active. The relocation offset can be computed as
> difference between _text start and __START_KERNEL, and therefore old_addr
> can be adjusted accordingly.
>
> Signed-off-by: Jiri Kosina <[email protected]>
> ---
>
> v1 -> v2: I accidentally used kgr_ suffix (which we use in kGraft) instead
> of klp_ in v1.
>
> arch/x86/include/asm/livepatch.h | 4 ++++
> arch/x86/kernel/livepatch.c | 5 +++++
> kernel/livepatch/core.c | 5 +++--
> 3 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
> index 2d29197..84a3247 100644
> --- a/arch/x86/include/asm/livepatch.h
> +++ b/arch/x86/include/asm/livepatch.h
> @@ -23,8 +23,12 @@
>
> #include <linux/module.h>
> #include <linux/ftrace.h>
> +#include <asm/setup.h>
>
> #ifdef CONFIG_LIVEPATCH
> +
> +extern unsigned long klp_vmlinux_relocation_offset(void);
> +
> static inline int klp_check_compiler_support(void)
> {
> #ifndef CC_USING_FENTRY
> diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
> index ff3c3101d..6df7902 100644
> --- a/arch/x86/kernel/livepatch.c
> +++ b/arch/x86/kernel/livepatch.c
> @@ -88,3 +88,8 @@ int klp_write_module_reloc(struct module *mod, unsigned long type,
>
> return ret;
> }
> +
> +unsigned long klp_vmlinux_relocation_offset(void)
> +{
> + return (unsigned long)&_text - __START_KERNEL;
> +}
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 284e269..ff4c35c 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -234,8 +234,9 @@ static int klp_find_verify_func_addr(struct klp_object *obj,
> int ret;
>
> #if defined(CONFIG_RANDOMIZE_BASE)
> - /* KASLR is enabled, disregard old_addr from user */
> - func->old_addr = 0;
> + /* If KASLR has been enabled, adjust old_addr accordingly */
> + if (kaslr_enabled())
> + func->old_addr += klp_vmlinux_relocation_offset();
> #endif
Can we remove the #ifdef now? It would probably be better to have an
#ifdef in asm/setup.h for the kaslr_enabled() definition.
--
Josh
On 04/24/15 at 09:59P, Jiri Kosina wrote:
> We give up old_addr hint from the coming patch module in cases when kernel load
> base has been randomized (as in such case, the coming module has no idea about
> the exact randomization offset).
>
> We are currently too pessimistic, and give up immediately as soon as
> CONFIG_RANDOMIZE_BASE is set; this doesn't however directly imply that the
> load base has actually been randomized. There are config options that
> disable kASLR (such as hibernation), user could have disabled kaslr on
> kernel command-line, etc.
>
> The loader propagates the information whether kernel has been randomized
> through bootparams. This allows us to have the condition more accurate.
>
> On top of that, it seems unnecessary to give up old_addr hints even if
> randomization is active. The relocation offset can be computed as
> difference between _text start and __START_KERNEL, and therefore old_addr
> can be adjusted accordingly.
>
> Signed-off-by: Jiri Kosina <[email protected]>
> ---
>
> v1 -> v2: I accidentally used kgr_ suffix (which we use in kGraft) instead
> of klp_ in v1.
>
> arch/x86/include/asm/livepatch.h | 4 ++++
> arch/x86/kernel/livepatch.c | 5 +++++
> kernel/livepatch/core.c | 5 +++--
> 3 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
> index 2d29197..84a3247 100644
> --- a/arch/x86/include/asm/livepatch.h
> +++ b/arch/x86/include/asm/livepatch.h
> @@ -23,8 +23,12 @@
>
> #include <linux/module.h>
> #include <linux/ftrace.h>
> +#include <asm/setup.h>
>
> #ifdef CONFIG_LIVEPATCH
> +
> +extern unsigned long klp_vmlinux_relocation_offset(void);
> +
> static inline int klp_check_compiler_support(void)
> {
> #ifndef CC_USING_FENTRY
> diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
> index ff3c3101d..6df7902 100644
> --- a/arch/x86/kernel/livepatch.c
> +++ b/arch/x86/kernel/livepatch.c
> @@ -88,3 +88,8 @@ int klp_write_module_reloc(struct module *mod, unsigned long type,
>
> return ret;
> }
> +
> +unsigned long klp_vmlinux_relocation_offset(void)
> +{
> + return (unsigned long)&_text - __START_KERNEL;
> +}
Is it possible to put above function into arch/x86/include/asm/setup.h?
It is more elegant, so that the function is re-used by other module.
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 284e269..ff4c35c 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -234,8 +234,9 @@ static int klp_find_verify_func_addr(struct klp_object *obj,
> int ret;
>
> #if defined(CONFIG_RANDOMIZE_BASE)
> - /* KASLR is enabled, disregard old_addr from user */
> - func->old_addr = 0;
> + /* If KASLR has been enabled, adjust old_addr accordingly */
> + if (kaslr_enabled())
> + func->old_addr += klp_vmlinux_relocation_offset();
Since KASLR only works for x86 arch now, it is better to put it into the
specfied arch (x86 now), or implement a weak function to let be overwritten
by specified arch.
Thanks
Minfei
> #endif
>
> if (!func->old_addr || klp_is_module(obj))
>
> --
> Jiri Kosina
> SUSE Labs
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 24 Apr 2015, Josh Poimboeuf wrote:
> > #if defined(CONFIG_RANDOMIZE_BASE)
> > - /* KASLR is enabled, disregard old_addr from user */
> > - func->old_addr = 0;
> > + /* If KASLR has been enabled, adjust old_addr accordingly */
> > + if (kaslr_enabled())
> > + func->old_addr += klp_vmlinux_relocation_offset();
>
> The old_addr field is optional, where a value of 0 means "lookup the
> address in kallsyms". So its value should only be adjusted if old_addr
> is already non-zero. Otherwise the zero value will be replaced with a
> bad address and it will mistakenly call klp_verify_vmlinux_symbol() with
> the bad address instead of klp_find_object_symbol().
Fair enough, good point, will take this into account in v3. Thanks,
--
Jiri Kosina
SUSE Labs
On Fri, 24 Apr 2015, Josh Poimboeuf wrote:
> > #if defined(CONFIG_RANDOMIZE_BASE)
> > - /* KASLR is enabled, disregard old_addr from user */
> > - func->old_addr = 0;
> > + /* If KASLR has been enabled, adjust old_addr accordingly */
> > + if (kaslr_enabled())
> > + func->old_addr += klp_vmlinux_relocation_offset();
> > #endif
>
> Can we remove the #ifdef now? It would probably be better to have an
> #ifdef in asm/setup.h for the kaslr_enabled() definition.
If we do that, we force all architectures that support (now, or at any
point in the future) live patching to provide kaslr_enabled(),
klp_vmlinux_relocation_offset() (or however it will be called), etc (even
if they don't care a bit about kASLR) I would like to avoid that; it seems
really unnecessary maintainance overhead imposed on archs.
Thanks,
--
Jiri Kosina
SUSE Labs
On Sat, 25 Apr 2015, Minfei Huang wrote:
> > diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
> > index 2d29197..84a3247 100644
> > --- a/arch/x86/include/asm/livepatch.h
> > +++ b/arch/x86/include/asm/livepatch.h
> > @@ -23,8 +23,12 @@
> >
> > #include <linux/module.h>
> > #include <linux/ftrace.h>
> > +#include <asm/setup.h>
> >
> > #ifdef CONFIG_LIVEPATCH
> > +
> > +extern unsigned long klp_vmlinux_relocation_offset(void);
> > +
> > static inline int klp_check_compiler_support(void)
> > {
> > #ifndef CC_USING_FENTRY
> > diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
> > index ff3c3101d..6df7902 100644
> > --- a/arch/x86/kernel/livepatch.c
> > +++ b/arch/x86/kernel/livepatch.c
> > @@ -88,3 +88,8 @@ int klp_write_module_reloc(struct module *mod, unsigned long type,
> >
> > return ret;
> > }
> > +
> > +unsigned long klp_vmlinux_relocation_offset(void)
> > +{
> > + return (unsigned long)&_text - __START_KERNEL;
> > +}
>
> Is it possible to put above function into arch/x86/include/asm/setup.h?
> It is more elegant, so that the function is re-used by other module.
Yup, makes sense. I will take that into account in v3. At least x86 module
loader can already immediately make use of that.
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index 284e269..ff4c35c 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -234,8 +234,9 @@ static int klp_find_verify_func_addr(struct klp_object *obj,
> > int ret;
> >
> > #if defined(CONFIG_RANDOMIZE_BASE)
> > - /* KASLR is enabled, disregard old_addr from user */
> > - func->old_addr = 0;
> > + /* If KASLR has been enabled, adjust old_addr accordingly */
> > + if (kaslr_enabled())
> > + func->old_addr += klp_vmlinux_relocation_offset();
>
> Since KASLR only works for x86 arch now, it is better to put it into the
> specfied arch (x86 now), or implement a weak function to let be overwritten
> by specified arch.
I responded to Josh on this matter already.
Thanks,
--
Jiri Kosina
SUSE Labs