linux-kernel - Re: [PATCH 4/5] usb:Push BKL on open down into the drivers

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-next>] [day] [month] [year] [list]

Message-ID: <alpine.LNX.2.00.1003291159470.24576@pobox.suse.cz>
Date:	Mon, 29 Mar 2010 12:04:51 +0200 (CEST)
From:	Jiri Kosina <jkosina@...e.cz>
To:	Oliver Neukum <oliver@...kum.org>
Cc:	Greg KH <greg@...ah.com>, USB list <linux-usb@...r.kernel.org>,
	Mauro Carvalho Chehab <mchehab@...radead.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/5] usb:Push BKL on open down into the drivers

On Wed, 13 Jan 2010, Oliver Neukum wrote:

> From 271f10a4643c788be08cc481dc594ee87af25140 Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oliver@...kum.org>
> Date: Wed, 13 Jan 2010 14:46:08 +0100
> Subject: [PATCH 4/5] usb:Push BKL on open down into the drivers
> 
> Straightforward push into the drivers to allow
> auditing individual drivers separately
> 
> Signed-off-by: Oliver Neukum <oliver@...kum.org>
> ---
>  drivers/hid/usbhid/hiddev.c           |    7 +++++--
>  drivers/media/video/dabusb.c          |    8 +++++++-
>  drivers/staging/frontier/alphatrack.c |    2 ++
>  drivers/staging/frontier/tranzport.c  |    2 ++
>  drivers/usb/class/cdc-wdm.c           |    3 +++
>  drivers/usb/class/usblp.c             |    3 +++
>  drivers/usb/class/usbtmc.c            |    3 +++
>  drivers/usb/core/file.c               |    2 --
>  drivers/usb/image/mdc800.c            |    3 +++
>  drivers/usb/misc/adutux.c             |    3 +++
>  drivers/usb/misc/ftdi-elan.c          |   15 ++++++++++++---
>  drivers/usb/misc/idmouse.c            |    8 +++++++-
>  drivers/usb/misc/iowarrior.c          |    4 ++++
>  drivers/usb/misc/ldusb.c              |   12 ++++++++++--
>  drivers/usb/misc/legousbtower.c       |    3 +++
>  drivers/usb/misc/rio500.c             |    3 ++-
>  drivers/usb/misc/sisusbvga/sisusb.c   |   14 ++++++++++++--
>  drivers/usb/misc/usblcd.c             |    5 +++++
>  drivers/usb/misc/vstusb.c             |    9 ++++++++-
>  drivers/usb/usb-skeleton.c            |    3 +++
>  20 files changed, 97 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
> index 867e084..433602a 100644
> --- a/drivers/hid/usbhid/hiddev.c
> +++ b/drivers/hid/usbhid/hiddev.c
> @@ -265,9 +265,10 @@ static int hiddev_release(struct inode * inode, struct file * file)
>  static int hiddev_open(struct inode *inode, struct file *file)
>  {
>  	struct hiddev_list *list;
> -	int res;
> +	int res, i;
>  
> -	int i = iminor(inode) - HIDDEV_MINOR_BASE;
> +	lock_kernel();
> +	i = iminor(inode) - HIDDEV_MINOR_BASE;
>  
>  	if (i >= HIDDEV_MINORS || i < 0 || !hiddev_table[i])
>  		return -ENODEV;
> @@ -313,10 +314,12 @@ static int hiddev_open(struct inode *inode, struct file *file)
>  			usbhid_open(hid);
>  		}
>  
> +	unlock_kernel();
>  	return 0;
>  bail:
>  	file->private_data = NULL;
>  	kfree(list);
> +	unlock_kernel();
>  	return res;
>  }
>  
[ ... snip ... ]
> diff --git a/drivers/usb/core/file.c b/drivers/usb/core/file.c
> index bfc6c2e..c3536f1 100644
> --- a/drivers/usb/core/file.c
> +++ b/drivers/usb/core/file.c
> @@ -34,7 +34,6 @@ static int usb_open(struct inode * inode, struct file * file)
>  	int err = -ENODEV;
>  	const struct file_operations *old_fops, *new_fops = NULL;
>  
> -	lock_kernel();
>  	down_read(&minor_rwsem);
>  	c = usb_minors[minor];
>  
> @@ -53,7 +52,6 @@ static int usb_open(struct inode * inode, struct file * file)
>  	fops_put(old_fops);
>   done:
>  	up_read(&minor_rwsem);
> -	unlock_kernel();
>  	return err;
>  }
>  

Hi Oliver,

looking at this a little bit more, this seems wrong, at least for the 
hiddev case. The reason is -- it changes the dependency order of BKL and 
minor_rwsem.

For hiddev, this causes a problem, as you introduced BKL into 
hiddev_connect() exactly because of the proper dependency -- we have the 
BKL there to avoid race after usb_register_dev() -- once the device node 
has been created, open() could happen on it.

The obvious fix -- introducing mutex to guard hiddev_table[] -- is wrong 
work, as usb_open() and usb_register_dev() both take minor_rwsem, thus 
there will be AB-BA deadlock between this mutex and minor_rwsem. 

So you avoided this situation by BKL (as usb_open() has been taking it in 
the right order), but now we are exactly in AB-BA deadlock situation 
because of the reversed dependency between BKL and minor_rwsem.

-- 
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/