* checkpatch.pl bug? (was Re: [PATCH] random: Make CPU trust a boot parameter)
@ 2018-08-27 21:55 Kees Cook
2018-08-28 5:12 ` Joe Perches
0 siblings, 1 reply; 4+ messages in thread
From: Kees Cook @ 2018-08-27 21:55 UTC (permalink / raw)
To: Joe Perches; +Cc: LKML
On Mon, Aug 27, 2018 at 2:51 PM, Kees Cook <keescook@chromium.org> wrote:
> Instead of forcing a distro or other system builder to choose
> at build time whether the CPU is trusted for CRNG seeding via
> CONFIG_RANDOM_TRUST_CPU, provide a boot-time parameter for end users to
> control the choice. The CONFIG will set the default state instead.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> Documentation/admin-guide/kernel-parameters.txt | 6 ++++++
> drivers/char/Kconfig | 4 ++--
> drivers/char/random.c | 11 ++++++++---
> 3 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 9871e649ffef..64a3bf54b974 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3523,6 +3523,12 @@
> ramdisk_size= [RAM] Sizes of RAM disks in kilobytes
> See Documentation/blockdev/ramdisk.txt.
>
> + random.trust_cpu={on,off}
> + [KNL] Enable or disable trusting the use of the
> + CPU's random number generator (if available) to
> + fully seed the kernel's CRNG. Default is controlled
> + by CONFIG_RANDOM_TRUST_CPU.
> +
> ras=option[,option,...] [KNL] RAS-specific options
>
> cec_disable [X86]
> diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
> index ce277ee0a28a..40728491f37b 100644
> --- a/drivers/char/Kconfig
> +++ b/drivers/char/Kconfig
> @@ -566,5 +566,5 @@ config RANDOM_TRUST_CPU
> that CPU manufacturer (perhaps with the insistence or mandate
> of a Nation State's intelligence or law enforcement agencies)
> has not installed a hidden back door to compromise the CPU's
> - random number generation facilities.
> -
> + random number generation facilities. This can also be configured
> + at boot with "random.trust_cpu=on/off".
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index bf5f99fc36f1..c75b6cdf0053 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -779,6 +779,13 @@ static struct crng_state **crng_node_pool __read_mostly;
>
> static void invalidate_batched_entropy(void);
>
> +static bool trust_cpu __ro_after_init = IS_ENABLED(CONFIG_RANDOM_TRUST_CPU);
> +static int __init parse_trust_cpu(char *arg)
> +{
> + return kstrtobool(arg, &trust_cpu);
> +}
> +early_param("random.trust_cpu", parse_trust_cpu);
> +
> static void crng_initialize(struct crng_state *crng)
> {
> int i;
> @@ -799,12 +806,10 @@ static void crng_initialize(struct crng_state *crng)
> }
> crng->state[i] ^= rv;
> }
> -#ifdef CONFIG_RANDOM_TRUST_CPU
> - if (arch_init) {
> + if (trust_cpu && arch_init) {
checkpatch.pl complains:
ERROR: space prohibited after that '&&' (ctx:WxW)
#79: FILE: drivers/char/random.c:809:
+ if (trust_cpu && arch_init) {
^
I can't figure out what is going on here. Using "||" doesn't trigger
the issue; it seems related to the earlier "&trust_cpu" use in the
patch, but I can't figure out what checkpatch was trying to do with
this...
-Kees
> crng_init = 2;
> pr_notice("random: crng done (trusting CPU's manufacturer)\n");
> }
> -#endif
> crng->init_time = jiffies - CRNG_RESEED_INTERVAL - 1;
> }
>
> --
> 2.17.1
>
>
> --
> Kees Cook
> Pixel Security
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: checkpatch.pl bug? (was Re: [PATCH] random: Make CPU trust a boot parameter)
2018-08-27 21:55 checkpatch.pl bug? (was Re: [PATCH] random: Make CPU trust a boot parameter) Kees Cook
@ 2018-08-28 5:12 ` Joe Perches
2018-08-28 5:33 ` [PATCH] checkpatch: Add __ro_after_init to known $Attribute Joe Perches
0 siblings, 1 reply; 4+ messages in thread
From: Joe Perches @ 2018-08-28 5:12 UTC (permalink / raw)
To: Kees Cook; +Cc: LKML
On Mon, 2018-08-27 at 14:55 -0700, Kees Cook wrote:
> On Mon, Aug 27, 2018 at 2:51 PM, Kees Cook <keescook@chromium.org> wrote:
> > Instead of forcing a distro or other system builder to choose
> > at build time whether the CPU is trusted for CRNG seeding via
> > CONFIG_RANDOM_TRUST_CPU, provide a boot-time parameter for end users to
> > control the choice. The CONFIG will set the default state instead.
[]
> > diff --git a/drivers/char/random.c b/drivers/char/random.c
[]
> > @@ -779,6 +779,13 @@ static struct crng_state **crng_node_pool __read_mostly;
> >
> > static void invalidate_batched_entropy(void);
> >
> > +static bool trust_cpu __ro_after_init = IS_ENABLED(CONFIG_RANDOM_TRUST_CPU);
> > +static int __init parse_trust_cpu(char *arg)
> > +{
> > + return kstrtobool(arg, &trust_cpu);
> > +}
> > +early_param("random.trust_cpu", parse_trust_cpu);
> > +
> > static void crng_initialize(struct crng_state *crng)
> > {
> > int i;
> > @@ -799,12 +806,10 @@ static void crng_initialize(struct crng_state *crng)
> > }
> > crng->state[i] ^= rv;
> > }
> > -#ifdef CONFIG_RANDOM_TRUST_CPU
> > - if (arch_init) {
> > + if (trust_cpu && arch_init) {
>
> checkpatch.pl complains:
>
> ERROR: space prohibited after that '&&' (ctx:WxW)
> #79: FILE: drivers/char/random.c:809:
> + if (trust_cpu && arch_init) {
> ^
>
> I can't figure out what is going on here. Using "||" doesn't trigger
> the issue; it seems related to the earlier "&trust_cpu" use in the
> patch, but I can't figure out what checkpatch was trying to do with
> this...
It's the __ro_after_init after the declaration of
static bool trust_cpu that confuses checkpatch.
I'll see what can be done for that.
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] checkpatch: Add __ro_after_init to known $Attribute
2018-08-28 5:12 ` Joe Perches
@ 2018-08-28 5:33 ` Joe Perches
2018-08-28 5:44 ` Kees Cook
0 siblings, 1 reply; 4+ messages in thread
From: Joe Perches @ 2018-08-28 5:33 UTC (permalink / raw)
To: Andrew Morton, Andy Whitcroft; +Cc: Kees Cook, LKML
__ro_after_init is a specific __attribute__ that checkpatch
does currently not understand.
Add it to the known $Attribute types so that code that uses
variables declared with __ro_after_init are not thought to
be a modifier type.
This appears as a defect in checkpatch output of code like:
static bool trust_cpu __ro_after_init = IS_ENABLED(CONFIG_RANDOM_TRUST_CPU);
[...]
if (trust_cpu && arch_init) {
where checkpatch reports:
ERROR: space prohibited after that '&&' (ctx:WxW)
if (trust_cpu && arch_init) {
Reported-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Joe Perches <joe@perches.com>
---
scripts/checkpatch.pl | 1 +
1 file changed, 1 insertion(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 5219280bf7ff..23cde9d90278 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -380,6 +380,7 @@ our $Attribute = qr{
__noclone|
__deprecated|
__read_mostly|
+ __ro_after_init|
__kprobes|
$InitAttribute|
____cacheline_aligned|
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] checkpatch: Add __ro_after_init to known $Attribute
2018-08-28 5:33 ` [PATCH] checkpatch: Add __ro_after_init to known $Attribute Joe Perches
@ 2018-08-28 5:44 ` Kees Cook
0 siblings, 0 replies; 4+ messages in thread
From: Kees Cook @ 2018-08-28 5:44 UTC (permalink / raw)
To: Joe Perches; +Cc: Andrew Morton, Andy Whitcroft, LKML
On Mon, Aug 27, 2018 at 10:33 PM, Joe Perches <joe@perches.com> wrote:
> __ro_after_init is a specific __attribute__ that checkpatch
> does currently not understand.
>
> Add it to the known $Attribute types so that code that uses
> variables declared with __ro_after_init are not thought to
> be a modifier type.
>
> This appears as a defect in checkpatch output of code like:
>
> static bool trust_cpu __ro_after_init = IS_ENABLED(CONFIG_RANDOM_TRUST_CPU);
> [...]
> if (trust_cpu && arch_init) {
>
> where checkpatch reports:
>
> ERROR: space prohibited after that '&&' (ctx:WxW)
> if (trust_cpu && arch_init) {
>
Thanks for tracking this down!
> Reported-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Joe Perches <joe@perches.com>
Tested-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
> scripts/checkpatch.pl | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 5219280bf7ff..23cde9d90278 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -380,6 +380,7 @@ our $Attribute = qr{
> __noclone|
> __deprecated|
> __read_mostly|
> + __ro_after_init|
> __kprobes|
> $InitAttribute|
> ____cacheline_aligned|
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-08-28 5:49 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-27 21:55 checkpatch.pl bug? (was Re: [PATCH] random: Make CPU trust a boot parameter) Kees Cook
2018-08-28 5:12 ` Joe Perches
2018-08-28 5:33 ` [PATCH] checkpatch: Add __ro_after_init to known $Attribute Joe Perches
2018-08-28 5:44 ` Kees Cook
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.