linux-kernel - Re: [BUG] ati_remote2.c: possible mutex_lock without mutex

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Tue, 13 Oct 2009 17:43:44 +0200 (CEST)
From:	Jiri Kosina <jkosina@...e.cz>
To:	iceberg <strakh@...ras.ru>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:	Vojtech Pavlik <vojtech@...e.cz>, Dmitry Torokhov <dtor@...l.ru>,
	Linux Kernlel Mailing List <linux-kernel@...r.kernel.org>,
	linux-input@...r.kernel.org
Subject: Re: [BUG] ati_remote2.c: possible mutex_lock without mutex_unlock

On Tue, 13 Oct 2009, iceberg wrote:

> 	In driver ./drivers/input/input.c possible call to mutex_lock from 
> function input_devices_seq_start without mutex_unlock.
> 
> 	After calling input_devices_seq_start we can't know whether 
> mutex was locked or not. 

> Case 1. If mutex_lock_interruptible was not locked due to interrupt then 
> input_devices_seq_start returns NULL.
> Case 2. If mutex was successfuly locked but seq_list_start returned NULL 
> then input_devices_seq_start returns NULL too. The last case occurs if 
> seq_list_start is called with pos>size of input_dev_list or pos<0.
> Hence, after calling input_devices_seq_start we can not simply check 
> that result is not NULL and call input_devices_seq_stop function 
> which unlocks the mutex. Because in case 2 the mutex will stay locked.
> void * ret = input_devices_seq_start(...);
> if(ret!=NULL) {
> 	//mutex is acquired for sure
> 	input_devices_seq_stop(...);//unlocks the mutex
> } else {
> 	//mutex may be acquired or not
> }

Plus, we should return EAGAIN rather than failing silently when 
input_handlers_seq_start() has been interrupted by signal, right?

Dmitry, how about the fix below?



From: Jiri Kosina <jkosina@...e.cz>
Subject: [PATCH] Input: make input_handlers_seq_start() signal safe

input_devices_seq_start() uses mutex_lock_interruptible() to acquire the 
input_mutex, but doesn't properly handle the situation if 
mutex_lock_interruptible() really gets interrupted. In such scenario, 
input_handlers_seq_start() returns NULL, which ambiguous, as 
seq_list_start() could return NULL as well. This could lead to the 
situation in which input_handlers_seq_stop() will try to unlock mutex that 
hasn't been locked.

Plus, in such situations, the code fails silently, rather than returning 
EAGAIN.

Reported-by: iceberg <strakh@...ras.ru>
Signed-off-by: Jiri Kosina <jkosina@...e.cz>
--- 
 drivers/input/input.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/input/input.c b/drivers/input/input.c
index c6f88eb..ef4d5c1 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -882,7 +882,7 @@ static const struct file_operations input_devices_fileops = {
 static void *input_handlers_seq_start(struct seq_file *seq, loff_t *pos)
 {
 	if (mutex_lock_interruptible(&input_mutex))
-		return NULL;
+		return ERR_PTR(-EAGAIN);
 
 	seq->private = (void *)(unsigned long)*pos;
 	return seq_list_start(&input_handler_list, *pos);
@@ -896,6 +896,10 @@ static void *input_handlers_seq_next(struct seq_file *seq, void *v, loff_t *pos)
 
 static void input_handlers_seq_stop(struct seq_file *seq, void *v)
 {
+	/* seq_start could get interrupted by signal before acquiring mutex */
+	if (IS_ERR(v) && ERR_PTR(v) == -EAGAIN)
+		return;
+
 	mutex_unlock(&input_mutex);
 }
 

-- 
Jiri Kosina
SUSE Labs, Novell Inc.
--
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/