[PATCH] MODSIGN: Warn when sign check fails due to -ENOKEY

[PATCH] MODSIGN: Warn when sign check fails due to -ENOKEY

[Chris Samuel]

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

/* Please CC me in responses, I am not subscribed to LKML */

Currently if a signature check fails on module load due to not having
the appropriate key (-ENOKEY) and we are not doing strict checking
there is no information provided to the user other than the lock debug
taint warning:

Disabling lock debugging due to kernel taint

This patch causes a single warning to be emitted to explain why the
kernel is being tainted, before the above taint warning occurs.

Module verification failed, required key not present, tainting kernel

Found whilst trying to work out why all the 3.8 development kernels
I was building and testing were warning about taints and why all modules
were listed as forced load (F) in /proc/modules when that wasn't the
case in the 3.5, 3.6 or 3.7 kernels I'd tried.

Signed-off-by: Christopher Samuel <chris@csamuel.org>
---
  kernel/module.c |    4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/module.c b/kernel/module.c
index 250092c..27de534 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2443,8 +2443,10 @@ static int module_sig_check(struct load_info *info)
  	if (err < 0 && fips_enabled)
  		panic("Module verification failed with error %d in FIPS mode\n",
  		      err);
-	if (err == -ENOKEY && !sig_enforce)
+	if (err == -ENOKEY && !sig_enforce) {
+		printk_once(KERN_DEBUG "Module verification failed, required key not 
present, tainting kernel\n");
  		err = 0;
+	}
   	return err;
  }
-- 
1.7.10.4


[-- Attachment #2: Attached Message Part --]
[-- Type: text/plain, Size: 0 bytes --]

Re: [PATCH] MODSIGN: Warn when sign check fails due to -ENOKEY

[Josh Boyer]

On Fri, Jan 11, 2013 at 4:44 AM, Chris Samuel <chris@csamuel.org> wrote:
> /* Please CC me in responses, I am not subscribed to LKML */
>
> Currently if a signature check fails on module load due to not having
> the appropriate key (-ENOKEY) and we are not doing strict checking
> there is no information provided to the user other than the lock debug
> taint warning:
>
> Disabling lock debugging due to kernel taint
>
> This patch causes a single warning to be emitted to explain why the
> kernel is being tainted, before the above taint warning occurs.
>
> Module verification failed, required key not present, tainting kernel
>
> Found whilst trying to work out why all the 3.8 development kernels
> I was building and testing were warning about taints and why all modules
> were listed as forced load (F) in /proc/modules when that wasn't the
> case in the 3.5, 3.6 or 3.7 kernels I'd tried.
>
> Signed-off-by: Christopher Samuel <chris@csamuel.org>
> ---
>  kernel/module.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index 250092c..27de534 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2443,8 +2443,10 @@ static int module_sig_check(struct load_info *info)
>         if (err < 0 && fips_enabled)
>                 panic("Module verification failed with error %d in FIPS
> mode\n",
>                       err);
> -       if (err == -ENOKEY && !sig_enforce)
> +       if (err == -ENOKEY && !sig_enforce) {
> +               printk_once(KERN_DEBUG "Module verification failed, required
> key not present, tainting kernel\n");
>                 err = 0;
> +       }
>         return err;

I'd suggest putting the printk in load_module where we call the
add_taint_module function instead.  Also, you might want to make the
priority a bit higher if it's meant to be informative.  Something like
KERN_INFO.

josh

Re: [PATCH] MODSIGN: Warn when sign check fails due to -ENOKEY

[Rusty Russell]

Josh Boyer <jwboyer@gmail.com> writes:

> On Fri, Jan 11, 2013 at 4:44 AM, Chris Samuel <chris@csamuel.org> wrote:
>> /* Please CC me in responses, I am not subscribed to LKML */
>>
>> Currently if a signature check fails on module load due to not having
>> the appropriate key (-ENOKEY) and we are not doing strict checking
>> there is no information provided to the user other than the lock debug
>> taint warning:
>>
>> Disabling lock debugging due to kernel taint
>>
>> This patch causes a single warning to be emitted to explain why the
>> kernel is being tainted, before the above taint warning occurs.
>>
>> Module verification failed, required key not present, tainting kernel
>>
>> Found whilst trying to work out why all the 3.8 development kernels
>> I was building and testing were warning about taints and why all modules
>> were listed as forced load (F) in /proc/modules when that wasn't the
>> case in the 3.5, 3.6 or 3.7 kernels I'd tried.
>>
>> Signed-off-by: Christopher Samuel <chris@csamuel.org>
>> ---
>>  kernel/module.c |    4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/module.c b/kernel/module.c
>> index 250092c..27de534 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -2443,8 +2443,10 @@ static int module_sig_check(struct load_info *info)
>>         if (err < 0 && fips_enabled)
>>                 panic("Module verification failed with error %d in FIPS
>> mode\n",
>>                       err);
>> -       if (err == -ENOKEY && !sig_enforce)
>> +       if (err == -ENOKEY && !sig_enforce) {
>> +               printk_once(KERN_DEBUG "Module verification failed, required
>> key not present, tainting kernel\n");
>>                 err = 0;
>> +       }
>>         return err;
>
> I'd suggest putting the printk in load_module where we call the
> add_taint_module function instead.  Also, you might want to make the
> priority a bit higher if it's meant to be informative.  Something like
> KERN_INFO.

Agreed.  KERN_NOTICE, I think: we really want to see if someone's
inserting an unsigned module!

Cheers,
Rusty.

Re: [PATCH] MODSIGN: Warn when sign check fails due to -ENOKEY

[Chris Samuel]

On 12/01/13 00:49, Josh Boyer wrote:

> On Fri, Jan 11, 2013 at 4:44 AM, Chris Samuel <chris@csamuel.org> wrote:
 >
>> /* Please CC me in responses, I am not subscribed to LKML */
>>
>> diff --git a/kernel/module.c b/kernel/module.c
>> index 250092c..27de534 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -2443,8 +2443,10 @@ static int module_sig_check(struct load_info *info)
>>          if (err < 0 && fips_enabled)
>>                  panic("Module verification failed with error %d in FIPS
>> mode\n",
>>                        err);
>> -       if (err == -ENOKEY && !sig_enforce)
>> +       if (err == -ENOKEY && !sig_enforce) {
>> +               printk_once(KERN_DEBUG "Module verification failed, required
>> key not present, tainting kernel\n");
>>                  err = 0;
>> +       }
>>          return err;
>
> I'd suggest putting the printk in load_module where we call the
> add_taint_module function instead.

I did ponder that, but I used module_sig_check() instead as here we know 
explicitly that the failure is -ENOKEY, that information doesn't seem to 
get propagated back to load_module().

Looking at the code again though it seems that any other reason will 
make module_sig_check() return non-zero and hence cause the module to 
fail to load, so currently we can infer that the reason was -ENOKEY.

I'm happy either way, just my inner pedant thought this was better as in 
future module_sig_check() may find another reason to have to return with 
a zero status when modules aren't signed and so we can no longer tell 
the user the reason the signature failed.

Rusty, which is your preference?

> Also, you might want to make the priority a bit higher if it's meant
> to be informative.  Something like KERN_INFO.

Yup, sounds good, I see Rusty suggested KERN_NOTICE so I'll use that.

cheers,
Chris
-- 
  Chris Samuel  :  http://www.csamuel.org/  :  Melbourne, VIC