[PATCH 0/2] Make kcov work properly with KASLR enabled

[PATCH 0/2] Make kcov work properly with KASLR enabled

[Alexander Popov]

If CONFIG_RANDOMIZE_BASE is enabled, kcov currently reports kernel addresses
including the random offset which breaks the coverage-guided fuzzing on x86_64 and
AArch64. Fix that by subtracting kaslr_offset() return value.

Alexander Popov (2):
  arm64: setup: introduce kaslr_offset()
  kcov: make kcov work properly with KASLR enabled

 arch/arm64/include/asm/setup.h      | 19 +++++++++++++++++++
 arch/arm64/include/uapi/asm/setup.h |  4 ++--
 arch/arm64/kernel/setup.c           |  8 ++++----
 kernel/kcov.c                       |  8 +++++++-
 4 files changed, 32 insertions(+), 7 deletions(-)
 create mode 100644 arch/arm64/include/asm/setup.h

-- 
2.7.4

[PATCH 1/2] arm64: setup: introduce kaslr_offset()

[Alexander Popov]

Introduce kaslr_offset() similarly to x86_64 for fixing kcov.

Signed-off-by: Alexander Popov <alex.popov@linux.com>
---
 arch/arm64/include/asm/setup.h      | 19 +++++++++++++++++++
 arch/arm64/include/uapi/asm/setup.h |  4 ++--
 arch/arm64/kernel/setup.c           |  8 ++++----
 3 files changed, 25 insertions(+), 6 deletions(-)
 create mode 100644 arch/arm64/include/asm/setup.h

diff --git a/arch/arm64/include/asm/setup.h b/arch/arm64/include/asm/setup.h
new file mode 100644
index 0000000..e7b59b9
--- /dev/null
+++ b/arch/arm64/include/asm/setup.h
@@ -0,0 +1,19 @@
+/*
+ * arch/arm64/include/asm/setup.h
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __ASM_SETUP_H
+#define __ASM_SETUP_H
+
+#include <uapi/asm/setup.h>
+
+static inline unsigned long kaslr_offset(void)
+{
+	return kimage_vaddr - KIMAGE_VADDR;
+}
+
+#endif
diff --git a/arch/arm64/include/uapi/asm/setup.h b/arch/arm64/include/uapi/asm/setup.h
index 9cf2e46..26631c8 100644
--- a/arch/arm64/include/uapi/asm/setup.h
+++ b/arch/arm64/include/uapi/asm/setup.h
@@ -16,8 +16,8 @@
  * You should have received a copy of the GNU General Public License
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
-#ifndef __ASM_SETUP_H
-#define __ASM_SETUP_H
+#ifndef _UAPI__ASM_SETUP_H
+#define _UAPI__ASM_SETUP_H
 
 #include <linux/types.h>
 
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index f534f49..11eefda5 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -329,11 +329,11 @@ subsys_initcall(topology_init);
 static int dump_kernel_offset(struct notifier_block *self, unsigned long v,
 			      void *p)
 {
-	u64 const kaslr_offset = kimage_vaddr - KIMAGE_VADDR;
+	const unsigned long offset = kaslr_offset();
 
-	if (IS_ENABLED(CONFIG_RANDOMIZE_BASE) && kaslr_offset > 0) {
-		pr_emerg("Kernel Offset: 0x%llx from 0x%lx\n",
-			 kaslr_offset, KIMAGE_VADDR);
+	if (IS_ENABLED(CONFIG_RANDOMIZE_BASE) && offset > 0) {
+		pr_emerg("Kernel Offset: 0x%lx from 0x%lx\n",
+			 offset, KIMAGE_VADDR);
 	} else {
 		pr_emerg("Kernel Offset: disabled\n");
 	}
-- 
2.7.4

[PATCH 2/2] kcov: make kcov work properly with KASLR enabled

[Alexander Popov]

Subtract KASLR offset from the kernel addresses reported by kcov.
Tested on x86_64 and AArch64 (Hikey LeMaker).

Signed-off-by: Alexander Popov <alex.popov@linux.com>
---
 kernel/kcov.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/kernel/kcov.c b/kernel/kcov.c
index 3cbb0c8..f8f3f4c 100644
--- a/kernel/kcov.c
+++ b/kernel/kcov.c
@@ -14,6 +14,7 @@
 #include <linux/debugfs.h>
 #include <linux/uaccess.h>
 #include <linux/kcov.h>
+#include <asm/setup.h>
 
 /*
  * kcov descriptor (one per opened debugfs file).
@@ -68,6 +69,11 @@ void notrace __sanitizer_cov_trace_pc(void)
 	if (mode == KCOV_MODE_TRACE) {
 		unsigned long *area;
 		unsigned long pos;
+		unsigned long ip = _RET_IP_;
+
+#ifdef CONFIG_RANDOMIZE_BASE
+		ip -= kaslr_offset();
+#endif
 
 		/*
 		 * There is some code that runs in interrupts but for which
@@ -81,7 +87,7 @@ void notrace __sanitizer_cov_trace_pc(void)
 		/* The first word is number of subsequent PCs. */
 		pos = READ_ONCE(area[0]) + 1;
 		if (likely(pos < t->kcov_size)) {
-			area[pos] = _RET_IP_;
+			area[pos] = ip;
 			WRITE_ONCE(area[0], pos);
 		}
 	}
-- 
2.7.4

Re: [PATCH 2/2] kcov: make kcov work properly with KASLR enabled

[Dmitry Vyukov]

On Sun, Dec 11, 2016 at 1:50 AM, Alexander Popov <alex.popov@linux.com> wrote:
> Subtract KASLR offset from the kernel addresses reported by kcov.
> Tested on x86_64 and AArch64 (Hikey LeMaker).
>
> Signed-off-by: Alexander Popov <alex.popov@linux.com>
> ---
>  kernel/kcov.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index 3cbb0c8..f8f3f4c 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -14,6 +14,7 @@
>  #include <linux/debugfs.h>
>  #include <linux/uaccess.h>
>  #include <linux/kcov.h>
> +#include <asm/setup.h>
>
>  /*
>   * kcov descriptor (one per opened debugfs file).
> @@ -68,6 +69,11 @@ void notrace __sanitizer_cov_trace_pc(void)
>         if (mode == KCOV_MODE_TRACE) {
>                 unsigned long *area;
>                 unsigned long pos;
> +               unsigned long ip = _RET_IP_;
> +
> +#ifdef CONFIG_RANDOMIZE_BASE
> +               ip -= kaslr_offset();
> +#endif
>
>                 /*
>                  * There is some code that runs in interrupts but for which
> @@ -81,7 +87,7 @@ void notrace __sanitizer_cov_trace_pc(void)
>                 /* The first word is number of subsequent PCs. */
>                 pos = READ_ONCE(area[0]) + 1;
>                 if (likely(pos < t->kcov_size)) {
> -                       area[pos] = _RET_IP_;
> +                       area[pos] = ip;
>                         WRITE_ONCE(area[0], pos);
>                 }
>         }
> --
> 2.7.4


Hi,

I think generally this is the right thing to do.

 There are 2 pending patches for kcov by +Quentin (hopefully in mm):
"kcov: add AFL-style tracing"
"kcov: size of arena is now given in bytes"
https://groups.google.com/forum/#!topic/syzkaller/gcqbIhKjGcY
https://groups.google.com/d/msg/syzkaller/gcqbIhKjGcY/KQFryjBKCAAJ

Your patch probably conflicts with them.
Should you base them on top of these patches, so that Andrew can merge
it without conflicts?

Re: [PATCH 2/2] kcov: make kcov work properly with KASLR enabled

[Alexander Popov]

On 11.12.2016 12:32, Dmitry Vyukov wrote:
> On Sun, Dec 11, 2016 at 1:50 AM, Alexander Popov <alex.popov@linux.com> wrote:
>> Subtract KASLR offset from the kernel addresses reported by kcov.
>> Tested on x86_64 and AArch64 (Hikey LeMaker).
>>
>> Signed-off-by: Alexander Popov <alex.popov@linux.com>
>> ---
>>  kernel/kcov.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> I think generally this is the right thing to do.
> 
>  There are 2 pending patches for kcov by +Quentin (hopefully in mm):
> "kcov: add AFL-style tracing"
> "kcov: size of arena is now given in bytes"
> https://groups.google.com/forum/#!topic/syzkaller/gcqbIhKjGcY
> https://groups.google.com/d/msg/syzkaller/gcqbIhKjGcY/KQFryjBKCAAJ
> 
> Your patch probably conflicts with them.
> Should you base them on top of these patches, so that Andrew can merge
> it without conflicts?

Excuse me, I can't find these patches in:
git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
git://git.kernel.org/pub/scm/linux/kernel/git/mhocko/mm.git
git://git.cmpxchg.org/linux-mmots.git

Could you point at the tree which I can rebase onto?
Should I cherry-pick Quentin's patches manually?

Best regards,
Alexander

Re: [PATCH 2/2] kcov: make kcov work properly with KASLR enabled

[Dmitry Vyukov]

On Sun, Dec 11, 2016 at 10:37 PM, Alexander Popov <alex.popov@linux.com> wrote:
> On 11.12.2016 12:32, Dmitry Vyukov wrote:
>> On Sun, Dec 11, 2016 at 1:50 AM, Alexander Popov <alex.popov@linux.com> wrote:
>>> Subtract KASLR offset from the kernel addresses reported by kcov.
>>> Tested on x86_64 and AArch64 (Hikey LeMaker).
>>>
>>> Signed-off-by: Alexander Popov <alex.popov@linux.com>
>>> ---
>>>  kernel/kcov.c | 8 +++++++-
>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> I think generally this is the right thing to do.
>>
>>  There are 2 pending patches for kcov by +Quentin (hopefully in mm):
>> "kcov: add AFL-style tracing"
>> "kcov: size of arena is now given in bytes"
>> https://groups.google.com/forum/#!topic/syzkaller/gcqbIhKjGcY
>> https://groups.google.com/d/msg/syzkaller/gcqbIhKjGcY/KQFryjBKCAAJ
>>
>> Your patch probably conflicts with them.
>> Should you base them on top of these patches, so that Andrew can merge
>> it without conflicts?
>
> Excuse me, I can't find these patches in:
> git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> git://git.kernel.org/pub/scm/linux/kernel/git/mhocko/mm.git
> git://git.cmpxchg.org/linux-mmots.git
>
> Could you point at the tree which I can rebase onto?
> Should I cherry-pick Quentin's patches manually?


Quentin, do you know destiny of your patches? They does not seem to be
in mm tree.

Re: [PATCH 1/2] arm64: setup: introduce kaslr_offset()

[Will Deacon]

On Sun, Dec 11, 2016 at 03:50:55AM +0300, Alexander Popov wrote:
> Introduce kaslr_offset() similarly to x86_64 for fixing kcov.
> 
> Signed-off-by: Alexander Popov <alex.popov@linux.com>
> ---
>  arch/arm64/include/asm/setup.h      | 19 +++++++++++++++++++
>  arch/arm64/include/uapi/asm/setup.h |  4 ++--
>  arch/arm64/kernel/setup.c           |  8 ++++----
>  3 files changed, 25 insertions(+), 6 deletions(-)
>  create mode 100644 arch/arm64/include/asm/setup.h
> 
> diff --git a/arch/arm64/include/asm/setup.h b/arch/arm64/include/asm/setup.h
> new file mode 100644
> index 0000000..e7b59b9
> --- /dev/null
> +++ b/arch/arm64/include/asm/setup.h
> @@ -0,0 +1,19 @@
> +/*
> + * arch/arm64/include/asm/setup.h
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef __ASM_SETUP_H
> +#define __ASM_SETUP_H
> +
> +#include <uapi/asm/setup.h>
> +
> +static inline unsigned long kaslr_offset(void)
> +{
> +	return kimage_vaddr - KIMAGE_VADDR;
> +}
> +
> +#endif

You could probably just stick this in asm/memory.h, since that's where
kimage_vaddr is declared and it would save adding a new header file.

> diff --git a/arch/arm64/include/uapi/asm/setup.h b/arch/arm64/include/uapi/asm/setup.h
> index 9cf2e46..26631c8 100644
> --- a/arch/arm64/include/uapi/asm/setup.h
> +++ b/arch/arm64/include/uapi/asm/setup.h
> @@ -16,8 +16,8 @@
>   * You should have received a copy of the GNU General Public License
>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
> -#ifndef __ASM_SETUP_H
> -#define __ASM_SETUP_H
> +#ifndef _UAPI__ASM_SETUP_H
> +#define _UAPI__ASM_SETUP_H
>  
>  #include <linux/types.h>

You can drop this hunk.

With those changes:

Acked-by: Will Deacon <will.deacon@arm.com>

Will

Re: [PATCH 1/2] arm64: setup: introduce kaslr_offset()

[Alexander Popov]

On 12.12.2016 14:29, Will Deacon wrote:
> On Sun, Dec 11, 2016 at 03:50:55AM +0300, Alexander Popov wrote:
>> Introduce kaslr_offset() similarly to x86_64 for fixing kcov.
>>
>> Signed-off-by: Alexander Popov <alex.popov@linux.com>
>> ---
>>  arch/arm64/include/asm/setup.h      | 19 +++++++++++++++++++
>>  arch/arm64/include/uapi/asm/setup.h |  4 ++--
>>  arch/arm64/kernel/setup.c           |  8 ++++----
>>  3 files changed, 25 insertions(+), 6 deletions(-)
>>  create mode 100644 arch/arm64/include/asm/setup.h
> 
> You could probably just stick this in asm/memory.h, since that's where
> kimage_vaddr is declared and it would save adding a new header file.

Thanks, Will. I'll do that.

--
Alexander

Re: [PATCH 1/2] arm64: setup: introduce kaslr_offset()

[Yury Norov]

On Sun, Dec 11, 2016 at 03:50:55AM +0300, Alexander Popov wrote:
> Introduce kaslr_offset() similarly to x86_64 for fixing kcov.
> 
> Signed-off-by: Alexander Popov <alex.popov@linux.com>
> ---
>  arch/arm64/include/asm/setup.h      | 19 +++++++++++++++++++
>  arch/arm64/include/uapi/asm/setup.h |  4 ++--
>  arch/arm64/kernel/setup.c           |  8 ++++----
>  3 files changed, 25 insertions(+), 6 deletions(-)
>  create mode 100644 arch/arm64/include/asm/setup.h
> 
> diff --git a/arch/arm64/include/asm/setup.h b/arch/arm64/include/asm/setup.h
> new file mode 100644
> index 0000000..e7b59b9
> --- /dev/null
> +++ b/arch/arm64/include/asm/setup.h
> @@ -0,0 +1,19 @@
> +/*
> + * arch/arm64/include/asm/setup.h
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef __ASM_SETUP_H
> +#define __ASM_SETUP_H
> +
> +#include <uapi/asm/setup.h>
> +
> +static inline unsigned long kaslr_offset(void)
> +{
> +	return kimage_vaddr - KIMAGE_VADDR;
> +}
> +
> +#endif

Hi Alexander,

I found today's linux-next master broken:
In file included from init/main.c:88:0:
./arch/arm64/include/asm/setup.h:14:100: error: redefinition of ‘kaslr_offset’
In file included from ./arch/arm64/include/asm/page.h:54:0,
   from ./include/linux/mm_types.h:16,
   from ./include/linux/sched.h:27,
   from ./arch/arm64/include/asm/compat.h:25,
   from ./arch/arm64/include/asm/stat.h:23,
   from ./include/linux/stat.h:5,
   from ./include/linux/module.h:10,
   from init/main.c:15:
/arch/arm64/include/asm/memory.h:168:100: note: previous definition of ‘kaslr_offset’ was here scripts/Makefile.build:293: recipe for target 'init/main.o' failed
make[1]: *** [init/main.o] Error 1

It looks like you declare kaslr_offset() twice - in this patch, and in 7ede8665f
(arm64: setup: introduce kaslr_offset()). 

Yury

Re: [PATCH 1/2] arm64: setup: introduce kaslr_offset()

[Alexander Popov]

On 22.12.2016 09:18, Yury Norov wrote:
> On Sun, Dec 11, 2016 at 03:50:55AM +0300, Alexander Popov wrote:
>> Introduce kaslr_offset() similarly to x86_64 for fixing kcov.

[...]

> Hi Alexander,
> 
> I found today's linux-next master broken:

[...]

> It looks like you declare kaslr_offset() twice - in this patch, and in 7ede8665f
> (arm64: setup: introduce kaslr_offset()). 

Hello Yury,

There was a race during applying this patch. So currently linux-next has 2 versions of it.

The first one is 1a339a14b1f2c7a0dfdd6db79eee1e55d3cec357, which is original.
The second one is 7ede8665f27cde7da69e8b2fbeaa1ed0664879c5, updated by Will Deacon and
applied to the mainline.

I'm sorry for that. The first one should be definitely dropped.

Best regards,
Alexander

Re: [PATCH 1/2] arm64: setup: introduce kaslr_offset()

[Jon Hunter]

Hi Will, Catalin,

On 22/12/16 12:51, Alexander Popov wrote:
> On 22.12.2016 09:18, Yury Norov wrote:
>> On Sun, Dec 11, 2016 at 03:50:55AM +0300, Alexander Popov wrote:
>>> Introduce kaslr_offset() similarly to x86_64 for fixing kcov.
> 
> [...]
> 
>> Hi Alexander,
>>
>> I found today's linux-next master broken:
> 
> [...]
> 
>> It looks like you declare kaslr_offset() twice - in this patch, and in 7ede8665f
>> (arm64: setup: introduce kaslr_offset()). 
> 
> Hello Yury,
> 
> There was a race during applying this patch. So currently linux-next has 2 versions of it.
> 
> The first one is 1a339a14b1f2c7a0dfdd6db79eee1e55d3cec357, which is original.
> The second one is 7ede8665f27cde7da69e8b2fbeaa1ed0664879c5, updated by Will Deacon and
> applied to the mainline.
> 
> I'm sorry for that. The first one should be definitely dropped.

Looks like this is still broken in today's -next.

Cheers
Jon

-- 
nvpublic

Re: [PATCH 1/2] arm64: setup: introduce kaslr_offset()

[Will Deacon]

On Tue, Jan 03, 2017 at 11:18:15AM +0000, Jon Hunter wrote:
> On 22/12/16 12:51, Alexander Popov wrote:
> > On 22.12.2016 09:18, Yury Norov wrote:
> >> On Sun, Dec 11, 2016 at 03:50:55AM +0300, Alexander Popov wrote:
> >>> Introduce kaslr_offset() similarly to x86_64 for fixing kcov.
> > 
> > [...]
> > 
> >> Hi Alexander,
> >>
> >> I found today's linux-next master broken:
> > 
> > [...]
> > 
> >> It looks like you declare kaslr_offset() twice - in this patch, and in 7ede8665f
> >> (arm64: setup: introduce kaslr_offset()). 
> > 
> > Hello Yury,
> > 
> > There was a race during applying this patch. So currently linux-next has 2 versions of it.
> > 
> > The first one is 1a339a14b1f2c7a0dfdd6db79eee1e55d3cec357, which is original.
> > The second one is 7ede8665f27cde7da69e8b2fbeaa1ed0664879c5, updated by Will Deacon and
> > applied to the mainline.
> > 
> > I'm sorry for that. The first one should be definitely dropped.
> 
> Looks like this is still broken in today's -next.

I think this is coming in via akpm's tree, so there's nothing we can do
about it from the arm64 side :/

Andrew -- please can you drop 1a339a14b1f2 ("arm64: setup: introduce
kaslr_offset()") from your -next branch? It's superceded by
7ede8665f27c ("arm64: setup: introduce kaslr_offset()"), which landed
in mainline for -rc1, so you'll need to pick that up if you want your
branch to build on its own.

Thanks,

Will

Re: [PATCH 2/2] kcov: make kcov work properly with KASLR enabled

[Quentin Casasnovas]

[-- Attachment #1: Type: text/plain, Size: 1837 bytes --]

On Mon, Dec 12, 2016 at 07:58:03AM +0100, Dmitry Vyukov wrote:
> On Sun, Dec 11, 2016 at 10:37 PM, Alexander Popov <alex.popov@linux.com> wrote:
> > On 11.12.2016 12:32, Dmitry Vyukov wrote:
> >> On Sun, Dec 11, 2016 at 1:50 AM, Alexander Popov <alex.popov@linux.com> wrote:
> >>> Subtract KASLR offset from the kernel addresses reported by kcov.
> >>> Tested on x86_64 and AArch64 (Hikey LeMaker).
> >>>
> >>> Signed-off-by: Alexander Popov <alex.popov@linux.com>
> >>> ---
> >>>  kernel/kcov.c | 8 +++++++-
> >>>  1 file changed, 7 insertions(+), 1 deletion(-)
> >>
> >> I think generally this is the right thing to do.
> >>
> >>  There are 2 pending patches for kcov by +Quentin (hopefully in mm):
> >> "kcov: add AFL-style tracing"
> >> "kcov: size of arena is now given in bytes"
> >> https://groups.google.com/forum/#!topic/syzkaller/gcqbIhKjGcY
> >> https://groups.google.com/d/msg/syzkaller/gcqbIhKjGcY/KQFryjBKCAAJ
> >>
> >> Your patch probably conflicts with them.
> >> Should you base them on top of these patches, so that Andrew can merge
> >> it without conflicts?
> >
> > Excuse me, I can't find these patches in:
> > git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> > git://git.kernel.org/pub/scm/linux/kernel/git/mhocko/mm.git
> > git://git.cmpxchg.org/linux-mmots.git
> >
> > Could you point at the tree which I can rebase onto?
> > Should I cherry-pick Quentin's patches manually?
> 
> 
> Quentin, do you know destiny of your patches? They does not seem to be
> in mm tree.

Huh since apologies, looks like I messed up my filters and completely
missed this thread.  I was going to ask where my patches landed and who
should take them...

I'm happy to re-send them rebased on the mm tree if that's where they're
going to land initially.

Thanks,
Quentin


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]