staging: line6: use after free bug requesting version
Subsystems (2)
PATCH 2/2 staging: line6: use after free bug requesting version
In line6_version_request_async() we set up an async message but we free the buffer with the version in it before the message has been sent.
I've introduced a new function line6_async_request_sent_free_buffer() which frees the data after we are done with it. I've added a "free" parameter to the line6_send_raw_message_async() functions so that they know if they have to free the msg->buffer or not at the end.
drivers/staging/line6/driver.h +1 -1
@@ -205,7 +205,7 @@ 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,int free);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,
drivers/staging/line6/variax.c +1 -1
@@ -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),0 ); }/*
drivers/staging/line6/driver.c +26 -9
@@ -110,7 +110,7 @@ struct message {*/ static void line6_data_received (struct urb *urb);static int line6_send_raw_message_async_part (struct message *msg, -struct urb *urb); +struct urb *urb,int free);/* Start to listen on endpoint.
@@ -219,24 +219,42 @@ static void line6_async_request_sent(struct urb *urb)usb_free_urb (urb);kfree (msg); }else -line6_send_raw_message_async_part (msg, urb); +line6_send_raw_message_async_part (msg, urb,0 ); +} + +static void line6_async_request_sent_free_buffer (struct urb *urb) +{ +struct message *msg = (struct message *)urb->context; + +if (msg->done >= msg->size) { +usb_free_urb (urb); +kfree (msg->buffer); +kfree (msg); + }else +line6_send_raw_message_async_part (msg, urb,1 ); }/* Asynchronously send part of a raw message. */ static int line6_send_raw_message_async_part (struct message *msg, -struct urb *urb) +struct urb *urb,int free) {int retval;struct usb_line6 *line6 = msg->line6;int done = msg->done;int bytes =min (msg->size - done, line6->max_packet_size); +usb_complete_t complete_fn; + +if (free) + complete_fn = line6_async_request_sent_free_buffer; +else + complete_fn = line6_async_request_sent;usb_fill_int_urb (urb, line6->usbdev,usb_sndintpipe (line6->usbdev, line6->ep_control_write), - (char *)msg->buffer + done, bytes, - line6_async_request_sent, msg, line6->interval); + (char *)msg->buffer + done, bytes, complete_fn, msg, + line6->interval); msg->done += bytes; retval =usb_submit_urb (urb,GFP_ATOMIC );
@@ -267,7 +285,7 @@ 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,int free) {struct message *msg;struct urb *urb;
@@ -296,7 +314,7 @@ int line6_send_raw_message_async(struct usb_line6 *line6, const char *buffer, msg->done =0 ;/* start sending: */ -return line6_send_raw_message_async_part (msg, urb); +return line6_send_raw_message_async_part (msg, urb, free); }/*
@@ -316,8 +334,7 @@ int line6_version_request_async(struct usb_line6 *line6)memcpy (buffer, line6_request_version,sizeof (line6_request_version)); retval =line6_send_raw_message_async (line6, buffer, -sizeof (line6_request_version)); -kfree (buffer); +sizeof (line6_request_version),1 );return retval; }
drivers/staging/line6/driver.c struct message { · line 113
Quoted 10 lines · Dan Carpenter
drivers/staging/line6/driver.c +1
-1@@ -110,7 +110,7 @@ struct message {*/ static void line6_data_received (struct urb *urb);static int line6_send_raw_message_async_part (struct message *msg, -struct urb *urb); +struct urb *urb,int free);
s/int/bool/
drivers/staging/line6/driver.c static void line6_async_request_sent(struct urb *urb) · line 223
Quoted 10 lines
drivers/staging/line6/driver.c +2
-1/* Start to listen on endpoint. @@ -219,24 +219,42 @@ static void line6_async_request_sent(struct urb *urb)usb_free_urb (urb);kfree (msg); }else -line6_send_raw_message_async_part (msg, urb); +line6_send_raw_message_async_part (msg, urb,0 ); +}
I'd add a bool free_buffer field to struct message and simply modify line6_async_request_sent() to do:
if (msg->free_buffer)
kfree(msg->buffer);
Then you don't need line6_async_request_sent_free_buffer() and line6_send_raw_message_async_part() doesn't need to take a bool free argument since struct message already contains that information. It would make the code simpler.
Quoted 28 lines · Stefan Hajnoczi
drivers/staging/line6/driver.c struct message { · line 113
Quoted 10 lines · Dan Carpenter
drivers/staging/line6/driver.c +1
-1@@ -110,7 +110,7 @@ struct message {*/ static void line6_data_received (struct urb *urb);static int line6_send_raw_message_async_part (struct message *msg, -struct urb *urb); +struct urb *urb,int free);s/int/bool/
drivers/staging/line6/driver.c static void line6_async_request_sent(struct urb *urb) · line 223
Quoted 10 lines
drivers/staging/line6/driver.c +2
-1/* Start to listen on endpoint. @@ -219,24 +219,42 @@ static void line6_async_request_sent(struct urb *urb)usb_free_urb (urb);kfree (msg); }else -line6_send_raw_message_async_part (msg, urb); +line6_send_raw_message_async_part (msg, urb,0 ); +}I'd add a bool
free_bufferfield to struct message and simply modifyline6_async_request_sent()to do:
Quoted 7 lines
if (
msg->free_buffer)kfree(msg->buffer);Then you don't need
line6_async_request_sent_free_buffer()andline6_send_raw_message_async_part()doesn't need to take a bool free argument since struct message already contains that information. It would make the code simpler.
Yeah. That's true. I'll redo it.
regards,
dan carpenter
Quoted 47 lines · Markus Grabner
Am Donnerstag, 6. Dezember 2012, 10:08:44 schrieb Dan Carpenter:
Quoted 2 lines · Stefan Hajnoczi
Quoted 1 line
On Wed, Dec 5, 2012 at 7:44 PM, Dan Carpenter <[email protected]>
Quoted 43 lines · wrote:
Quoted 42 lines
drivers/staging/line6/driver.c struct message { · line 115
Quoted 12 lines
(quoted diff) +0
-0
b/drivers/staging/line6/driver.cindex 8a5d89e..884e0d8 100644drivers/staging/line6/driver.c +1
-1@@ -110,7 +110,7 @@ struct message {*/ static void line6_data_received (struct urb *urb);static int line6_send_raw_message_async_part (struct message *msg, -struct urb *urb); +struct urb *urb,int free);s/int/bool/
Quoted 5 lines
drivers/staging/line6/driver.c +0
-0/* Start to listen on endpoint.
drivers/staging/line6/driver.c static void line6_async_request_sent(struct urb · line 227
Quoted 10 lines
drivers/staging/line6/driver.c +2
-1@@ -219,24 +219,42 @@ static void line6_async_request_sent(struct urb *urb)usb_free_urb (urb);kfree (msg); }else -line6_send_raw_message_async_part (msg, urb); +line6_send_raw_message_async_part (msg, urb,0 ); +}I'd add a bool
free_bufferfield to struct message and simply modifyline6_async_request_sent()to do:if (
msg->free_buffer)kfree(msg->buffer);Then you don't need
line6_async_request_sent_free_buffer()andline6_send_raw_message_async_part()doesn't need to take a bool free argument since struct message already contains that information. It would make the code simpler.
Quoted 6 lines
Quoted 1 line
Yeah. That's true. I'll redo it.
Since two users reported this bug to me recently, I proposed a fix and asked them to test it. If it works for them, I'll prepare a patch against Stefan's repository. This is for your information only to avoid duplicate work in case you just wanted to pick this up again.
Crap... I'm sorry. I completely lost track of this. Please send your patch.
regards,
dan carpenter
Quoted 1 line · On Thursday 06 December 2012 06:18:02 Stefan Hajnoczi wrote:
On Wed, Dec 5, 2012 at 7:44 PM, Dan Carpenter <[email protected]>
Quoted 34 lines · wrote:
drivers/staging/line6/driver.c struct message { · line 115
Quoted 12 lines
(quoted diff) +0
-0
b/drivers/staging/line6/driver.cindex 8a5d89e..884e0d8 100644drivers/staging/line6/driver.c +1
-1@@ -110,7 +110,7 @@ struct message {*/ static void line6_data_received (struct urb *urb);static int line6_send_raw_message_async_part (struct message *msg, -struct urb *urb); +struct urb *urb,int free);s/int/bool/
Quoted 5 lines
drivers/staging/line6/driver.c +0
-0/* Start to listen on endpoint.
drivers/staging/line6/driver.c static void line6_async_request_sent(struct urb · line 227
Quoted 10 lines
drivers/staging/line6/driver.c +2
-1@@ -219,24 +219,42 @@ static void line6_async_request_sent(struct urb *urb)usb_free_urb (urb);kfree (msg); }else -line6_send_raw_message_async_part (msg, urb); +line6_send_raw_message_async_part (msg, urb,0 ); +}I'd add a bool
free_bufferfield to struct message and simply modifyline6_async_request_sent()to do:
Quoted 7 lines
if (
msg->free_buffer)kfree(msg->buffer);Then you don't need
line6_async_request_sent_free_buffer()andline6_send_raw_message_async_part()doesn't need to take a bool free argument since struct message already contains that information. It would make the code simpler.
Considering the suggestions made so far, I came up with the following
solution: 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.
The patch is against linux-next since this bug should be fixed regardless of the status of moving the Line6 driver out of the staging area.
drivers/staging/line6/driver.c +4 -0
@@ -106,6 +106,7 @@ struct message {const char *buffer;int size;int done; +bool copy; };/*
@@ -239,6 +240,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
@@ -294,26 +298,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: */
@@ -321,9 +328,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;
}
/*
@@ -331,21 +349,8 @@ 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 = kmalloc(sizeof(line6_request_version), GFP_ATOMIC);
- if (buffer = NULL) {
- dev_err(line6->ifcdev, "Out of memory");
- return -ENOMEM;
- }
-
- memcpy(buffer, line6_request_version, sizeof(line6_request_version));
-
- 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);
}
/*
drivers/staging/line6/driver.h +0 -0
@@ -213,7 +213,7 @@ extern int line6_send_program(struct usb_line6 *line6, int
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 int line6_send_sysex_message_async(struct usb_line6 *line6,
(quoted diff) +0 -0
b/drivers/staging/line6/dumprequest.c
index 60c7bae..c2750c5 100644
drivers/staging/line6/dumprequest.c +0 -0
@@ -49,7 +49,7 @@ int line6_dump_request_async(struct line6_dump_request
*l6dr,
int ret;
line6_dump_started(l6dr, dest);
ret = line6_send_raw_message_async(line6, l6dr->reqbufs[num].buffer,
- l6dr->reqbufs[num].length);
+ l6dr->reqbufs[num].length, false);
if (ret < 0)
line6_dump_finished(l6dr);
drivers/staging/line6/variax.c +0 -0
@@ -94,7 +94,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);
}
/*
Quoted 7 lines · Markus Grabner
Considering the suggestions made so far, I came up with the following
solution: 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.
Quoted 5 lines
The patch is against linux-next since this bug should be fixed regardless of the status of moving the Line6 driver out of the staging area.
Reported-by Stefan Hajnoczi <[email protected]>Signed-off-by Markus Grabner <[email protected]>
Greg Kroah-Hartman
maintainerQuoted 51 lines · Markus Grabner
Quoted 1 line · On Thursday 06 December 2012 06:18:02 Stefan Hajnoczi wrote:
On Wed, Dec 5, 2012 at 7:44 PM, Dan Carpenter <[email protected]>
Quoted 34 lines · wrote:
drivers/staging/line6/driver.c struct message { · line 115
Quoted 12 lines
(quoted diff) +0
-0
b/drivers/staging/line6/driver.cindex 8a5d89e..884e0d8 100644drivers/staging/line6/driver.c +1
-1@@ -110,7 +110,7 @@ struct message {*/ static void line6_data_received (struct urb *urb);static int line6_send_raw_message_async_part (struct message *msg, -struct urb *urb); +struct urb *urb,int free);s/int/bool/
Quoted 5 lines
drivers/staging/line6/driver.c +0
-0/* Start to listen on endpoint.
drivers/staging/line6/driver.c static void line6_async_request_sent(struct urb · line 227
Quoted 10 lines
drivers/staging/line6/driver.c +2
-1@@ -219,24 +219,42 @@ static void line6_async_request_sent(struct urb *urb)usb_free_urb (urb);kfree (msg); }else -line6_send_raw_message_async_part (msg, urb); +line6_send_raw_message_async_part (msg, urb,0 ); +}I'd add a bool
free_bufferfield to struct message and simply modifyline6_async_request_sent()to do:Quoted 7 lines
if (
msg->free_buffer)kfree(msg->buffer);Then you don't need
line6_async_request_sent_free_buffer()andline6_send_raw_message_async_part()doesn't need to take a bool free argument since struct message already contains that information. It would make the code simpler.Considering the suggestions made so far, I came up with the following
solution: 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.
Quoted 5 lines
The patch is against linux-next since this bug should be fixed regardless of the status of moving the Line6 driver out of the staging area.
Reported-by Stefan Hajnoczi <[email protected]>Signed-off-by Markus Grabner <[email protected]>
Can you resend this in a format I can apply it in (proper Subject, not line-wrapped, etc.)
thanks,
greg k-h