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
Subtract KASLR offset from the kernel addresses reported by kcov.
Tested on x86_64 and AArch64 (Hikey LeMaker).
Signed-off-by: Alexander Popov <[email protected]>
---
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
Introduce kaslr_offset() similarly to x86_64 for fixing kcov.
Signed-off-by: Alexander Popov <[email protected]>
---
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
On Sun, Dec 11, 2016 at 1:50 AM, Alexander Popov <[email protected]> wrote:
> Subtract KASLR offset from the kernel addresses reported by kcov.
> Tested on x86_64 and AArch64 (Hikey LeMaker).
>
> Signed-off-by: Alexander Popov <[email protected]>
> ---
> 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?
On 11.12.2016 12:32, Dmitry Vyukov wrote:
> On Sun, Dec 11, 2016 at 1:50 AM, Alexander Popov <[email protected]> wrote:
>> Subtract KASLR offset from the kernel addresses reported by kcov.
>> Tested on x86_64 and AArch64 (Hikey LeMaker).
>>
>> Signed-off-by: Alexander Popov <[email protected]>
>> ---
>> 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
On Sun, Dec 11, 2016 at 10:37 PM, Alexander Popov <[email protected]> wrote:
> On 11.12.2016 12:32, Dmitry Vyukov wrote:
>> On Sun, Dec 11, 2016 at 1:50 AM, Alexander Popov <[email protected]> wrote:
>>> Subtract KASLR offset from the kernel addresses reported by kcov.
>>> Tested on x86_64 and AArch64 (Hikey LeMaker).
>>>
>>> Signed-off-by: Alexander Popov <[email protected]>
>>> ---
>>> 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.
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 <[email protected]>
> ---
> 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 <[email protected]>
Will
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
> ---
> 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
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