* [BUG] ati_remote2.c: possible mutex_lock without mutex_unlock
@ 2009-10-13 17:52 ` iceberg
0 siblings, 0 replies; 7+ messages in thread
From: iceberg @ 2009-10-13 17:52 UTC (permalink / raw)
To: Vojtech Pavlik, Dmitry Torokhov, Dmitry Torokhov,
Linux Kernlel Mailing List, linux-input
KERNEL_VERSION: 2.6.31
DESCRIBE:
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
}
783 static void *input_devices_seq_start(struct seq_file *seq, loff_t
*pos)
784{
785 if (mutex_lock_interruptible(&input_mutex))
786 return NULL;
787
788 return seq_list_start(&input_dev_list, *pos);
789}
663struct list_head *seq_list_start(struct list_head *head, loff_t
pos)
664{
665 struct list_head *lh;
666
667 list_for_each(lh, head)
668 if (pos-- == 0)
669 return lh;
670
671 return NULL;
672}
673
674EXPORT_SYMBOL(seq_list_start);
675
Found by: Linux Driver Verification project
^ permalink raw reply [flat|nested] 7+ messages in thread* [BUG] ati_remote2.c: possible mutex_lock without mutex_unlock @ 2009-10-13 17:52 ` iceberg 0 siblings, 0 replies; 7+ messages in thread From: iceberg @ 2009-10-13 17:52 UTC (permalink / raw) To: Vojtech Pavlik, Dmitry Torokhov, Dmitry Torokhov, Linux Kernlel Mailing List, linux-input KERNEL_VERSION: 2.6.31 DESCRIBE: 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 } 783 static void *input_devices_seq_start(struct seq_file *seq, loff_t *pos) 784{ 785 if (mutex_lock_interruptible(&input_mutex)) 786 return NULL; 787 788 return seq_list_start(&input_dev_list, *pos); 789} 663struct list_head *seq_list_start(struct list_head *head, loff_t pos) 664{ 665 struct list_head *lh; 666 667 list_for_each(lh, head) 668 if (pos-- == 0) 669 return lh; 670 671 return NULL; 672} 673 674EXPORT_SYMBOL(seq_list_start); 675 Found by: Linux Driver Verification project ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUG] ati_remote2.c: possible mutex_lock without mutex_unlock 2009-10-13 17:52 ` iceberg (?) @ 2009-10-13 15:43 ` Jiri Kosina 2009-10-14 6:29 ` Dmitry Torokhov -1 siblings, 1 reply; 7+ messages in thread From: Jiri Kosina @ 2009-10-13 15:43 UTC (permalink / raw) To: iceberg, Dmitry Torokhov Cc: Vojtech Pavlik, Dmitry Torokhov, Linux Kernlel Mailing List, linux-input 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@suse.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@ispras.ru> Signed-off-by: Jiri Kosina <jkosina@suse.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. ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [BUG] ati_remote2.c: possible mutex_lock without mutex_unlock 2009-10-13 15:43 ` Jiri Kosina @ 2009-10-14 6:29 ` Dmitry Torokhov 2009-10-14 7:11 ` Jiri Kosina 0 siblings, 1 reply; 7+ messages in thread From: Dmitry Torokhov @ 2009-10-14 6:29 UTC (permalink / raw) To: Jiri Kosina Cc: iceberg, Vojtech Pavlik, Linux Kernlel Mailing List, linux-input Hi Jiri, On Tue, Oct 13, 2009 at 05:43:44PM +0200, Jiri Kosina wrote: > 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? > Umm, I don't like assuming that EAGAIN can only mean that mutex_lock_interruptible() failed, seq_file core may theoretically return -EAGAIN too. In fact, looking through seq_file.c traverse() does return -EAGAIN in certain cases... Plus input_devices_seq_start() needs the same treatment (you fixed input_handlers_seq_start() only). HOw about this one instead? -- Dmitry Input: fix locking issue in /proc/bus/input/ handlers From: Dmitry Torokhov <dmitry.torokhov@gmail.com> input_devices_seq_start() uses mutex_lock_interruptible() to acquire the input_mutex, but doesn't properly handle the situation when the call fails (for example due to interrupt). Instead of returning NULL (which indicates that there is no more data) we should return ERR_PTR()-encoded error. We also need explicit flag indicating whether input_mutex was acquired since input_devices_seq_stop() is called whether input_devices_seq_start() was successful or not. The same applies to input_handlers_seq_start(). Reported-by: iceberg <strakh@ispras.ru> Signed-off-by: Dmitry Torokhov <dtor@mail.ru> --- drivers/input/input.c | 65 ++++++++++++++++++++++++++++++++++++------------- 1 files changed, 48 insertions(+), 17 deletions(-) diff --git a/drivers/input/input.c b/drivers/input/input.c index c6f88eb..cc763c9 100644 --- a/drivers/input/input.c +++ b/drivers/input/input.c @@ -782,10 +782,29 @@ static unsigned int input_proc_devices_poll(struct file *file, poll_table *wait) return 0; } +union input_seq_state { + struct { + unsigned short pos; + bool mutex_acquired; + }; + void *p; +}; + static void *input_devices_seq_start(struct seq_file *seq, loff_t *pos) { - if (mutex_lock_interruptible(&input_mutex)) - return NULL; + union input_seq_state *state = (union input_seq_state *)&seq->private; + int error; + + /* We need to fit into seq->private pointer */ + BUILD_BUG_ON(sizeof(union input_seq_state) != sizeof(seq->private)); + + error = mutex_lock_interruptible(&input_mutex); + if (error) { + state->mutex_acquired = false; + return ERR_PTR(error); + } + + state->mutex_acquired = true; return seq_list_start(&input_dev_list, *pos); } @@ -795,9 +814,12 @@ static void *input_devices_seq_next(struct seq_file *seq, void *v, loff_t *pos) return seq_list_next(v, &input_dev_list, pos); } -static void input_devices_seq_stop(struct seq_file *seq, void *v) +static void input_seq_stop(struct seq_file *seq, void *v) { - mutex_unlock(&input_mutex); + union input_seq_state *state = (union input_seq_state *)&seq->private; + + if (state->mutex_acquired) + mutex_unlock(&input_mutex); } static void input_seq_print_bitmap(struct seq_file *seq, const char *name, @@ -861,7 +883,7 @@ static int input_devices_seq_show(struct seq_file *seq, void *v) static const struct seq_operations input_devices_seq_ops = { .start = input_devices_seq_start, .next = input_devices_seq_next, - .stop = input_devices_seq_stop, + .stop = input_seq_stop, .show = input_devices_seq_show, }; @@ -881,40 +903,49 @@ 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; + union input_seq_state *state = (union input_seq_state *)&seq->private; + int error; + + /* We need to fit into seq->private pointer */ + BUILD_BUG_ON(sizeof(union input_seq_state) != sizeof(seq->private)); + + error = mutex_lock_interruptible(&input_mutex); + if (error) { + state->mutex_acquired = false; + return ERR_PTR(error); + } + + state->mutex_acquired = true; + state->pos = *pos; - seq->private = (void *)(unsigned long)*pos; return seq_list_start(&input_handler_list, *pos); } static void *input_handlers_seq_next(struct seq_file *seq, void *v, loff_t *pos) { - seq->private = (void *)(unsigned long)(*pos + 1); - return seq_list_next(v, &input_handler_list, pos); -} + union input_seq_state *state = (union input_seq_state *)&seq->private; -static void input_handlers_seq_stop(struct seq_file *seq, void *v) -{ - mutex_unlock(&input_mutex); + state->pos = *pos + 1; + return seq_list_next(v, &input_handler_list, pos); } static int input_handlers_seq_show(struct seq_file *seq, void *v) { struct input_handler *handler = container_of(v, struct input_handler, node); + union input_seq_state *state = (union input_seq_state *)&seq->private; - seq_printf(seq, "N: Number=%ld Name=%s", - (unsigned long)seq->private, handler->name); + seq_printf(seq, "N: Number=%u Name=%s", state->pos, handler->name); if (handler->fops) seq_printf(seq, " Minor=%d", handler->minor); seq_putc(seq, '\n'); return 0; } + static const struct seq_operations input_handlers_seq_ops = { .start = input_handlers_seq_start, .next = input_handlers_seq_next, - .stop = input_handlers_seq_stop, + .stop = input_seq_stop, .show = input_handlers_seq_show, }; ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [BUG] ati_remote2.c: possible mutex_lock without mutex_unlock 2009-10-14 6:29 ` Dmitry Torokhov @ 2009-10-14 7:11 ` Jiri Kosina 2009-10-14 7:14 ` Dmitry Torokhov 0 siblings, 1 reply; 7+ messages in thread From: Jiri Kosina @ 2009-10-14 7:11 UTC (permalink / raw) To: Dmitry Torokhov Cc: iceberg, Vojtech Pavlik, Linux Kernlel Mailing List, linux-input On Tue, 13 Oct 2009, Dmitry Torokhov wrote: > Umm, I don't like assuming that EAGAIN can only mean that > mutex_lock_interruptible() failed, seq_file core may theoretically > return -EAGAIN too. In fact, looking through seq_file.c traverse() does > return -EAGAIN in certain cases... Damn, you are right -- I explicitly checked for this, but have completely overlooked the "Eoveflow:" branch in traverse(), which returns EAGAIN. So my previous patch is of course incorrect. > Input: fix locking issue in /proc/bus/input/ handlers > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > input_devices_seq_start() uses mutex_lock_interruptible() to acquire > the input_mutex, but doesn't properly handle the situation when the > call fails (for example due to interrupt). Instead of returning NULL > (which indicates that there is no more data) we should return > ERR_PTR()-encoded error. > > We also need explicit flag indicating whether input_mutex was acquired > since input_devices_seq_stop() is called whether input_devices_seq_start() > was successful or not. > > The same applies to input_handlers_seq_start(). > > Reported-by: iceberg <strakh@ispras.ru> > Signed-off-by: Dmitry Torokhov <dtor@mail.ru> Yup, looks OK to me. -- Jiri Kosina SUSE Labs, Novell Inc. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUG] ati_remote2.c: possible mutex_lock without mutex_unlock 2009-10-14 7:11 ` Jiri Kosina @ 2009-10-14 7:14 ` Dmitry Torokhov 2009-10-14 7:16 ` Jiri Kosina 0 siblings, 1 reply; 7+ messages in thread From: Dmitry Torokhov @ 2009-10-14 7:14 UTC (permalink / raw) To: Jiri Kosina Cc: iceberg, Vojtech Pavlik, Linux Kernlel Mailing List, linux-input On Wed, Oct 14, 2009 at 09:11:06AM +0200, Jiri Kosina wrote: > On Tue, 13 Oct 2009, Dmitry Torokhov wrote: > > > Umm, I don't like assuming that EAGAIN can only mean that > > mutex_lock_interruptible() failed, seq_file core may theoretically > > return -EAGAIN too. In fact, looking through seq_file.c traverse() does > > return -EAGAIN in certain cases... > > Damn, you are right -- I explicitly checked for this, but have completely > overlooked the "Eoveflow:" branch in traverse(), which returns EAGAIN. So > my previous patch is of course incorrect. > > > Input: fix locking issue in /proc/bus/input/ handlers > > > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > > input_devices_seq_start() uses mutex_lock_interruptible() to acquire > > the input_mutex, but doesn't properly handle the situation when the > > call fails (for example due to interrupt). Instead of returning NULL > > (which indicates that there is no more data) we should return > > ERR_PTR()-encoded error. > > > > We also need explicit flag indicating whether input_mutex was acquired > > since input_devices_seq_stop() is called whether input_devices_seq_start() > > was successful or not. > > > > The same applies to input_handlers_seq_start(). > > > > Reported-by: iceberg <strakh@ispras.ru> > > Signed-off-by: Dmitry Torokhov <dtor@mail.ru> > > Yup, looks OK to me. > Putting you as "Reviewed-by.." then, OK? -- Dmitry ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUG] ati_remote2.c: possible mutex_lock without mutex_unlock 2009-10-14 7:14 ` Dmitry Torokhov @ 2009-10-14 7:16 ` Jiri Kosina 0 siblings, 0 replies; 7+ messages in thread From: Jiri Kosina @ 2009-10-14 7:16 UTC (permalink / raw) To: Dmitry Torokhov Cc: iceberg, Vojtech Pavlik, Linux Kernlel Mailing List, linux-input On Wed, 14 Oct 2009, Dmitry Torokhov wrote: > > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > > > > input_devices_seq_start() uses mutex_lock_interruptible() to acquire > > > the input_mutex, but doesn't properly handle the situation when the > > > call fails (for example due to interrupt). Instead of returning NULL > > > (which indicates that there is no more data) we should return > > > ERR_PTR()-encoded error. > > > > > > We also need explicit flag indicating whether input_mutex was acquired > > > since input_devices_seq_stop() is called whether input_devices_seq_start() > > > was successful or not. > > > > > > The same applies to input_handlers_seq_start(). > > > > > > Reported-by: iceberg <strakh@ispras.ru> > > > Signed-off-by: Dmitry Torokhov <dtor@mail.ru> > > > > Yup, looks OK to me. > > Putting you as "Reviewed-by.." then, OK? Sure, feel free to do that. Thanks, -- Jiri Kosina SUSE Labs, Novell Inc. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2009-10-14 7:17 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-10-13 17:52 [BUG] ati_remote2.c: possible mutex_lock without mutex_unlock iceberg 2009-10-13 17:52 ` iceberg 2009-10-13 15:43 ` Jiri Kosina 2009-10-14 6:29 ` Dmitry Torokhov 2009-10-14 7:11 ` Jiri Kosina 2009-10-14 7:14 ` Dmitry Torokhov 2009-10-14 7:16 ` Jiri Kosina
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.