diff mbox

List corruption in hidraw_release in 3.11-rc4

Message ID alpine.LNX.2.00.1308070258480.10817@pobox.suse.cz
State New, archived
Delegated to: Jiri Kosina
Headers show

Commit Message

Jiri Kosina Aug. 7, 2013, 1:01 a.m. UTC
On Tue, 6 Aug 2013, Peter Wu wrote:

> While debugging upowerd (with Logitech Unifying receiver via hidraw),
> I came across this list corruption warning.

Peter,

does the patch below fix the problem you are seeing?

---
 drivers/hid/hidraw.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Comments

Peter Wu Aug. 7, 2013, 1:30 p.m. UTC | #1
On Wednesday 07 August 2013 03:01:26 Jiri Kosina wrote:
> On Tue, 6 Aug 2013, Peter Wu wrote:
> > While debugging upowerd (with Logitech Unifying receiver via hidraw),
> > I came across this list corruption warning.
> 
> Peter,
> 
> does the patch below fix the problem you are seeing?
That one is already in 3.11-rc4 as far as I can see. Also, that code can 
probably simplified by moving the mutex_unlock after the out label, removing 
the need to duplicate the mutex_unlock.

Remember what I said about "no Oopses"? Well, it turned out that several 
memory structures were damaged which causes a general protection fault in 
sock_alloc_inode and other places.

I managed to create a program that can reproduce this bug 100% in a QEMU 
virtual machine with a Logitech USB receiver passed to it.

qemu-system-x86_64 -enable-kvm -m 1G -usb -usbdevice host:046d:c52b
(pass -kernel, -initrd, -append as needed)

Copy hidraw-test to initrd, boot QEMU and run `hidraw-test`. Result: instant
(= +/- 2 seconds) crash.

I have applied Manoj's patch[1] on top of 3.11-rc4 which seem to fix the issue. 
One observation is that the new device is named /dev/hidraw1 instead of 
/dev/hidraw0. Example:

f(){ hidraw-test /dev/hidraw$1 usb1;}
# needed for 3.11-rc4
f 1; f 1 # crash
# needed for 3.11-rc4 + patch
f 1; f 2 # ok

Regards,
Peter

 [1]: http://lkml.org/lkml/2013/7/22/248
--
/* cc hidraw-test.c -o hidraw-test
 * hidraw-test /dev/hidraw0 usb1; hidraw-test /dev/hidraw0 usb1;
 */
#include <unistd.h>
#include <fcntl.h>
#include <stdio.h>
#include <errno.h>
#include <string.h>
#include <stdlib.h>

int open_and_write(const char *path, const char *data) {
	int sfd, r;

	sfd = open(path, O_WRONLY);
	if (sfd < 0) {
		perror(path);
		return 1;
	}

	r = write(sfd, data, strlen(data));
	if (r < 0) {
		fprintf(stderr, "write(%s, %s): %s\n",
			path, data, strerror(errno));
		return 1;
	}
	close(sfd);
	return 0;
}

int dork(const char *hiddev, const char *name) {
	int fd;
	char c;

	fd = open(hiddev, O_RDWR | O_NONBLOCK);
	if (fd < 0) {
		perror("open");
		return 1;
	}

	if (open_and_write("/sys/bus/usb/drivers/usb/unbind", name))
		return 1;

	// does not make a difference
	//sleep(1);

	if (open_and_write("/sys/bus/usb/drivers/usb/bind", name))
		return 1;

	// allow devices to get discovered
	sleep(1);

	printf("read() = %zi\n", read(fd, &c, 1)); perror("read");
	close(fd);
	return 0;
}

int main(int argc, char **argv) {
	if (argc < 3) {
		fprintf(stderr, "Usage: %s /dev/hidrawN usbN\n", *argv);
		return 1;
	}

	system("modprobe -v usbhid");
	system("modprobe -v hid-logitech-dj");

	dork(argv[1], argv[2]);

	return 0;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiri Kosina Aug. 7, 2013, 1:34 p.m. UTC | #2
On Wed, 7 Aug 2013, Peter Wu wrote:

> > does the patch below fix the problem you are seeing?
> That one is already in 3.11-rc4 as far as I can see. Also, that code can 
> probably simplified by moving the mutex_unlock after the out label, removing 
> the need to duplicate the mutex_unlock.
> 
> Remember what I said about "no Oopses"? Well, it turned out that several 
> memory structures were damaged which causes a general protection fault in 
> sock_alloc_inode and other places.
> 
> I managed to create a program that can reproduce this bug 100% in a QEMU 
> virtual machine with a Logitech USB receiver passed to it.
> 
> qemu-system-x86_64 -enable-kvm -m 1G -usb -usbdevice host:046d:c52b
> (pass -kernel, -initrd, -append as needed)
> 
> Copy hidraw-test to initrd, boot QEMU and run `hidraw-test`. Result: instant
> (= +/- 2 seconds) crash.
> 
> I have applied Manoj's patch[1] on top of 3.11-rc4 which seem to fix the issue. 
> One observation is that the new device is named /dev/hidraw1 instead of 
> /dev/hidraw0. Example:
> 
> f(){ hidraw-test /dev/hidraw$1 usb1;}
> # needed for 3.11-rc4
> f 1; f 1 # crash
> # needed for 3.11-rc4 + patch
> f 1; f 2 # ok
> 
> Regards,
> Peter
> 
>  [1]: http://lkml.org/lkml/2013/7/22/248

That one I am still reviewing ... can I add your Tested-by: to it when 
I'll be applying it and pushing to Linus?

Thanks.

> --
> /* cc hidraw-test.c -o hidraw-test
>  * hidraw-test /dev/hidraw0 usb1; hidraw-test /dev/hidraw0 usb1;
>  */
> #include <unistd.h>
> #include <fcntl.h>
> #include <stdio.h>
> #include <errno.h>
> #include <string.h>
> #include <stdlib.h>
> 
> int open_and_write(const char *path, const char *data) {
> 	int sfd, r;
> 
> 	sfd = open(path, O_WRONLY);
> 	if (sfd < 0) {
> 		perror(path);
> 		return 1;
> 	}
> 
> 	r = write(sfd, data, strlen(data));
> 	if (r < 0) {
> 		fprintf(stderr, "write(%s, %s): %s\n",
> 			path, data, strerror(errno));
> 		return 1;
> 	}
> 	close(sfd);
> 	return 0;
> }
> 
> int dork(const char *hiddev, const char *name) {
> 	int fd;
> 	char c;
> 
> 	fd = open(hiddev, O_RDWR | O_NONBLOCK);
> 	if (fd < 0) {
> 		perror("open");
> 		return 1;
> 	}
> 
> 	if (open_and_write("/sys/bus/usb/drivers/usb/unbind", name))
> 		return 1;
> 
> 	// does not make a difference
> 	//sleep(1);
> 
> 	if (open_and_write("/sys/bus/usb/drivers/usb/bind", name))
> 		return 1;
> 
> 	// allow devices to get discovered
> 	sleep(1);
> 
> 	printf("read() = %zi\n", read(fd, &c, 1)); perror("read");
> 	close(fd);
> 	return 0;
> }
> 
> int main(int argc, char **argv) {
> 	if (argc < 3) {
> 		fprintf(stderr, "Usage: %s /dev/hidrawN usbN\n", *argv);
> 		return 1;
> 	}
> 
> 	system("modprobe -v usbhid");
> 	system("modprobe -v hid-logitech-dj");
> 
> 	dork(argv[1], argv[2]);
> 
> 	return 0;
> }
>
Manoj Chourasia Aug. 7, 2013, 3:06 p.m. UTC | #3
Hi Peter,

The patch I posted was solving slab memory corruption issue which was occurring because of the race in device disconnect and device release. We found the some of the device data structure being used after free. Later we figure out the patch which was reverted earlier was solving our issue but there was still some slab memory corruption.  That was due to reason that list delete of the device was called after freeing the hidraw. I protect drop_ref by mutex lock and also delete the list before calling drop_ref that solve the issue. If you are seeing memory corruption then the patch could solve your issue.

Regards
-Manoj

-----Original Message-----
From: Jiri Kosina [mailto:jkosina@suse.cz] 
Sent: Wednesday, August 07, 2013 7:04 PM
To: Peter Wu
Cc: linux-input@vger.kernel.org; Manoj Chourasia; linux-kernel@vger.kernel.org; alnovak@suse.cz
Subject: Re: List corruption in hidraw_release in 3.11-rc4

On Wed, 7 Aug 2013, Peter Wu wrote:

> > does the patch below fix the problem you are seeing?
> That one is already in 3.11-rc4 as far as I can see. Also, that code 
> can probably simplified by moving the mutex_unlock after the out 
> label, removing the need to duplicate the mutex_unlock.
> 
> Remember what I said about "no Oopses"? Well, it turned out that 
> several memory structures were damaged which causes a general 
> protection fault in sock_alloc_inode and other places.
> 
> I managed to create a program that can reproduce this bug 100% in a 
> QEMU virtual machine with a Logitech USB receiver passed to it.
> 
> qemu-system-x86_64 -enable-kvm -m 1G -usb -usbdevice host:046d:c52b 
> (pass -kernel, -initrd, -append as needed)
> 
> Copy hidraw-test to initrd, boot QEMU and run `hidraw-test`. Result: 
> instant (= +/- 2 seconds) crash.
> 
> I have applied Manoj's patch[1] on top of 3.11-rc4 which seem to fix the issue. 
> One observation is that the new device is named /dev/hidraw1 instead 
> of /dev/hidraw0. Example:
> 
> f(){ hidraw-test /dev/hidraw$1 usb1;}
> # needed for 3.11-rc4
> f 1; f 1 # crash
> # needed for 3.11-rc4 + patch
> f 1; f 2 # ok
> 
> Regards,
> Peter
> 
>  [1]: http://lkml.org/lkml/2013/7/22/248

That one I am still reviewing ... can I add your Tested-by: to it when I'll be applying it and pushing to Linus?

Thanks.

> --
> /* cc hidraw-test.c -o hidraw-test
>  * hidraw-test /dev/hidraw0 usb1; hidraw-test /dev/hidraw0 usb1;  */ 
> #include <unistd.h> #include <fcntl.h> #include <stdio.h> #include 
> <errno.h> #include <string.h> #include <stdlib.h>
> 
> int open_and_write(const char *path, const char *data) {
> 	int sfd, r;
> 
> 	sfd = open(path, O_WRONLY);
> 	if (sfd < 0) {
> 		perror(path);
> 		return 1;
> 	}
> 
> 	r = write(sfd, data, strlen(data));
> 	if (r < 0) {
> 		fprintf(stderr, "write(%s, %s): %s\n",
> 			path, data, strerror(errno));
> 		return 1;
> 	}
> 	close(sfd);
> 	return 0;
> }
> 
> int dork(const char *hiddev, const char *name) {
> 	int fd;
> 	char c;
> 
> 	fd = open(hiddev, O_RDWR | O_NONBLOCK);
> 	if (fd < 0) {
> 		perror("open");
> 		return 1;
> 	}
> 
> 	if (open_and_write("/sys/bus/usb/drivers/usb/unbind", name))
> 		return 1;
> 
> 	// does not make a difference
> 	//sleep(1);
> 
> 	if (open_and_write("/sys/bus/usb/drivers/usb/bind", name))
> 		return 1;
> 
> 	// allow devices to get discovered
> 	sleep(1);
> 
> 	printf("read() = %zi\n", read(fd, &c, 1)); perror("read");
> 	close(fd);
> 	return 0;
> }
> 
> int main(int argc, char **argv) {
> 	if (argc < 3) {
> 		fprintf(stderr, "Usage: %s /dev/hidrawN usbN\n", *argv);
> 		return 1;
> 	}
> 
> 	system("modprobe -v usbhid");
> 	system("modprobe -v hid-logitech-dj");
> 
> 	dork(argv[1], argv[2]);
> 
> 	return 0;
> }
> 

--
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Wu Aug. 7, 2013, 10:28 p.m. UTC | #4
On Wednesday 07 August 2013 15:34:08 Jiri Kosina wrote:
> On Wed, 7 Aug 2013, Peter Wu wrote:
> > [..]
> > I have applied Manoj's patch[1] on top of 3.11-rc4 which seem to fix the
> > issue. One observation is that the new device is named /dev/hidraw1
> > instead of /dev/hidraw0. Example:
> > 
> > f(){ hidraw-test /dev/hidraw$1 usb1;}
> > # needed for 3.11-rc4
> > f 1; f 1 # crash
> > # needed for 3.11-rc4 + patch
> > f 1; f 2 # ok
> > 
> > Regards,
> > Peter
> > 
> >  [1]: http://lkml.org/lkml/2013/7/22/248
> 
> That one I am still reviewing ... can I add your Tested-by: to it when
> I'll be applying it and pushing to Linus?

Sure, once you accept it you can add:
Tested-by: Peter Wu <lekensteyn@gmail.com>

While you are at it, are the other functions also safe? (i.e. hidraw_poll, 
that one is not protected by any locks?)

Regards,
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index a745163..6f1feb2 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -518,7 +518,6 @@  int hidraw_connect(struct hid_device *hid)
 		goto out;
 	}
 
-	mutex_unlock(&minors_lock);
 	init_waitqueue_head(&dev->wait);
 	INIT_LIST_HEAD(&dev->list);
 
@@ -528,6 +527,7 @@  int hidraw_connect(struct hid_device *hid)
 	dev->exist = 1;
 	hid->hidraw = dev;
 
+	mutex_unlock(&minors_lock);
 out:
 	return result;