linux-kernel - Re: [PATCH v9 2/2] uio: Fix uio driver to refcount device

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]

Message-ID: <20150323204145.GA22203@kroah.com>
Date:	Mon, 23 Mar 2015 21:41:45 +0100
From:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:	Brian Russell <brian.russell@...cade.com>
Cc:	"Hans J. Koch" <hjk@...sjkoch.de>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v9 2/2] uio: Fix uio driver to refcount device

On Fri, Mar 20, 2015 at 02:54:44PM +0000, Brian Russell wrote:
> Protect uio driver from its owner being unplugged while there are open fds.
> Embed struct device in struct uio_device, use refcounting on device, free
> uio_device on release.
> info struct passed in uio_register_device can be freed on unregister, so null
> out the field in uio_unregister_device and check accesses.

That's really not protecting anything except heavy-handed problems...

Look at the code:

> @@ -493,7 +499,7 @@ static unsigned int uio_poll(struct file *filep, poll_table *wait)
>  	struct uio_listener *listener = filep->private_data;
>  	struct uio_device *idev = listener->dev;
>  
> -	if (!idev->info->irq)
> +	if (!idev->info || !idev->info->irq)
>  		return -EIO;
>  

Great, you checked the irq value, but what if it changes the very next
line:

>  	poll_wait(filep, &idev->wait, wait);

Or any other line within this function?  Or any other function that you
try to check the value for in the beginning...

This really isn't protecting anything "properly", sorry.  Either we
don't care about it (hint, I don't think we really do), or we need to
properly lock things and check, and protect, things that way.

Please do the first one, as the reference count should be all that we
need to care about here.

Sorry I missed this on the previous review, just realized it now this
time around.

thanks,

greg k-h
--
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/