linux-kernel - Re: [RFC PATCH] sysrq: don't hold the sysrq_key_table

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20100726105148.GB14198@hmsreliant.think-freely.org>
Date:	Mon, 26 Jul 2010 06:51:48 -0400
From:	Neil Horman <nhorman@...driver.com>
To:	Xiaotian Feng <dfeng@...hat.com>
Cc:	linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...e.hu>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"David S. Miller" <davem@...emloft.net>
Subject: Re: [RFC PATCH] sysrq: don't hold the sysrq_key_table_lock during
 the handler

On Mon, Jul 26, 2010 at 05:54:02PM +0800, Xiaotian Feng wrote:
> sysrq_key_table_lock is used to protect the sysrq_key_table, make sure
> we get/replace the right operation for the sysrq. But in __handle_sysrq,
> kernel will hold this lock and disable irqs until we finished op_p->handler().
> This may cause false positive watchdog alert when we're doing "show-task-states"
> on a system with many tasks.
> 
> Signed-off-by: Xiaotian Feng <dfeng@...hat.com>
> Cc: Ingo Molnar <mingo@...e.hu>
> Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Neil Horman <nhorman@...driver.com>
> Cc: "David S. Miller" <davem@...emloft.net>
> ---
>  drivers/char/sysrq.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/char/sysrq.c b/drivers/char/sysrq.c
> index 878ac0c..0856e2e 100644
> --- a/drivers/char/sysrq.c
> +++ b/drivers/char/sysrq.c
> @@ -520,9 +520,11 @@ void __handle_sysrq(int key, struct tty_struct *tty, int check_mask)
>  		if (!check_mask || sysrq_on_mask(op_p->enable_mask)) {
>  			printk("%s\n", op_p->action_msg);
>  			console_loglevel = orig_log_level;
> +			spin_unlock_irqrestore(&sysrq_key_table_lock, flags);
>  			op_p->handler(key, tty);
>  		} else {
>  			printk("This sysrq operation is disabled.\n");
> +			spin_unlock_irqrestore(&sysrq_key_table_lock, flags);
>  		}
>  	} else {
>  		printk("HELP : ");
> @@ -541,8 +543,8 @@ void __handle_sysrq(int key, struct tty_struct *tty, int check_mask)
>  		}
>  		printk("\n");
>  		console_loglevel = orig_log_level;
> +		spin_unlock_irqrestore(&sysrq_key_table_lock, flags);
>  	}
> -	spin_unlock_irqrestore(&sysrq_key_table_lock, flags);
>  }
>  
>  void handle_sysrq(int key, struct tty_struct *tty)
> -- 
> 1.7.2
> 
> 

This creates the possibility of a race in the handler.  Not that it happens
often, but sysrq keys can be registered and unregistered dynamically.  If that
lock isn't held while we call the keys handler, the code implementing that
handler can live in a module that gets removed while its executing, leading to
an oops, etc.  I think the better solution would be to use an rcu lock here.
Neil

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/