[BUG] hidraw.c: double mutex_lock

[BUG] hidraw.c: double mutex_lock

[iceberg]

	KERNEL_VERSION: 2.6.31
	DESCRIBE:
	In driver ./drivers/hid/hidraw.c in function hidraw_read may be 
double mutex_lock:

Path:
1. line 50: begin first iteration of "while(ret==0)"
2. line 52: first call to mutex_lock
3. inner loop "while (list->head == list->tail)" does not change state 
of mutex, because mutex_lock immediatelly follows mutex_unlock
4. if we go to the second iteration of "while(ret == 0)" in 
line 50 then there are second call to mutex_lock in line 52 (mutex 
aquired twice).

Second iteration of loop "while(ret==0)" is possible if local variable 
ret is not changed at line 94: ret+=len - i.e. len==0;
Variable len may be zero if hidraw_read is called with count==0 or 
list->buffer[list->tail].len == 0.

static ssize_t hidraw_read(struct file *file, char __user *buffer, 
size_t count, loff_t *ppos)
  44{
  45        struct hidraw_list *list = file->private_data;
  46        int ret = 0, len;
  47        char *report;
  48        DECLARE_WAITQUEUE(wait, current);
  49
  50        while (ret == 0) {
  51
  52                mutex_lock(&list->read_mutex);
  53
  54                if (list->head == list->tail) {
  55                        add_wait_queue(&list->hidraw->wait, &wait);
  56                        set_current_state(TASK_INTERRUPTIBLE);
  57
  58                        while (list->head == list->tail) {
  59                                if (file->f_flags & O_NONBLOCK) {
  60                                        ret = -EAGAIN;
  61                                        break;
  62                                }
  63                                if (signal_pending(current)) {
  64                                        ret = -ERESTARTSYS;
  65                                        break;
  66                                }
  67                                if (!list->hidraw->exist) {
  68                                        ret = -EIO;
  69                                        break;
  70                                }
  71
  72                                /* allow O_NONBLOCK to work well 
from other threads */
  73                                mutex_unlock(&list->read_mutex);
  74                                schedule();
  75                                mutex_lock(&list->read_mutex);
  76                                set_current_state
(TASK_INTERRUPTIBLE);
  77                        }
  78
  79                        set_current_state(TASK_RUNNING);
  80                        remove_wait_queue(&list->hidraw->wait, 
&wait);
  81                }
  82
  83                if (ret)
  84                        goto out;
  85
  86                report = list->buffer[list->tail].value;
  87                len = list->buffer[list->tail].len > count ?
  88                        count : list->buffer[list->tail].len;
  89
  90                if (copy_to_user(buffer, list->buffer[list-
>tail].value, len)) {
  91                        ret = -EFAULT;
  92                        goto out;
  93                }
  94                ret += len;
  95
  96                kfree(list->buffer[list->tail].value);
  97                list->tail = (list->tail + 1) & (HIDRAW_BUFFER_SIZE 
- 1);
  98        }
  99out:
 100        mutex_unlock(&list->read_mutex);
 101        return ret;
 102}

Re: [BUG] hidraw.c: double mutex_lock

[Jiri Kosina]

On Mon, 12 Oct 2009, iceberg wrote:

> 	KERNEL_VERSION: 2.6.31
> 	DESCRIBE:
> 	In driver ./drivers/hid/hidraw.c in function hidraw_read may be 
> double mutex_lock:
> 
> Path:
> 1. line 50: begin first iteration of "while(ret==0)"
> 2. line 52: first call to mutex_lock
> 3. inner loop "while (list->head == list->tail)" does not change state 
> of mutex, because mutex_lock immediatelly follows mutex_unlock
> 4. if we go to the second iteration of "while(ret == 0)" in 
> line 50 then there are second call to mutex_lock in line 52 (mutex 
> aquired twice).
> 
> Second iteration of loop "while(ret==0)" is possible if local variable 
> ret is not changed at line 94: ret+=len - i.e. len==0;
> Variable len may be zero if hidraw_read is called with count==0 or 
> list->buffer[list->tail].len == 0.

Good catch. I will fix that up by moving the mutex_lock() so that it's 
locked before the loop is entered.

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.