[PATCH v2] livepatch: x86: bugfix about kASLR

[PATCH v2] livepatch: x86: bugfix about kASLR

[Zhou Chengming]

When enable KASLR, livepatch will adjust old_addr of changed
function accordingly. So do the same thing for reloc.

[PATCH v1] https://lkml.org/lkml/2015/11/4/91

Reported-by: Cyril B. <cbay@alwaysdata.com>
Signed-off-by: Zhou Chengming <zhouchengming1@huawei.com>
---
 kernel/livepatch/core.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 6e53441..db545cb 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -294,6 +294,12 @@ static int klp_write_object_relocations(struct module *pmod,
 
 	for (reloc = obj->relocs; reloc->name; reloc++) {
 		if (!klp_is_module(obj)) {
+
+#if defined(CONFIG_RANDOMIZE_BASE)
+			/* If KASLR has been enabled, adjust old value accordingly */
+			if (kaslr_enabled())
+				reloc->val += kaslr_offset();
+#endif
 			ret = klp_verify_vmlinux_symbol(reloc->name,
 							reloc->val);
 			if (ret)
-- 
1.7.7

Re: [PATCH v2] livepatch: x86: bugfix about kASLR

[Josh Poimboeuf]

On Fri, Nov 06, 2015 at 02:25:00PM +0800, Zhou Chengming wrote:
> When enable KASLR, livepatch will adjust old_addr of changed
> function accordingly. So do the same thing for reloc.
> 
> [PATCH v1] https://lkml.org/lkml/2015/11/4/91
> 
> Reported-by: Cyril B. <cbay@alwaysdata.com>
> Signed-off-by: Zhou Chengming <zhouchengming1@huawei.com>
> ---
>  kernel/livepatch/core.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 6e53441..db545cb 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -294,6 +294,12 @@ static int klp_write_object_relocations(struct module *pmod,
>  
>  	for (reloc = obj->relocs; reloc->name; reloc++) {
>  		if (!klp_is_module(obj)) {
> +
> +#if defined(CONFIG_RANDOMIZE_BASE)
> +			/* If KASLR has been enabled, adjust old value accordingly */
> +			if (kaslr_enabled())
> +				reloc->val += kaslr_offset();
> +#endif
>  			ret = klp_verify_vmlinux_symbol(reloc->name,
>  							reloc->val);
>  			if (ret)

Zhou, thanks a lot for this fix.

Generally I think this patch is fine.  However, Chris J Arges is working
on another patch[*] which may get rid of reloc->val as an input and make
this patch obsolete.

So, assuming Chris's patch eventually gets accepted, I don't see a need
for this one unless anybody wants it as a bug fix for 4.4.

[*] https://lkml.kernel.org/r/1447085770-11729-1-git-send-email-chris.j.arges@canonical.com

-- 
Josh

Re: [PATCH v2] livepatch: x86: bugfix about kASLR

[Minfei Huang]

On 11/10/15 at 08:07am, Josh Poimboeuf wrote:
> On Fri, Nov 06, 2015 at 02:25:00PM +0800, Zhou Chengming wrote:
> > When enable KASLR, livepatch will adjust old_addr of changed
> > function accordingly. So do the same thing for reloc.
> > 
> > +
> > +#if defined(CONFIG_RANDOMIZE_BASE)
> > +			/* If KASLR has been enabled, adjust old value accordingly */
> > +			if (kaslr_enabled())
> > +				reloc->val += kaslr_offset();
> > +#endif
> >  			ret = klp_verify_vmlinux_symbol(reloc->name,
> >  							reloc->val);
> >  			if (ret)
> 
> Zhou, thanks a lot for this fix.
> 
> Generally I think this patch is fine.  However, Chris J Arges is working
> on another patch[*] which may get rid of reloc->val as an input and make
> this patch obsolete.
> 
> So, assuming Chris's patch eventually gets accepted, I don't see a need
> for this one unless anybody wants it as a bug fix for 4.4.
> 
> [*] https://lkml.kernel.org/r/1447085770-11729-1-git-send-email-chris.j.arges@canonical.com
> 

Hi, Josh.

I think Jessica Yu is working on the relative patchset which will
offload relocation logical to the module loader.

Thanks
Minfei

Re: [PATCH v2] livepatch: x86: bugfix about kASLR

[Josh Poimboeuf]

On Wed, Nov 11, 2015 at 04:46:48PM +0800, Minfei Huang wrote:
> On 11/10/15 at 08:07am, Josh Poimboeuf wrote:
> > On Fri, Nov 06, 2015 at 02:25:00PM +0800, Zhou Chengming wrote:
> > > When enable KASLR, livepatch will adjust old_addr of changed
> > > function accordingly. So do the same thing for reloc.
> > > 
> > > +
> > > +#if defined(CONFIG_RANDOMIZE_BASE)
> > > +			/* If KASLR has been enabled, adjust old value accordingly */
> > > +			if (kaslr_enabled())
> > > +				reloc->val += kaslr_offset();
> > > +#endif
> > >  			ret = klp_verify_vmlinux_symbol(reloc->name,
> > >  							reloc->val);
> > >  			if (ret)
> > 
> > Zhou, thanks a lot for this fix.
> > 
> > Generally I think this patch is fine.  However, Chris J Arges is working
> > on another patch[*] which may get rid of reloc->val as an input and make
> > this patch obsolete.
> > 
> > So, assuming Chris's patch eventually gets accepted, I don't see a need
> > for this one unless anybody wants it as a bug fix for 4.4.
> > 
> > [*] https://lkml.kernel.org/r/1447085770-11729-1-git-send-email-chris.j.arges@canonical.com
> > 
> 
> Hi, Josh.
> 
> I think Jessica Yu is working on the relative patchset which will
> offload relocation logical to the module loader.

Yeah, Jessica's patch set does move the relocation logic to the module
loader, and it does result in a rewrite of this code.  However, it still
uses an address-based symbol addressing scheme, so it doesn't resolve
this particular issue.

Chris's patch changes the symbol addressing scheme from address-based to
sympos-based.  To be consistent, I think it should do that for both
function addresses and relocation symbol addresses.  Then that would fix
this issue and make Zhou's patch obsolete.

If Chris's smaller patch is merged before Jessica's bigger patch set,
Jessica's patches can be rebased on top of Chris's to keep the new
sympos-based addressing scheme.

Or vice versa: if Jessica's patches are merged first, then Chris's can
be rebased.  Either way, Chris's patches will obsolete this one.

-- 
Josh

Re: [PATCH v2] livepatch: x86: bugfix about kASLR

[Jiri Kosina]

On Wed, 11 Nov 2015, Josh Poimboeuf wrote:

> If Chris's smaller patch is merged before Jessica's bigger patch set,
> Jessica's patches can be rebased on top of Chris's to keep the new
> sympos-based addressing scheme.
> 
> Or vice versa: if Jessica's patches are merged first, then Chris's can
> be rebased.  Either way, Chris's patches will obsolete this one.

My current plan is to take Zhou's simple kASLR offset fix for 4.4 still 
(it's my fault that I overlooked this when putting kASLR bits in place 
anyway), and we'll see what ends up being merged for 4.5+.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2] livepatch: x86: bugfix about kASLR

[Josh Poimboeuf]

On Wed, Nov 11, 2015 at 05:19:58PM +0100, Jiri Kosina wrote:
> On Wed, 11 Nov 2015, Josh Poimboeuf wrote:
> 
> > If Chris's smaller patch is merged before Jessica's bigger patch set,
> > Jessica's patches can be rebased on top of Chris's to keep the new
> > sympos-based addressing scheme.
> > 
> > Or vice versa: if Jessica's patches are merged first, then Chris's can
> > be rebased.  Either way, Chris's patches will obsolete this one.
> 
> My current plan is to take Zhou's simple kASLR offset fix for 4.4 still 
> (it's my fault that I overlooked this when putting kASLR bits in place 
> anyway), and we'll see what ends up being merged for 4.5+.

Ok, sounds fine to me.

-- 
Josh

Re: [PATCH v2] livepatch: x86: bugfix about kASLR

[Josh Poimboeuf]

On Fri, Nov 06, 2015 at 02:25:00PM +0800, Zhou Chengming wrote:
> When enable KASLR, livepatch will adjust old_addr of changed
> function accordingly. So do the same thing for reloc.
> 
> [PATCH v1] https://lkml.org/lkml/2015/11/4/91
> 
> Reported-by: Cyril B. <cbay@alwaysdata.com>
> Signed-off-by: Zhou Chengming <zhouchengming1@huawei.com>
> ---
>  kernel/livepatch/core.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 6e53441..db545cb 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -294,6 +294,12 @@ static int klp_write_object_relocations(struct module *pmod,
>  
>  	for (reloc = obj->relocs; reloc->name; reloc++) {
>  		if (!klp_is_module(obj)) {
> +
> +#if defined(CONFIG_RANDOMIZE_BASE)
> +			/* If KASLR has been enabled, adjust old value accordingly */
> +			if (kaslr_enabled())
> +				reloc->val += kaslr_offset();
> +#endif
>  			ret = klp_verify_vmlinux_symbol(reloc->name,
>  							reloc->val);
>  			if (ret)

Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>

Re: [PATCH v2] livepatch: x86: bugfix about kASLR

[Jiri Kosina]

On Fri, 6 Nov 2015, Zhou Chengming wrote:

> When enable KASLR, livepatch will adjust old_addr of changed
> function accordingly. So do the same thing for reloc.
> 
> [PATCH v1] https://lkml.org/lkml/2015/11/4/91
> 
> Reported-by: Cyril B. <cbay@alwaysdata.com>
> Signed-off-by: Zhou Chengming <zhouchengming1@huawei.com>

I have made the changelog and patch title a little bit more verbose and 
applied to for-4.4/upstream-fixes branch.

Thanks,

-- 
Jiri Kosina
SUSE Labs