[patch 2/2] staging: line6: use after free bug requesting version

[patch 2/2] staging: line6: use after free bug requesting version

[Dan Carpenter]

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.

Reported-by: Stefan Hajnoczi <stefanha@gmail.com>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/staging/line6/driver.h b/drivers/staging/line6/driver.h
index 1dd768c2..918d19e 100644
--- a/drivers/staging/line6/driver.h
+++ b/drivers/staging/line6/driver.h
@@ -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,
diff --git a/drivers/staging/line6/variax.c b/drivers/staging/line6/variax.c
index 4fca58f..f97512d 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), 0);
 }
 
 /*
diff --git a/drivers/staging/line6/driver.c b/drivers/staging/line6/driver.c
index 8a5d89e..884e0d8 100644
--- a/drivers/staging/line6/driver.c
+++ b/drivers/staging/line6/driver.c
@@ -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;
 }
 

Re: [patch 2/2] staging: line6: use after free bug requesting version

[Stefan Hajnoczi]

On Wed, Dec 5, 2012 at 7:44 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> diff --git a/drivers/staging/line6/driver.c b/drivers/staging/line6/driver.c
> index 8a5d89e..884e0d8 100644
> --- a/drivers/staging/line6/driver.c
> +++ b/drivers/staging/line6/driver.c
> @@ -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/

>
>  /*
>         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.

Re: [patch 2/2] staging: line6: use after free bug requesting version

[Dan Carpenter]

On Thu, Dec 06, 2012 at 06:18:02AM +0100, Stefan Hajnoczi wrote:
> On Wed, Dec 5, 2012 at 7:44 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > diff --git a/drivers/staging/line6/driver.c b/drivers/staging/line6/driver.c
> > index 8a5d89e..884e0d8 100644
> > --- a/drivers/staging/line6/driver.c
> > +++ b/drivers/staging/line6/driver.c
> > @@ -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/
> 
> >
> >  /*
> >         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.

Yeah.  That's true.  I'll redo it.

regards,
dan carpenter

Re: [patch 2/2] staging: line6: use after free bug requesting version

[Dan Carpenter]

On Tue, Jan 08, 2013 at 11:54:25PM +0100, Markus Grabner wrote:
> Am Donnerstag, 6. Dezember 2012, 10:08:44 schrieb Dan Carpenter:
> > On Thu, Dec 06, 2012 at 06:18:02AM +0100, Stefan Hajnoczi wrote:
> > > On Wed, Dec 5, 2012 at 7:44 PM, Dan Carpenter <dan.carpenter@oracle.com> 
> wrote:
> > > > diff --git a/drivers/staging/line6/driver.c
> > > > b/drivers/staging/line6/driver.c index 8a5d89e..884e0d8 100644
> > > > --- a/drivers/staging/line6/driver.c
> > > > +++ b/drivers/staging/line6/driver.c
> > > > @@ -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/
> > > 
> > > >  /*
> > > >  
> > > >         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.
> > 
> > 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

Re: [patch 2/2] staging: line6: use after free bug requesting version

[Markus Grabner]

On Thursday 06 December 2012 06:18:02 Stefan Hajnoczi wrote:
> On Wed, Dec 5, 2012 at 7:44 PM, Dan Carpenter <dan.carpenter@oracle.com> 
wrote:
> > diff --git a/drivers/staging/line6/driver.c
> > b/drivers/staging/line6/driver.c index 8a5d89e..884e0d8 100644
> > --- a/drivers/staging/line6/driver.c
> > +++ b/drivers/staging/line6/driver.c
> > @@ -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/
> 
> >  /*
> >  
> >         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.
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.

Reported-by: Stefan Hajnoczi <stefanha@gmail.com>
Signed-off-by: Markus Grabner <grabner@icg.tugraz.at>

diff --git a/drivers/staging/line6/driver.c b/drivers/staging/line6/driver.c
index 4513f78..e29e126 100644
--- a/drivers/staging/line6/driver.c
+++ b/drivers/staging/line6/driver.c
@@ -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);
 }
 
 /*
diff --git a/drivers/staging/line6/driver.h b/drivers/staging/line6/driver.h
index 117bf99..793ceda 100644
--- a/drivers/staging/line6/driver.h
+++ b/drivers/staging/line6/driver.h
@@ -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,
diff --git a/drivers/staging/line6/dumprequest.c 
b/drivers/staging/line6/dumprequest.c
index 60c7bae..c2750c5 100644
--- a/drivers/staging/line6/dumprequest.c
+++ b/drivers/staging/line6/dumprequest.c
@@ -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);
diff --git a/drivers/staging/line6/variax.c b/drivers/staging/line6/variax.c
index d366222..d013e26 100644
--- a/drivers/staging/line6/variax.c
+++ b/drivers/staging/line6/variax.c
@@ -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);
 }
 
 /*

Re: [patch 2/2] staging: line6: use after free bug requesting version

[Stefan Hajnoczi]

On Sun, Jan 13, 2013 at 7:15 PM, Markus Grabner <grabner@icg.tugraz.at> wrote:
> 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.
>
> Reported-by: Stefan Hajnoczi <stefanha@gmail.com>
> Signed-off-by: Markus Grabner <grabner@icg.tugraz.at>

Reviewed-by: Stefan Hajnoczi <stefanha@gmail.com>

Re: [patch 2/2] staging: line6: use after free bug requesting version

[Greg Kroah-Hartman]

On Sun, Jan 13, 2013 at 07:15:22PM +0100, Markus Grabner wrote:
> On Thursday 06 December 2012 06:18:02 Stefan Hajnoczi wrote:
> > On Wed, Dec 5, 2012 at 7:44 PM, Dan Carpenter <dan.carpenter@oracle.com> 
> wrote:
> > > diff --git a/drivers/staging/line6/driver.c
> > > b/drivers/staging/line6/driver.c index 8a5d89e..884e0d8 100644
> > > --- a/drivers/staging/line6/driver.c
> > > +++ b/drivers/staging/line6/driver.c
> > > @@ -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/
> > 
> > >  /*
> > >  
> > >         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.
> 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.
> 
> Reported-by: Stefan Hajnoczi <stefanha@gmail.com>
> Signed-off-by: Markus Grabner <grabner@icg.tugraz.at>

Can you resend this in a format I can apply it in (proper Subject, not
line-wrapped, etc.)

thanks,

greg k-h

Re: [patch 2/2] staging: line6: use after free bug requesting version

[Markus Grabner]

Am Donnerstag, 6. Dezember 2012, 10:08:44 schrieb Dan Carpenter:
> On Thu, Dec 06, 2012 at 06:18:02AM +0100, Stefan Hajnoczi wrote:
> > On Wed, Dec 5, 2012 at 7:44 PM, Dan Carpenter <dan.carpenter@oracle.com> 
wrote:
> > > diff --git a/drivers/staging/line6/driver.c
> > > b/drivers/staging/line6/driver.c index 8a5d89e..884e0d8 100644
> > > --- a/drivers/staging/line6/driver.c
> > > +++ b/drivers/staging/line6/driver.c
> > > @@ -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/
> > 
> > >  /*
> > >  
> > >         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.
> 
> 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.

	Kind regards,
		Markus