Re: [PATCH 5/5] Add a sysrq option to exit secure boot mode

[joeyli]

Hi,

On Wed, May 24, 2017 at 03:46:03PM +0100, David Howells wrote:
> From: Kyle McMartin <kyle@xxxxxxxxxx>
> 
> Make sysrq+x exit secure boot mode on x86_64, thereby allowing the running
> kernel image to be modified.  This lifts the lockdown.
> 
> Signed-off-by: Kyle McMartin <kyle@xxxxxxxxxx>
> Signed-off-by: David Howells <dhowells@xxxxxxxxxx>
> cc: x86@xxxxxxxxxx
> ---
> 
>  arch/x86/include/asm/efi.h        |    2 ++
>  drivers/firmware/efi/Kconfig      |   10 ++++++++++
>  drivers/firmware/efi/secureboot.c |   37 +++++++++++++++++++++++++++++++++++++
>  drivers/input/misc/uinput.c       |    1 +
>  drivers/tty/sysrq.c               |   19 +++++++++++++------
>  include/linux/input.h             |    5 +++++
>  include/linux/sysrq.h             |    8 +++++++-
>  kernel/debug/kdb/kdb_main.c       |    2 +-
>  8 files changed, 76 insertions(+), 8 deletions(-)
> 
[...snip]  
> diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> index 3ffc1ce29023..8b766dbad6dd 100644
> --- a/drivers/tty/sysrq.c
> +++ b/drivers/tty/sysrq.c
> @@ -481,6 +481,7 @@ static struct sysrq_key_op *sysrq_key_table[36] = {
>  	/* x: May be registered on mips for TLB dump */
>  	/* x: May be registered on ppc/powerpc for xmon */
>  	/* x: May be registered on sparc64 for global PMU dump */
> +	/* x: May be registered on x86_64 for disabling secure boot */
>  	NULL,				/* x */

I suggest that add this key to the "What are the 'command' keys?"
session in Documentation/admin-guide/sysrq.rst.

>  	/* y: May be registered on sparc64 for global register dump */
>  	NULL,				/* y */
> @@ -524,7 +525,7 @@ static void __sysrq_put_key_op(int key, struct sysrq_key_op *op_p)
>                  sysrq_key_table[i] = op_p;
>  }
>  
> -void __handle_sysrq(int key, bool check_mask)
> +void __handle_sysrq(int key, unsigned int from)
>  {
>  	struct sysrq_key_op *op_p;
>  	int orig_log_level;
> @@ -544,11 +545,15 @@ void __handle_sysrq(int key, bool check_mask)
>  
>          op_p = __sysrq_get_key_op(key);
>          if (op_p) {
> +		/* Ban synthetic events from some sysrq functionality */
> +		if ((from == SYSRQ_FROM_PROC || from == SYSRQ_FROM_SYNTHETIC) &&
> +		    op_p->enable_mask & SYSRQ_DISABLE_USERSPACE)
> +			printk("This sysrq operation is disabled from userspace.\n");
>  		/*
>  		 * Should we check for enabled operations (/proc/sysrq-trigger
>  		 * should not) and is the invoked operation enabled?
>  		 */
> -		if (!check_mask || sysrq_on_mask(op_p->enable_mask)) {
> +		if (from == SYSRQ_FROM_KERNEL || sysrq_on_mask(op_p->enable_mask)) {
>  			pr_cont("%s\n", op_p->action_msg);
>  			console_loglevel = orig_log_level;
>  			op_p->handler(key);

Looking at sysrq_on_mask():

static bool sysrq_on_mask(int mask)
{
	return sysrq_always_enabled ||
	       sysrq_enabled == 1 ||
	       (sysrq_enabled & mask); 
}

The SYSRQ_DISABLE_USERSPACE can be ignored by sysrq_always_enabled or
sysrq_enabled (the default value is CONFIG_MAGIC_SYSRQ_DEFAULT_ENABLE=0x1).
Kernel should checks the locked down flag here when secure boot ON. Will we
have another lock down patch against this? Or I missed something?

Thanks a lot!
Joey Lee