[PATCH] kcov: reject open when kernel not instrumented

[PATCH] kcov: reject open when kernel not instrumented

[Mark Rutland]

If the toolchain does not support -fsanitize-coverage=trace-pc, we blat
this option from CFLAGS_KCOV, and build the kernel without
instrumentation, even if CONFIG_KCOV was selected. However, we still
build the rest of the kcov infrastructure, and expose a kcov file under
debugfs. This can be confusing, as the kernel will appear to support
kcov, yet will never manage to sample any trace PC values. While we do
note this fact at build time, this may be missed, and a user may not
have access to build logs.

This patch adds an artificial CONFIG symbol, CONFIG_KCOV_CC, that is
only set when the toolchain supports -fsanitize-coverage=trace-pc, and
hence the kernel is built with instrumentation. When this is not the
case, the kernel will return -ENOTSUPP if userspace attempts to open the
kcov debugfs file, indicating that kcov functionality is unavailable.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Potapenko <glider@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: James Morse <james.morse@arm.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Michal Marek <mmarek@suse.com>
Cc: linux-kernel@vger.kernel.org
---
 Makefile      | 2 +-
 kernel/kcov.c | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

As discussed [1] in reply to the arm64 patch.

[1] http://lkml.kernel.org/r/CACT4Y+Z3=juvxBJBXRh5PgE35twFRxg-3iMc-owenONU84x5XQ@mail.gmail.com

Mark.

diff --git a/Makefile b/Makefile
index 0f70de6..e6ef260 100644
--- a/Makefile
+++ b/Makefile
@@ -369,7 +369,7 @@ LDFLAGS_MODULE  =
 CFLAGS_KERNEL	=
 AFLAGS_KERNEL	=
 CFLAGS_GCOV	= -fprofile-arcs -ftest-coverage -fno-tree-loop-im -Wno-maybe-uninitialized
-CFLAGS_KCOV	= -fsanitize-coverage=trace-pc
+CFLAGS_KCOV	= -fsanitize-coverage=trace-pc -DCONFIG_KCOV_CC
 
 
 # Use USERINCLUDE when you must reference the UAPI directories only.
diff --git a/kernel/kcov.c b/kernel/kcov.c
index a02f2dd..df2cafd 100644
--- a/kernel/kcov.c
+++ b/kernel/kcov.c
@@ -3,6 +3,7 @@
 #define DISABLE_BRANCH_PROFILING
 #include <linux/compiler.h>
 #include <linux/types.h>
+#include <linux/errno.h>
 #include <linux/file.h>
 #include <linux/fs.h>
 #include <linux/mm.h>
@@ -160,6 +161,13 @@ static int kcov_open(struct inode *inode, struct file *filep)
 {
 	struct kcov *kcov;
 
+	/*
+	 * CONFIG_KCOV was selected, but the compiler does not support the
+	 * options KCOV requires.
+	 */
+	if (!IS_ENABLED(CONFIG_KCOV_CC))
+		return -ENOTSUPP;
+
 	kcov = kzalloc(sizeof(*kcov), GFP_KERNEL);
 	if (!kcov)
 		return -ENOMEM;
-- 
1.9.1

Re: [PATCH] kcov: reject open when kernel not instrumented

[Dmitry Vyukov]

On Wed, Jun 15, 2016 at 5:49 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> If the toolchain does not support -fsanitize-coverage=trace-pc, we blat
> this option from CFLAGS_KCOV, and build the kernel without
> instrumentation, even if CONFIG_KCOV was selected. However, we still
> build the rest of the kcov infrastructure, and expose a kcov file under
> debugfs. This can be confusing, as the kernel will appear to support
> kcov, yet will never manage to sample any trace PC values. While we do
> note this fact at build time, this may be missed, and a user may not
> have access to build logs.
>
> This patch adds an artificial CONFIG symbol, CONFIG_KCOV_CC, that is
> only set when the toolchain supports -fsanitize-coverage=trace-pc, and
> hence the kernel is built with instrumentation. When this is not the
> case, the kernel will return -ENOTSUPP if userspace attempts to open the
> kcov debugfs file, indicating that kcov functionality is unavailable.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Alexander Potapenko <glider@google.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: James Morse <james.morse@arm.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Michal Marek <mmarek@suse.com>
> Cc: linux-kernel@vger.kernel.org
> ---
>  Makefile      | 2 +-
>  kernel/kcov.c | 8 ++++++++
>  2 files changed, 9 insertions(+), 1 deletion(-)
>
> As discussed [1] in reply to the arm64 patch.
>
> [1] http://lkml.kernel.org/r/CACT4Y+Z3=juvxBJBXRh5PgE35twFRxg-3iMc-owenONU84x5XQ@mail.gmail.com
>
> Mark.
>
> diff --git a/Makefile b/Makefile
> index 0f70de6..e6ef260 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -369,7 +369,7 @@ LDFLAGS_MODULE  =
>  CFLAGS_KERNEL  =
>  AFLAGS_KERNEL  =
>  CFLAGS_GCOV    = -fprofile-arcs -ftest-coverage -fno-tree-loop-im -Wno-maybe-uninitialized
> -CFLAGS_KCOV    = -fsanitize-coverage=trace-pc
> +CFLAGS_KCOV    = -fsanitize-coverage=trace-pc -DCONFIG_KCOV_CC
>
>
>  # Use USERINCLUDE when you must reference the UAPI directories only.
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index a02f2dd..df2cafd 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -3,6 +3,7 @@
>  #define DISABLE_BRANCH_PROFILING
>  #include <linux/compiler.h>
>  #include <linux/types.h>
> +#include <linux/errno.h>
>  #include <linux/file.h>
>  #include <linux/fs.h>
>  #include <linux/mm.h>
> @@ -160,6 +161,13 @@ static int kcov_open(struct inode *inode, struct file *filep)
>  {
>         struct kcov *kcov;
>
> +       /*
> +        * CONFIG_KCOV was selected, but the compiler does not support the
> +        * options KCOV requires.
> +        */
> +       if (!IS_ENABLED(CONFIG_KCOV_CC))
> +               return -ENOTSUPP;
> +
>         kcov = kzalloc(sizeof(*kcov), GFP_KERNEL);
>         if (!kcov)
>                 return -ENOMEM;


Reviewed-by: Dmitry Vyukov <dvyukov@google.com>

Thanks!

Re: [PATCH] kcov: reject open when kernel not instrumented

[Michal Marek]

Dne 15.6.2016 v 17:49 Mark Rutland napsal(a):
> If the toolchain does not support -fsanitize-coverage=trace-pc, we blat
> this option from CFLAGS_KCOV, and build the kernel without
> instrumentation, even if CONFIG_KCOV was selected. However, we still
> build the rest of the kcov infrastructure, and expose a kcov file under
> debugfs. This can be confusing, as the kernel will appear to support
> kcov, yet will never manage to sample any trace PC values. While we do
> note this fact at build time, this may be missed, and a user may not
> have access to build logs.
> 
> This patch adds an artificial CONFIG symbol, CONFIG_KCOV_CC, that is
> only set when the toolchain supports -fsanitize-coverage=trace-pc, and
> hence the kernel is built with instrumentation. When this is not the
> case, the kernel will return -ENOTSUPP if userspace attempts to open the
> kcov debugfs file, indicating that kcov functionality is unavailable.

Hi Mark,

please use a define outside the CONFIG_ namespace, because it is not a
config option that one can find in .config or a Kconfig file. We already
have CC_HAVE_ASM_GOTO, CC_USING_FENTRY and similar in the kernel.

Thanks,
Michal

Re: [PATCH] kcov: reject open when kernel not instrumented

[Mark Rutland]

On Wed, Jun 15, 2016 at 06:21:03PM +0200, Michal Marek wrote:
> Dne 15.6.2016 v 17:49 Mark Rutland napsal(a):
> > If the toolchain does not support -fsanitize-coverage=trace-pc, we blat
> > this option from CFLAGS_KCOV, and build the kernel without
> > instrumentation, even if CONFIG_KCOV was selected. However, we still
> > build the rest of the kcov infrastructure, and expose a kcov file under
> > debugfs. This can be confusing, as the kernel will appear to support
> > kcov, yet will never manage to sample any trace PC values. While we do
> > note this fact at build time, this may be missed, and a user may not
> > have access to build logs.
> > 
> > This patch adds an artificial CONFIG symbol, CONFIG_KCOV_CC, that is
> > only set when the toolchain supports -fsanitize-coverage=trace-pc, and
> > hence the kernel is built with instrumentation. When this is not the
> > case, the kernel will return -ENOTSUPP if userspace attempts to open the
> > kcov debugfs file, indicating that kcov functionality is unavailable.
> 
> Hi Mark,
> 
> please use a define outside the CONFIG_ namespace, because it is not a
> config option that one can find in .config or a Kconfig file. We already
> have CC_HAVE_ASM_GOTO, CC_USING_FENTRY and similar in the kernel.

Sure thing. I guess I should also not use IS_ENABLED, and will fall back
to the usual ifdefferty.

Dmitry, Alexander, any preference for the name? Is
CC_HAVE_SANCOV_TRACE_PC ok?

I guess we should also clean up arm64's current use of CONFIG_AS_LSE
(which I guess should be something like AS_HAVE_LSE per the
CC_HAVE_ASM_GOTO example). I can cook up a patch for that too.

Thanks,
Mark.

Re: [PATCH] kcov: reject open when kernel not instrumented

[Dmitry Vyukov]

On Wed, Jun 15, 2016 at 6:34 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Jun 15, 2016 at 06:21:03PM +0200, Michal Marek wrote:
>> Dne 15.6.2016 v 17:49 Mark Rutland napsal(a):
>> > If the toolchain does not support -fsanitize-coverage=trace-pc, we blat
>> > this option from CFLAGS_KCOV, and build the kernel without
>> > instrumentation, even if CONFIG_KCOV was selected. However, we still
>> > build the rest of the kcov infrastructure, and expose a kcov file under
>> > debugfs. This can be confusing, as the kernel will appear to support
>> > kcov, yet will never manage to sample any trace PC values. While we do
>> > note this fact at build time, this may be missed, and a user may not
>> > have access to build logs.
>> >
>> > This patch adds an artificial CONFIG symbol, CONFIG_KCOV_CC, that is
>> > only set when the toolchain supports -fsanitize-coverage=trace-pc, and
>> > hence the kernel is built with instrumentation. When this is not the
>> > case, the kernel will return -ENOTSUPP if userspace attempts to open the
>> > kcov debugfs file, indicating that kcov functionality is unavailable.
>>
>> Hi Mark,
>>
>> please use a define outside the CONFIG_ namespace, because it is not a
>> config option that one can find in .config or a Kconfig file. We already
>> have CC_HAVE_ASM_GOTO, CC_USING_FENTRY and similar in the kernel.
>
> Sure thing. I guess I should also not use IS_ENABLED, and will fall back
> to the usual ifdefferty.
>
> Dmitry, Alexander, any preference for the name? Is
> CC_HAVE_SANCOV_TRACE_PC ok?

Fine with me.

> I guess we should also clean up arm64's current use of CONFIG_AS_LSE
> (which I guess should be something like AS_HAVE_LSE per the
> CC_HAVE_ASM_GOTO example). I can cook up a patch for that too.
>
> Thanks,
> Mark.