[PATCH] livepatch: x86: make kASLR logic more accurate

[PATCH] livepatch: x86: make kASLR logic more accurate

[Jiri Kosina]

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 <jkosina@suse.cz>
---
 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

[PATCH v2] livepatch: x86: make kASLR logic more accurate

[Jiri Kosina]

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 <jkosina@suse.cz>
---

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

Re: [PATCH v2] livepatch: x86: make kASLR logic more accurate

[Josh Poimboeuf]

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 <jkosina@suse.cz>
> ---
> 
> 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

Re: [PATCH v2] livepatch: x86: make kASLR logic more accurate

[Josh Poimboeuf]

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 <jkosina@suse.cz>
> ---
> 
> 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

Re: [PATCH v2] livepatch: x86: make kASLR logic more accurate

[Minfei Huang]

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 <jkosina@suse.cz>
> ---
> 
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] livepatch: x86: make kASLR logic more accurate

[Jiri Kosina]

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

Re: [PATCH v2] livepatch: x86: make kASLR logic more accurate

[Jiri Kosina]

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

Re: [PATCH v2] livepatch: x86: make kASLR logic more accurate

[Jiri Kosina]

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