[PATCHv2] kcov: reject open when kernel not instrumented

[PATCHv2] 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 ensures that CC_HAVE_SANCOV_TRACE_PC is defined when the
toolchain supports -fsanitize-coverage=trace-pc, and is not defined
otherwise. When CC_HAVE_SANCOV_TRACE_PC is not defined, 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 | 9 +++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

Since v1 [1]:
* Use CC_HAVE_SANCOV_TRACE_PC rather than CONFIG_KCOV_CC

[1] lkml.kernel.org/r/1466005756-15626-1-git-send-email-mark.rutland@arm.com

diff --git a/Makefile b/Makefile
index 0f70de6..3785a63 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 -DCC_HAVE_SANCOV_TRACE_PC
 
 
 # Use USERINCLUDE when you must reference the UAPI directories only.
diff --git a/kernel/kcov.c b/kernel/kcov.c
index a02f2dd..0a0b164 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,14 @@ static int kcov_open(struct inode *inode, struct file *filep)
 {
 	struct kcov *kcov;
 
+#ifndef CC_HAVE_SANCOV_TRACE_PC
+	/*
+	 * CONFIG_KCOV was selected, but the compiler does not support the
+	 * options KCOV requires.
+	 */
+	return -ENOTSUPP;
+#endif /* CC_HAVE_SANCOV_TRACE_PC */
+
 	kcov = kzalloc(sizeof(*kcov), GFP_KERNEL);
 	if (!kcov)
 		return -ENOMEM;
-- 
1.9.1

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

[Dmitry Vyukov]

On Wed, Jun 15, 2016 at 7:04 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 ensures that CC_HAVE_SANCOV_TRACE_PC is defined when the
> toolchain supports -fsanitize-coverage=trace-pc, and is not defined
> otherwise. When CC_HAVE_SANCOV_TRACE_PC is not defined, 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 | 9 +++++++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> Since v1 [1]:
> * Use CC_HAVE_SANCOV_TRACE_PC rather than CONFIG_KCOV_CC
>
> [1] lkml.kernel.org/r/1466005756-15626-1-git-send-email-mark.rutland@arm.com
>
> diff --git a/Makefile b/Makefile
> index 0f70de6..3785a63 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 -DCC_HAVE_SANCOV_TRACE_PC
>
>
>  # Use USERINCLUDE when you must reference the UAPI directories only.
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index a02f2dd..0a0b164 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,14 @@ static int kcov_open(struct inode *inode, struct file *filep)
>  {
>         struct kcov *kcov;
>
> +#ifndef CC_HAVE_SANCOV_TRACE_PC
> +       /*
> +        * CONFIG_KCOV was selected, but the compiler does not support the
> +        * options KCOV requires.
> +        */
> +       return -ENOTSUPP;
> +#endif /* CC_HAVE_SANCOV_TRACE_PC */
> +
>         kcov = kzalloc(sizeof(*kcov), GFP_KERNEL);
>         if (!kcov)
>                 return -ENOMEM;


Looks good to me.

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

[James Morse]

Hi Mark,

On 15/06/16 18:04, Mark Rutland 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 ensures that CC_HAVE_SANCOV_TRACE_PC is defined when the
> toolchain supports -fsanitize-coverage=trace-pc, and is not defined
> otherwise. When CC_HAVE_SANCOV_TRACE_PC is not defined, the kernel will
> return -ENOTSUPP if userspace attempts to open the kcov debugfs file,
> indicating that kcov functionality is unavailable.

> diff --git a/Makefile b/Makefile
> index 0f70de6..3785a63 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 -DCC_HAVE_SANCOV_TRACE_PC
>  
>  
>  # Use USERINCLUDE when you must reference the UAPI directories only.
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index a02f2dd..0a0b164 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,14 @@ static int kcov_open(struct inode *inode, struct file *filep)
>  {
>  	struct kcov *kcov;
>  
> +#ifndef CC_HAVE_SANCOV_TRACE_PC

I don't think this will work as kernel/kcov.c is listed in the Makefile as:
> # Don't self-instrument.
> KCOV_INSTRUMENT_kcov.o := n


This will cause the build machinery in scripts/Makefile.lib to not give
kernel/kcov.c the CFLAGS_KCOV contents:
> ifeq ($(CONFIG_KCOV),y)
> _c_flags += $(if $(patsubst n%,, \
> 	$(KCOV_INSTRUMENT_$(basetarget).o)$(KCOV_INSTRUMENT)y), \
> 	$(CFLAGS_KCOV))
> endif

... so kernel/kcov.c will never see anything in CFLAGS_KCOV ...


An alternative would be to add the flag to the compiler test that generates the
'not supported' warning, but it needs to go in another CFLAGS variable.
Something like:

-------------------%<-------------------
diff --git a/Makefile b/Makefile
@@ -687,6 +687,8 @@ ifdef CONFIG_KCOV
     $(warning Cannot use CONFIG_KCOV: \
              -fsanitize-coverage=trace-pc is not supported by compiler)
     CFLAGS_KCOV =
+  else
+    KBUILD_CFLAGS += -DCC_HAVE_SANCOV_TRACE_PC
   endif
 endif
-------------------%<-------------------


Thanks,

James

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

[Mark Rutland]

On Thu, Jun 16, 2016 at 11:09:37AM +0100, James Morse wrote:
> Hi Mark,
> 
> On 15/06/16 18:04, Mark Rutland 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 ensures that CC_HAVE_SANCOV_TRACE_PC is defined when the
> > toolchain supports -fsanitize-coverage=trace-pc, and is not defined
> > otherwise. When CC_HAVE_SANCOV_TRACE_PC is not defined, the kernel will
> > return -ENOTSUPP if userspace attempts to open the kcov debugfs file,
> > indicating that kcov functionality is unavailable.
> 
> > diff --git a/Makefile b/Makefile
> > index 0f70de6..3785a63 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 -DCC_HAVE_SANCOV_TRACE_PC
> >  
> >  
> >  # Use USERINCLUDE when you must reference the UAPI directories only.
> > diff --git a/kernel/kcov.c b/kernel/kcov.c
> > index a02f2dd..0a0b164 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,14 @@ static int kcov_open(struct inode *inode, struct file *filep)
> >  {
> >  	struct kcov *kcov;
> >  
> > +#ifndef CC_HAVE_SANCOV_TRACE_PC
> 
> I don't think this will work as kernel/kcov.c is listed in the Makefile as:
> > # Don't self-instrument.
> > KCOV_INSTRUMENT_kcov.o := n
> 
> 
> This will cause the build machinery in scripts/Makefile.lib to not give
> kernel/kcov.c the CFLAGS_KCOV contents:
> > ifeq ($(CONFIG_KCOV),y)
> > _c_flags += $(if $(patsubst n%,, \
> > 	$(KCOV_INSTRUMENT_$(basetarget).o)$(KCOV_INSTRUMENT)y), \
> > 	$(CFLAGS_KCOV))
> > endif
> 
> ... so kernel/kcov.c will never see anything in CFLAGS_KCOV ...

Good spot!

Evidently I was trying to be overly clever here; my bad.

> An alternative would be to add the flag to the compiler test that generates the
> 'not supported' warning, but it needs to go in another CFLAGS variable.
> Something like:
> 
> -------------------%<-------------------
> diff --git a/Makefile b/Makefile
> @@ -687,6 +687,8 @@ ifdef CONFIG_KCOV
>      $(warning Cannot use CONFIG_KCOV: \
>               -fsanitize-coverage=trace-pc is not supported by compiler)
>      CFLAGS_KCOV =
> +  else
> +    KBUILD_CFLAGS += -DCC_HAVE_SANCOV_TRACE_PC
>    endif
>  endif
> -------------------%<-------------------

I'll fold the above in for v3.

Thanks,
Mark.