public inbox for kernel-janitors@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] staging: line6: fix use-after-free bug
@ 2013-01-18 21:52 Markus Grabner
  2013-01-19  0:57 ` Greg Kroah-Hartman
                   ` (8 more replies)
  0 siblings, 9 replies; 10+ messages in thread
From: Markus Grabner @ 2013-01-18 21:52 UTC (permalink / raw)
  To: kernel-janitors

The function "line6_send_raw_message_async" now has an additional argument
"bool copy", which indicates whether the supplied buffer should be copied into
a dynamically allocated block of memory. The copy flag is also stored in the
"message" struct such that the temporary memory can be freed when appropriate
without intervention of the caller.

Reported-by: Stefan Hajnoczi <stefanha@gmail.com>
Signed-off-by: Markus Grabner <grabner@icg.tugraz.at>
---
 drivers/staging/line6/driver.c |   57 ++++++++++++++++++++++------------------
 drivers/staging/line6/driver.h |    3 ++-
 drivers/staging/line6/variax.c |    2 +-
 3 files changed, 35 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/line6/driver.c b/drivers/staging/line6/driver.c
index 1b05633..1e6e0da 100644
--- a/drivers/staging/line6/driver.c
+++ b/drivers/staging/line6/driver.c
@@ -103,6 +103,7 @@ struct message {
 	const char *buffer;
 	int size;
 	int done;
+	bool copy;
 };
 
 /*
@@ -216,6 +217,9 @@ static void line6_async_request_sent(struct urb *urb)
 	struct message *msg = (struct message *)urb->context;
 
 	if (msg->done >= msg->size) {
+		if (msg->copy)
+			kfree(msg->buffer);
+
 		usb_free_urb(urb);
 		kfree(msg);
 	} else
@@ -267,26 +271,29 @@ void line6_start_timer(struct timer_list *timer, unsigned int msecs,
 	Asynchronously send raw message.
 */
 int line6_send_raw_message_async(struct usb_line6 *line6, const char *buffer,
-				 int size)
+				 int size, bool copy)
 {
-	struct message *msg;
-	struct urb *urb;
+	struct message *msg = NULL;
+	struct urb *urb = NULL;
 
 	/* create message: */
 	msg = kmalloc(sizeof(struct message), GFP_ATOMIC);
 
-	if (msg = NULL) {
-		dev_err(line6->ifcdev, "Out of memory\n");
-		return -ENOMEM;
-	}
+	if (msg = NULL)
+		goto out_of_memory;
 
 	/* create URB: */
 	urb = usb_alloc_urb(0, GFP_ATOMIC);
 
-	if (urb = NULL) {
-		kfree(msg);
-		dev_err(line6->ifcdev, "Out of memory\n");
-		return -ENOMEM;
+	if (urb = NULL)
+		goto out_of_memory;
+
+	/* copy buffer if requested: */
+	if (copy) {
+		buffer = kmemdup(buffer, size, GFP_ATOMIC);
+		
+		if (buffer = NULL)
+			goto out_of_memory;
 	}
 
 	/* set message data: */
@@ -294,9 +301,20 @@ int line6_send_raw_message_async(struct usb_line6 *line6, const char *buffer,
 	msg->buffer = buffer;
 	msg->size = size;
 	msg->done = 0;
+	msg->copy = copy;
 
 	/* start sending: */
 	return line6_send_raw_message_async_part(msg, urb);
+
+out_of_memory:
+	if (urb != NULL)
+		usb_free_urb(urb);
+
+	if (msg != NULL)
+		kfree(msg);
+
+	dev_err(line6->ifcdev, "Out of memory");
+	return -ENOMEM;
 }
 
 /*
@@ -304,20 +322,9 @@ int line6_send_raw_message_async(struct usb_line6 *line6, const char *buffer,
 */
 int line6_version_request_async(struct usb_line6 *line6)
 {
-	char *buffer;
-	int retval;
-
-	buffer = kmemdup(line6_request_version,
-			sizeof(line6_request_version), GFP_ATOMIC);
-	if (buffer = NULL) {
-		dev_err(line6->ifcdev, "Out of memory");
-		return -ENOMEM;
-	}
-
-	retval = line6_send_raw_message_async(line6, buffer,
-					      sizeof(line6_request_version));
-	kfree(buffer);
-	return retval;
+	return line6_send_raw_message_async(line6, line6_request_version,
+					    sizeof(line6_request_version),
+					    true);
 }
 
 /*
diff --git a/drivers/staging/line6/driver.h b/drivers/staging/line6/driver.h
index 2e0f134..8120cdf 100644
--- a/drivers/staging/line6/driver.h
+++ b/drivers/staging/line6/driver.h
@@ -205,7 +205,8 @@ extern int line6_send_program(struct usb_line6 *line6, u8 value);
 extern int line6_send_raw_message(struct usb_line6 *line6, const char *buffer,
 				  int size);
 extern int line6_send_raw_message_async(struct usb_line6 *line6,
-					const char *buffer, int size);
+					const char *buffer, int size,
+					bool copy);
 extern int line6_send_sysex_message(struct usb_line6 *line6,
 				    const char *buffer, int size);
 extern ssize_t line6_set_raw(struct device *dev, struct device_attribute *attr,
diff --git a/drivers/staging/line6/variax.c b/drivers/staging/line6/variax.c
index 4fca58f..47753d5 100644
--- a/drivers/staging/line6/variax.c
+++ b/drivers/staging/line6/variax.c
@@ -47,7 +47,7 @@ static void variax_activate_async(struct usb_line6_variax *variax, int a)
 {
 	variax->buffer_activate[VARIAX_OFFSET_ACTIVATE] = a;
 	line6_send_raw_message_async(&variax->line6, variax->buffer_activate,
-				     sizeof(variax_activate));
+				     sizeof(variax_activate), false);
 }
 
 /*
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] staging: line6: fix use-after-free bug
  2013-01-18 21:52 [PATCH] staging: line6: fix use-after-free bug Markus Grabner
@ 2013-01-19  0:57 ` Greg Kroah-Hartman
  2013-01-19 21:55 ` Markus Grabner
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Greg Kroah-Hartman @ 2013-01-19  0:57 UTC (permalink / raw)
  To: kernel-janitors

On Fri, Jan 18, 2013 at 10:52:14PM +0100, Markus Grabner wrote:
> The function "line6_send_raw_message_async" now has an additional argument
> "bool copy", which indicates whether the supplied buffer should be copied into
> a dynamically allocated block of memory. The copy flag is also stored in the
> "message" struct such that the temporary memory can be freed when appropriate
> without intervention of the caller.

Why do this?  Why not either always copy it, or always not?  That would
make it simpler overall, right?

What is this fixing?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] staging: line6: fix use-after-free bug
  2013-01-18 21:52 [PATCH] staging: line6: fix use-after-free bug Markus Grabner
  2013-01-19  0:57 ` Greg Kroah-Hartman
@ 2013-01-19 21:55 ` Markus Grabner
  2013-01-20 17:11 ` Greg Kroah-Hartman
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Markus Grabner @ 2013-01-19 21:55 UTC (permalink / raw)
  To: kernel-janitors

Am Freitag, 18. Januar 2013, 16:57:31 schrieb Greg Kroah-Hartman:
> On Fri, Jan 18, 2013 at 10:52:14PM +0100, Markus Grabner wrote:
> > The function "line6_send_raw_message_async" now has an additional argument
> > "bool copy", which indicates whether the supplied buffer should be copied
> > into a dynamically allocated block of memory. The copy flag is also
> > stored in the "message" struct such that the temporary memory can be
> > freed when appropriate without intervention of the caller.
> 
> Why do this?  Why not either always copy it, or always not?
Some messages are sent to the device which have no parameters, they are 
declared at global scope as constant byte arrays and therefore must be copied 
into a dynamically allocated block of memory in order to be sent over the USB 
interface. On the other hand, there are messages which do have parameters and 
which are composed in dynamically allocated memory and can therefore directly 
be sent without copying.

> What is this fixing?
Two users reported to me independently that the driver doesn't work for them. 
I couldn't reproduce the problem since it seems to be triggered by subtle 
timing issues in the system, but after some further investigations, the 
kfree() of the message buffer immediately after submitting the message for 
asynchronous transmission was clearly identified as the reason for the driver 
not working. The patch puts the kfree() at the right place and (hopefully) 
prevents incorrect use of the new buffer copy feature. The patch is tested by 
me and the users who initially reported the bug, and they confirmed that the 
issue is fixed for them.

If anybody has a better idea how to fix this, please go ahead! The patch might 
also become obsolete in the future due to refactoring. But currently there is 
a bug which prevents some people from using the driver at all, and this should 
be fixed soon IMO.

	Kind regards,
		Markus


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] staging: line6: fix use-after-free bug
  2013-01-18 21:52 [PATCH] staging: line6: fix use-after-free bug Markus Grabner
  2013-01-19  0:57 ` Greg Kroah-Hartman
  2013-01-19 21:55 ` Markus Grabner
@ 2013-01-20 17:11 ` Greg Kroah-Hartman
  2013-01-20 22:51 ` Markus Grabner
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Greg Kroah-Hartman @ 2013-01-20 17:11 UTC (permalink / raw)
  To: kernel-janitors

On Sat, Jan 19, 2013 at 10:55:29PM +0100, Markus Grabner wrote:
> Am Freitag, 18. Januar 2013, 16:57:31 schrieb Greg Kroah-Hartman:
> > On Fri, Jan 18, 2013 at 10:52:14PM +0100, Markus Grabner wrote:
> > > The function "line6_send_raw_message_async" now has an additional argument
> > > "bool copy", which indicates whether the supplied buffer should be copied
> > > into a dynamically allocated block of memory. The copy flag is also
> > > stored in the "message" struct such that the temporary memory can be
> > > freed when appropriate without intervention of the caller.
> > 
> > Why do this?  Why not either always copy it, or always not?
> Some messages are sent to the device which have no parameters, they are 
> declared at global scope as constant byte arrays and therefore must be copied 
> into a dynamically allocated block of memory in order to be sent over the USB 
> interface. On the other hand, there are messages which do have parameters and 
> which are composed in dynamically allocated memory and can therefore directly 
> be sent without copying.

Then if you always copy the memory, and "own" it after the call, you
should be fine, right?

> > What is this fixing?
> Two users reported to me independently that the driver doesn't work for them. 
> I couldn't reproduce the problem since it seems to be triggered by subtle 
> timing issues in the system, but after some further investigations, the 
> kfree() of the message buffer immediately after submitting the message for 
> asynchronous transmission was clearly identified as the reason for the driver 
> not working. The patch puts the kfree() at the right place and (hopefully) 
> prevents incorrect use of the new buffer copy feature. The patch is tested by 
> me and the users who initially reported the bug, and they confirmed that the 
> issue is fixed for them.
> 
> If anybody has a better idea how to fix this, please go ahead! The patch might 
> also become obsolete in the future due to refactoring. But currently there is 
> a bug which prevents some people from using the driver at all, and this should 
> be fixed soon IMO.

I agree, it should be fixed, but having the code always do the copy and
manage the memory, and not have the crazy "flag" option, should solve
the bug for everyone.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] staging: line6: fix use-after-free bug
  2013-01-18 21:52 [PATCH] staging: line6: fix use-after-free bug Markus Grabner
                   ` (2 preceding siblings ...)
  2013-01-20 17:11 ` Greg Kroah-Hartman
@ 2013-01-20 22:51 ` Markus Grabner
  2013-01-20 23:04 ` Greg Kroah-Hartman
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Markus Grabner @ 2013-01-20 22:51 UTC (permalink / raw)
  To: kernel-janitors

Am Sonntag, 20. Januar 2013, 09:11:50 schrieb Greg Kroah-Hartman:
> On Sat, Jan 19, 2013 at 10:55:29PM +0100, Markus Grabner wrote:
> > Am Freitag, 18. Januar 2013, 16:57:31 schrieb Greg Kroah-Hartman:
> > > On Fri, Jan 18, 2013 at 10:52:14PM +0100, Markus Grabner wrote:
> > > > The function "line6_send_raw_message_async" now has an additional
> > > > argument
> > > > "bool copy", which indicates whether the supplied buffer should be
> > > > copied
> > > > into a dynamically allocated block of memory. The copy flag is also
> > > > stored in the "message" struct such that the temporary memory can be
> > > > freed when appropriate without intervention of the caller.
> > > 
> > > Why do this?  Why not either always copy it, or always not?
> > 
> > Some messages are sent to the device which have no parameters, they are
> > declared at global scope as constant byte arrays and therefore must be
> > copied into a dynamically allocated block of memory in order to be sent
> > over the USB interface. On the other hand, there are messages which do
> > have parameters and which are composed in dynamically allocated memory
> > and can therefore directly be sent without copying.
> 
> Then if you always copy the memory, and "own" it after the call, you
> should be fine, right?
> 
> > > What is this fixing?
> > 
> > Two users reported to me independently that the driver doesn't work for
> > them. I couldn't reproduce the problem since it seems to be triggered by
> > subtle timing issues in the system, but after some further
> > investigations, the kfree() of the message buffer immediately after
> > submitting the message for asynchronous transmission was clearly
> > identified as the reason for the driver not working. The patch puts the
> > kfree() at the right place and (hopefully) prevents incorrect use of the
> > new buffer copy feature. The patch is tested by me and the users who
> > initially reported the bug, and they confirmed that the issue is fixed
> > for them.
> > 
> > If anybody has a better idea how to fix this, please go ahead! The patch
> > might also become obsolete in the future due to refactoring. But
> > currently there is a bug which prevents some people from using the driver
> > at all, and this should be fixed soon IMO.
> 
> I agree, it should be fixed, but having the code always do the copy and
> manage the memory, and not have the crazy "flag" option, should solve
> the bug for everyone.
Removing the flag saves three lines of code, keeping the flag saves a tiny 
amount of time and memory, so it's not really worth a lengthy discussion, and 
I actually don't care much. I will focus on the user space library I'm 
currently working on since it will make much of the MIDI-related Line6 kernel 
driver code obsolete.

On the other hand, would it be possible in usb_submit_urb to detect whether 
the data pointer provided to it is suitable for DMA, and if not, transparently 
make a copy and free it if no longer used (or, by the same argument as above, 
just always make a copy)?

	Kind regards,
		Markus


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] staging: line6: fix use-after-free bug
  2013-01-18 21:52 [PATCH] staging: line6: fix use-after-free bug Markus Grabner
                   ` (3 preceding siblings ...)
  2013-01-20 22:51 ` Markus Grabner
@ 2013-01-20 23:04 ` Greg Kroah-Hartman
  2013-06-03 22:49 ` Markus Grabner
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Greg Kroah-Hartman @ 2013-01-20 23:04 UTC (permalink / raw)
  To: kernel-janitors

On Sun, Jan 20, 2013 at 11:51:36PM +0100, Markus Grabner wrote:
> Am Sonntag, 20. Januar 2013, 09:11:50 schrieb Greg Kroah-Hartman:
> > On Sat, Jan 19, 2013 at 10:55:29PM +0100, Markus Grabner wrote:
> > > Am Freitag, 18. Januar 2013, 16:57:31 schrieb Greg Kroah-Hartman:
> > > > On Fri, Jan 18, 2013 at 10:52:14PM +0100, Markus Grabner wrote:
> > > > > The function "line6_send_raw_message_async" now has an additional
> > > > > argument
> > > > > "bool copy", which indicates whether the supplied buffer should be
> > > > > copied
> > > > > into a dynamically allocated block of memory. The copy flag is also
> > > > > stored in the "message" struct such that the temporary memory can be
> > > > > freed when appropriate without intervention of the caller.
> > > > 
> > > > Why do this?  Why not either always copy it, or always not?
> > > 
> > > Some messages are sent to the device which have no parameters, they are
> > > declared at global scope as constant byte arrays and therefore must be
> > > copied into a dynamically allocated block of memory in order to be sent
> > > over the USB interface. On the other hand, there are messages which do
> > > have parameters and which are composed in dynamically allocated memory
> > > and can therefore directly be sent without copying.
> > 
> > Then if you always copy the memory, and "own" it after the call, you
> > should be fine, right?
> > 
> > > > What is this fixing?
> > > 
> > > Two users reported to me independently that the driver doesn't work for
> > > them. I couldn't reproduce the problem since it seems to be triggered by
> > > subtle timing issues in the system, but after some further
> > > investigations, the kfree() of the message buffer immediately after
> > > submitting the message for asynchronous transmission was clearly
> > > identified as the reason for the driver not working. The patch puts the
> > > kfree() at the right place and (hopefully) prevents incorrect use of the
> > > new buffer copy feature. The patch is tested by me and the users who
> > > initially reported the bug, and they confirmed that the issue is fixed
> > > for them.
> > > 
> > > If anybody has a better idea how to fix this, please go ahead! The patch
> > > might also become obsolete in the future due to refactoring. But
> > > currently there is a bug which prevents some people from using the driver
> > > at all, and this should be fixed soon IMO.
> > 
> > I agree, it should be fixed, but having the code always do the copy and
> > manage the memory, and not have the crazy "flag" option, should solve
> > the bug for everyone.
> Removing the flag saves three lines of code, keeping the flag saves a tiny 
> amount of time and memory, so it's not really worth a lengthy discussion, and 
> I actually don't care much. I will focus on the user space library I'm 
> currently working on since it will make much of the MIDI-related Line6 kernel 
> driver code obsolete.

Removing the flag makes the api easier to understand :)

And given that this is a USB device, speed usually isn't an issue here.

> On the other hand, would it be possible in usb_submit_urb to detect whether 
> the data pointer provided to it is suitable for DMA, and if not, transparently 
> make a copy and free it if no longer used (or, by the same argument as above, 
> just always make a copy)?

Yes, it has been able to do that for a very long time, I don't have
access to the kernel tree at the moment, but look in usb.h it should
tell you how to do it there.

greg k-h

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] staging: line6: fix use-after-free bug
  2013-01-18 21:52 [PATCH] staging: line6: fix use-after-free bug Markus Grabner
                   ` (4 preceding siblings ...)
  2013-01-20 23:04 ` Greg Kroah-Hartman
@ 2013-06-03 22:49 ` Markus Grabner
  2013-06-03 23:08 ` Greg Kroah-Hartman
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Markus Grabner @ 2013-06-03 22:49 UTC (permalink / raw)
  To: kernel-janitors

On Sunday 20 January 2013 23:51:36 Markus Grabner wrote:
> Am Sonntag, 20. Januar 2013, 09:11:50 schrieb Greg Kroah-Hartman:
> > On Sat, Jan 19, 2013 at 10:55:29PM +0100, Markus Grabner wrote:
> > > Am Freitag, 18. Januar 2013, 16:57:31 schrieb Greg Kroah-Hartman:
> > > > On Fri, Jan 18, 2013 at 10:52:14PM +0100, Markus Grabner wrote:
> > > > > The function "line6_send_raw_message_async" now has an additional
> > > > > argument
> > > > > "bool copy", which indicates whether the supplied buffer should be
> > > > > copied
> > > > > into a dynamically allocated block of memory. The copy flag is also
> > > > > stored in the "message" struct such that the temporary memory can be
> > > > > freed when appropriate without intervention of the caller.
> > > > 
> > > > Why do this?  Why not either always copy it, or always not?
> > > 
> > > Some messages are sent to the device which have no parameters, they are
> > > declared at global scope as constant byte arrays and therefore must be
> > > copied into a dynamically allocated block of memory in order to be sent
> > > over the USB interface. On the other hand, there are messages which do
> > > have parameters and which are composed in dynamically allocated memory
> > > and can therefore directly be sent without copying.
> > 
> > Then if you always copy the memory, and "own" it after the call, you
> > should be fine, right?
> > 
> > > > What is this fixing?
> > > 
> > > Two users reported to me independently that the driver doesn't work for
> > > them. I couldn't reproduce the problem since it seems to be triggered by
> > > subtle timing issues in the system, but after some further
> > > investigations, the kfree() of the message buffer immediately after
> > > submitting the message for asynchronous transmission was clearly
> > > identified as the reason for the driver not working. The patch puts the
> > > kfree() at the right place and (hopefully) prevents incorrect use of the
> > > new buffer copy feature. The patch is tested by me and the users who
> > > initially reported the bug, and they confirmed that the issue is fixed
> > > for them.
> > > 
> > > If anybody has a better idea how to fix this, please go ahead! The patch
> > > might also become obsolete in the future due to refactoring. But
> > > currently there is a bug which prevents some people from using the
> > > driver
> > > at all, and this should be fixed soon IMO.
> > 
> > I agree, it should be fixed, but having the code always do the copy and
> > manage the memory, and not have the crazy "flag" option, should solve
> > the bug for everyone.
> 
> Removing the flag saves three lines of code, keeping the flag saves a tiny
> amount of time and memory, so it's not really worth a lengthy discussion,
> and I actually don't care much. I will focus on the user space library I'm
> currently working on since it will make much of the MIDI-related Line6
> kernel driver code obsolete.
I'm currently re-investigating this, and I have been informed by users that 
some newer Line6 devices talk a device-specific protocol over USB which is 
different from the MIDI standard and should therefore not be mapped to a 
virtual MIDI device. This raises some additional questions:

*) The easiest way to deal with this would be to use libusb in user space to 
exchange data with the device. However, as far as I understand, if the device 
is being used as an ALSA sound card (i.e., the kernel driver has claimed the 
USB interface to access isochronous endpoints), libusb can't access interrupt 
endpoints of the same interface at the same time since it can't claim the 
interface while it is claimed by the kernel. Is this correct?

*) If shared kernel/user space access to the same USB interface is not 
possible as discussed above, what would be the preferred interface for user 
space applications to talk to the kernel driver? I think netlink is a good 
candidate, or do you have any other suggestions?

A non-MIDI solution would have the additional benefit that all device 
initialization stuff could be moved to user space, including the code related 
to the copy flag discussed in January.

What do you think?

	Thanks & kind regards,
		Markus


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] staging: line6: fix use-after-free bug
  2013-01-18 21:52 [PATCH] staging: line6: fix use-after-free bug Markus Grabner
                   ` (5 preceding siblings ...)
  2013-06-03 22:49 ` Markus Grabner
@ 2013-06-03 23:08 ` Greg Kroah-Hartman
  2013-06-04 20:10 ` Markus Grabner
  2013-06-19 16:40 ` Greg Kroah-Hartman
  8 siblings, 0 replies; 10+ messages in thread
From: Greg Kroah-Hartman @ 2013-06-03 23:08 UTC (permalink / raw)
  To: kernel-janitors

On Tue, Jun 04, 2013 at 12:49:36AM +0200, Markus Grabner wrote:
> On Sunday 20 January 2013 23:51:36 Markus Grabner wrote:
> > Am Sonntag, 20. Januar 2013, 09:11:50 schrieb Greg Kroah-Hartman:
> > > On Sat, Jan 19, 2013 at 10:55:29PM +0100, Markus Grabner wrote:
> > > > Am Freitag, 18. Januar 2013, 16:57:31 schrieb Greg Kroah-Hartman:
> > > > > On Fri, Jan 18, 2013 at 10:52:14PM +0100, Markus Grabner wrote:
> > > > > > The function "line6_send_raw_message_async" now has an additional
> > > > > > argument
> > > > > > "bool copy", which indicates whether the supplied buffer should be
> > > > > > copied
> > > > > > into a dynamically allocated block of memory. The copy flag is also
> > > > > > stored in the "message" struct such that the temporary memory can be
> > > > > > freed when appropriate without intervention of the caller.
> > > > > 
> > > > > Why do this?  Why not either always copy it, or always not?
> > > > 
> > > > Some messages are sent to the device which have no parameters, they are
> > > > declared at global scope as constant byte arrays and therefore must be
> > > > copied into a dynamically allocated block of memory in order to be sent
> > > > over the USB interface. On the other hand, there are messages which do
> > > > have parameters and which are composed in dynamically allocated memory
> > > > and can therefore directly be sent without copying.
> > > 
> > > Then if you always copy the memory, and "own" it after the call, you
> > > should be fine, right?
> > > 
> > > > > What is this fixing?
> > > > 
> > > > Two users reported to me independently that the driver doesn't work for
> > > > them. I couldn't reproduce the problem since it seems to be triggered by
> > > > subtle timing issues in the system, but after some further
> > > > investigations, the kfree() of the message buffer immediately after
> > > > submitting the message for asynchronous transmission was clearly
> > > > identified as the reason for the driver not working. The patch puts the
> > > > kfree() at the right place and (hopefully) prevents incorrect use of the
> > > > new buffer copy feature. The patch is tested by me and the users who
> > > > initially reported the bug, and they confirmed that the issue is fixed
> > > > for them.
> > > > 
> > > > If anybody has a better idea how to fix this, please go ahead! The patch
> > > > might also become obsolete in the future due to refactoring. But
> > > > currently there is a bug which prevents some people from using the
> > > > driver
> > > > at all, and this should be fixed soon IMO.
> > > 
> > > I agree, it should be fixed, but having the code always do the copy and
> > > manage the memory, and not have the crazy "flag" option, should solve
> > > the bug for everyone.
> > 
> > Removing the flag saves three lines of code, keeping the flag saves a tiny
> > amount of time and memory, so it's not really worth a lengthy discussion,
> > and I actually don't care much. I will focus on the user space library I'm
> > currently working on since it will make much of the MIDI-related Line6
> > kernel driver code obsolete.
> I'm currently re-investigating this, and I have been informed by users that 
> some newer Line6 devices talk a device-specific protocol over USB which is 
> different from the MIDI standard and should therefore not be mapped to a 
> virtual MIDI device. This raises some additional questions:
> 
> *) The easiest way to deal with this would be to use libusb in user space to 
> exchange data with the device. However, as far as I understand, if the device 
> is being used as an ALSA sound card (i.e., the kernel driver has claimed the 
> USB interface to access isochronous endpoints), libusb can't access interrupt 
> endpoints of the same interface at the same time since it can't claim the 
> interface while it is claimed by the kernel. Is this correct?

Yes, that is correct.

> *) If shared kernel/user space access to the same USB interface is not 
> possible as discussed above, what would be the preferred interface for user 
> space applications to talk to the kernel driver? I think netlink is a good 
> candidate, or do you have any other suggestions?

What exactly do you need to communicate from user to kernel?  That is
going to dictate what interface to use.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] staging: line6: fix use-after-free bug
  2013-01-18 21:52 [PATCH] staging: line6: fix use-after-free bug Markus Grabner
                   ` (6 preceding siblings ...)
  2013-06-03 23:08 ` Greg Kroah-Hartman
@ 2013-06-04 20:10 ` Markus Grabner
  2013-06-19 16:40 ` Greg Kroah-Hartman
  8 siblings, 0 replies; 10+ messages in thread
From: Markus Grabner @ 2013-06-04 20:10 UTC (permalink / raw)
  To: kernel-janitors

On Monday 03 June 2013 16:08:21 Greg Kroah-Hartman wrote:
> On Tue, Jun 04, 2013 at 12:49:36AM +0200, Markus Grabner wrote:
> > I'm currently re-investigating this, and I have been informed by users
> > that
> > some newer Line6 devices talk a device-specific protocol over USB which is
> > different from the MIDI standard and should therefore not be mapped to a
> > virtual MIDI device. This raises some additional questions:
> > 
> > *) The easiest way to deal with this would be to use libusb in user space
> > to exchange data with the device. However, as far as I understand, if the
> > device is being used as an ALSA sound card (i.e., the kernel driver has
> > claimed the USB interface to access isochronous endpoints), libusb can't
> > access interrupt endpoints of the same interface at the same time since
> > it can't claim the interface while it is claimed by the kernel. Is this
> > correct?
> 
> Yes, that is correct.
Thanks for the clarification!

> > *) If shared kernel/user space access to the same USB interface is not
> > possible as discussed above, what would be the preferred interface for
> > user
> > space applications to talk to the kernel driver? I think netlink is a good
> > candidate, or do you have any other suggestions?
> 
> What exactly do you need to communicate from user to kernel?  That is
> going to dictate what interface to use.
Line6 devices use USB interrupt messages for the following purposes:
*) device->host: when the user interacts with the device (e.g., turns the 
volume dial), the modified state is reported to the host by an asynchronous 
message (typically 2 to 16 bytes)
*) host->device: the state of the device can be changed by sending similar 
messages from host to device (e.g., a GUI application would do so when the 
user turns the volume dial on screen)
*) some device information can explicitly be queried by sending a message, to 
which the device responds with one or more messages (these can be much longer, 
e.g., the device's NVRAM content)

Moreover, some (few) information is exchanged via USB control messages. All 
messages are encoded in a device-dependent binary format, so far I am aware of 
~200 different message types.

All of this could be done by libusb, if there weren't the restriction that the 
user space can't claim the interface after the kernel has done so. Since the 
Line6 kernel driver has exclusive access to the USB interface, it must provide 
some additional "entry point" to route messages between user space code and 
the Line6 USB device.

With these requirements in mind, I did some search and found the page
"http://people.ee.ethz.ch/~arkeller/linux/kernel_user_space_howto.html", from 
which I think netlink is best suited. What do you think?

	Kind regards,
		Markus


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] staging: line6: fix use-after-free bug
  2013-01-18 21:52 [PATCH] staging: line6: fix use-after-free bug Markus Grabner
                   ` (7 preceding siblings ...)
  2013-06-04 20:10 ` Markus Grabner
@ 2013-06-19 16:40 ` Greg Kroah-Hartman
  8 siblings, 0 replies; 10+ messages in thread
From: Greg Kroah-Hartman @ 2013-06-19 16:40 UTC (permalink / raw)
  To: kernel-janitors

On Tue, Jun 04, 2013 at 10:10:07PM +0200, Markus Grabner wrote:
> On Monday 03 June 2013 16:08:21 Greg Kroah-Hartman wrote:
> > On Tue, Jun 04, 2013 at 12:49:36AM +0200, Markus Grabner wrote:
> > > I'm currently re-investigating this, and I have been informed by users
> > > that
> > > some newer Line6 devices talk a device-specific protocol over USB which is
> > > different from the MIDI standard and should therefore not be mapped to a
> > > virtual MIDI device. This raises some additional questions:
> > > 
> > > *) The easiest way to deal with this would be to use libusb in user space
> > > to exchange data with the device. However, as far as I understand, if the
> > > device is being used as an ALSA sound card (i.e., the kernel driver has
> > > claimed the USB interface to access isochronous endpoints), libusb can't
> > > access interrupt endpoints of the same interface at the same time since
> > > it can't claim the interface while it is claimed by the kernel. Is this
> > > correct?
> > 
> > Yes, that is correct.
> Thanks for the clarification!
> 
> > > *) If shared kernel/user space access to the same USB interface is not
> > > possible as discussed above, what would be the preferred interface for
> > > user
> > > space applications to talk to the kernel driver? I think netlink is a good
> > > candidate, or do you have any other suggestions?
> > 
> > What exactly do you need to communicate from user to kernel?  That is
> > going to dictate what interface to use.
> Line6 devices use USB interrupt messages for the following purposes:
> *) device->host: when the user interacts with the device (e.g., turns the 
> volume dial), the modified state is reported to the host by an asynchronous 
> message (typically 2 to 16 bytes)
> *) host->device: the state of the device can be changed by sending similar 
> messages from host to device (e.g., a GUI application would do so when the 
> user turns the volume dial on screen)
> *) some device information can explicitly be queried by sending a message, to 
> which the device responds with one or more messages (these can be much longer, 
> e.g., the device's NVRAM content)
> 
> Moreover, some (few) information is exchanged via USB control messages. All 
> messages are encoded in a device-dependent binary format, so far I am aware of 
> ~200 different message types.
> 
> All of this could be done by libusb, if there weren't the restriction that the 
> user space can't claim the interface after the kernel has done so. Since the 
> Line6 kernel driver has exclusive access to the USB interface, it must provide 
> some additional "entry point" to route messages between user space code and 
> the Line6 USB device.

Either do it all in the in-kernel driver, or just do it all in
userspace, don't try to mix the two.

> With these requirements in mind, I did some search and found the page
> "http://people.ee.ethz.ch/~arkeller/linux/kernel_user_space_howto.html", from 
> which I think netlink is best suited. What do you think?

I really don't think netlink is a viable tool to control drivers other
than network drivers.

But it's not my driver / device, so I really don't know, sorry.

greg k-h

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2013-06-19 16:40 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-01-18 21:52 [PATCH] staging: line6: fix use-after-free bug Markus Grabner
2013-01-19  0:57 ` Greg Kroah-Hartman
2013-01-19 21:55 ` Markus Grabner
2013-01-20 17:11 ` Greg Kroah-Hartman
2013-01-20 22:51 ` Markus Grabner
2013-01-20 23:04 ` Greg Kroah-Hartman
2013-06-03 22:49 ` Markus Grabner
2013-06-03 23:08 ` Greg Kroah-Hartman
2013-06-04 20:10 ` Markus Grabner
2013-06-19 16:40 ` Greg Kroah-Hartman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox