linux-kernel - Re: BUG: unable to handle kernel paging request at 6b6b6bcb (v4l2_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: <20110905223102.GA26980@sucs.org>
Date:	Mon, 5 Sep 2011 23:31:03 +0100
From:	Sitsofe Wheeler <sitsofe@...oo.com>
To:	Hans Verkuil <hverkuil@...all.nl>
Cc:	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Dave Young <hidave.darkstar@...il.com>,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
	Guennadi Liakhovetski <g.liakhovetski@....de>,
	Mauro Carvalho Chehab <mchehab@...radead.org>
Subject: Re: BUG: unable to handle kernel paging request at 6b6b6bcb
 (v4l2_device_disconnect+0x11/0x30)

On Mon, Sep 05, 2011 at 12:16:42PM +0200, Hans Verkuil wrote:
> On Monday, September 05, 2011 12:13:26 Hans Verkuil wrote:
> > 
> > The original order is correct, but what I missed is that for drivers
> > that release (free) everything in the videodev release callback the
> > v4l2_device struct is also freed and v4l2_device_put will fail.
> > 
> > To fix this, add this code just before the vdev->release call:
> > 
> > 	/* Do not call v4l2_device_put if there is no release callback set. */
> > 	if (v4l2_dev->release == NULL)
> > 		v4l2_dev = NULL;
> > 
> > If there is no release callback, then the refcounting is pointless
> > anyway.
> > 
> > This should work.
> 
> Note that in the long run using the v4l2_device release callback
> instead of the videodev release is better. But it's a lot of work to
> convert everything so that's long term. I'm quite surprised BTW that
> this bug wasn't found much earlier.

This inline patch fixes the second "poison overwritten" problem so:
Tested-by: Sitsofe Wheeler <sitsofe@...oo.com>

However, it does not prevent the original oops that was reported in the
original message. Yang Ruirui's patch in
https://lkml.org/lkml/2011/9/1/74 seems to be required to resolve
that initial problem - can it be ACK'd? Yang's patch is reproduced
inline below:

For uvc device, dev->vdev.dev is the &intf->dev,
uvc_delete code is as below:
	usb_put_intf(dev->intf);
	usb_put_dev(dev->udev);

	uvc_status_cleanup(dev);
	uvc_ctrl_cleanup_device(dev);

## the intf dev is released above, so below code will oops.

	if (dev->vdev.dev)
		v4l2_device_unregister(&dev->vdev);
Fix it by get_device in v4l2_device_register and put_device in v4l2_device_disconnect
---
 drivers/media/video/v4l2-device.c |    2 ++
 1 file changed, 2 insertions(+)
diff --git a/drivers/media/video/v4l2-device.c b/drivers/media/video/v4l2-device.c
index c72856c..e6a2c3b 100644
--- a/drivers/media/video/v4l2-device.c
+++ b/drivers/media/video/v4l2-device.c
@@ -38,6 +38,7 @@ int v4l2_device_register(struct device *dev, struct v4l2_device *v4l2_dev)
 	mutex_init(&v4l2_dev->ioctl_lock);
 	v4l2_prio_init(&v4l2_dev->prio);
 	kref_init(&v4l2_dev->ref);
+	get_device(dev);
 	v4l2_dev->dev = dev;
 	if (dev == NULL) {
 		/* If dev == NULL, then name must be filled in by the caller */
@@ -93,6 +94,7 @@ void v4l2_device_disconnect(struct v4l2_device *v4l2_dev)
 
 	if (dev_get_drvdata(v4l2_dev->dev) == v4l2_dev)
 		dev_set_drvdata(v4l2_dev->dev, NULL);
+	put_device(v4l2_dev->dev);
 	v4l2_dev->dev = NULL;
 }
 EXPORT_SYMBOL_GPL(v4l2_device_disconnect);

-- 
Sitsofe | http://sucs.org/~sits/
--
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/