diff mbox

[v2,9/9] ALSA: line6: Add yet more sanity checks for invalid EPs

Message ID 20171011103646.11879-10-tiwai@suse.de (mailing list archive)
State New, archived
Headers show

Commit Message

Takashi Iwai Oct. 11, 2017, 10:36 a.m. UTC
There are a few other places calling usb_submit_urb() with the URB
composed from the fixed endpoint without validation.  For avoiding the
spurious kernel warnings, add the sanity checks to appropriate
places.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/line6/driver.c | 23 +++++++++++++++--------
 sound/usb/line6/midi.c   | 17 +++++++++++------
 2 files changed, 26 insertions(+), 14 deletions(-)

Comments

Johan Hovold Oct. 11, 2017, 2:39 p.m. UTC | #1
On Wed, Oct 11, 2017 at 12:36:46PM +0200, Takashi Iwai wrote:
> There are a few other places calling usb_submit_urb() with the URB
> composed from the fixed endpoint without validation.  For avoiding the
> spurious kernel warnings, add the sanity checks to appropriate
> places.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/usb/line6/driver.c | 23 +++++++++++++++--------
>  sound/usb/line6/midi.c   | 17 +++++++++++------
>  2 files changed, 26 insertions(+), 14 deletions(-)
> 
> diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c
> index 0da6f68761e3..7c682b219584 100644
> --- a/sound/usb/line6/driver.c
> +++ b/sound/usb/line6/driver.c
> @@ -175,17 +175,24 @@ static int line6_send_raw_message_async_part(struct message *msg,
>  	}
>  
>  	msg->done += bytes;
> -	retval = usb_submit_urb(urb, GFP_ATOMIC);
>  
> -	if (retval < 0) {
> -		dev_err(line6->ifcdev, "%s: usb_submit_urb failed (%d)\n",
> -			__func__, retval);
> -		usb_free_urb(urb);
> -		kfree(msg);
> -		return retval;
> -	}
> +	/* sanity checks of EP before actually submitting */
> +	retval = usb_urb_ep_type_check(urb);
> +	if (retval < 0)
> +		goto error;

This also seems like something which should have been done once at urb
allocation time, rather than on every submission (just like the URB
initialisation should have been).

> +
> +	retval = usb_submit_urb(urb, GFP_ATOMIC);
> +	if (retval < 0)
> +		goto error;
>  
>  	return 0;
> +
> + error:
> +	dev_err(line6->ifcdev, "%s: usb_submit_urb failed (%d)\n",
> +		__func__, retval);
> +	usb_free_urb(urb);
> +	kfree(msg);
> +	return retval;

Johan
Takashi Iwai Oct. 11, 2017, 2:48 p.m. UTC | #2
On Wed, 11 Oct 2017 16:39:52 +0200,
Johan Hovold wrote:
> 
> On Wed, Oct 11, 2017 at 12:36:46PM +0200, Takashi Iwai wrote:
> > There are a few other places calling usb_submit_urb() with the URB
> > composed from the fixed endpoint without validation.  For avoiding the
> > spurious kernel warnings, add the sanity checks to appropriate
> > places.
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  sound/usb/line6/driver.c | 23 +++++++++++++++--------
> >  sound/usb/line6/midi.c   | 17 +++++++++++------
> >  2 files changed, 26 insertions(+), 14 deletions(-)
> > 
> > diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c
> > index 0da6f68761e3..7c682b219584 100644
> > --- a/sound/usb/line6/driver.c
> > +++ b/sound/usb/line6/driver.c
> > @@ -175,17 +175,24 @@ static int line6_send_raw_message_async_part(struct message *msg,
> >  	}
> >  
> >  	msg->done += bytes;
> > -	retval = usb_submit_urb(urb, GFP_ATOMIC);
> >  
> > -	if (retval < 0) {
> > -		dev_err(line6->ifcdev, "%s: usb_submit_urb failed (%d)\n",
> > -			__func__, retval);
> > -		usb_free_urb(urb);
> > -		kfree(msg);
> > -		return retval;
> > -	}
> > +	/* sanity checks of EP before actually submitting */
> > +	retval = usb_urb_ep_type_check(urb);
> > +	if (retval < 0)
> > +		goto error;
> 
> This also seems like something which should have been done once at urb
> allocation time, rather than on every submission (just like the URB
> initialisation should have been).

Yes, ideally this would be done in the init time.  But the code
allocates a URB temporarily and fills it depending on the board
configuration, so it's not too trivial to do that without a big code
refactoring.  So the current patch is merely hardening a bit.
For obtaining the ideal code flow, we'd need the whole code
restructuring in anyway for this driver; currently it's too messy.


thanks,

Takashi
diff mbox

Patch

diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c
index 0da6f68761e3..7c682b219584 100644
--- a/sound/usb/line6/driver.c
+++ b/sound/usb/line6/driver.c
@@ -175,17 +175,24 @@  static int line6_send_raw_message_async_part(struct message *msg,
 	}
 
 	msg->done += bytes;
-	retval = usb_submit_urb(urb, GFP_ATOMIC);
 
-	if (retval < 0) {
-		dev_err(line6->ifcdev, "%s: usb_submit_urb failed (%d)\n",
-			__func__, retval);
-		usb_free_urb(urb);
-		kfree(msg);
-		return retval;
-	}
+	/* sanity checks of EP before actually submitting */
+	retval = usb_urb_ep_type_check(urb);
+	if (retval < 0)
+		goto error;
+
+	retval = usb_submit_urb(urb, GFP_ATOMIC);
+	if (retval < 0)
+		goto error;
 
 	return 0;
+
+ error:
+	dev_err(line6->ifcdev, "%s: usb_submit_urb failed (%d)\n",
+		__func__, retval);
+	usb_free_urb(urb);
+	kfree(msg);
+	return retval;
 }
 
 /*
diff --git a/sound/usb/line6/midi.c b/sound/usb/line6/midi.c
index 1d3a23b02d68..6d7cde56a355 100644
--- a/sound/usb/line6/midi.c
+++ b/sound/usb/line6/midi.c
@@ -130,16 +130,21 @@  static int send_midi_async(struct usb_line6 *line6, unsigned char *data,
 			 transfer_buffer, length, midi_sent, line6,
 			 line6->interval);
 	urb->actual_length = 0;
-	retval = usb_submit_urb(urb, GFP_ATOMIC);
+	retval = usb_urb_ep_type_check(urb);
+	if (retval < 0)
+		goto error;
 
-	if (retval < 0) {
-		dev_err(line6->ifcdev, "usb_submit_urb failed\n");
-		usb_free_urb(urb);
-		return retval;
-	}
+	retval = usb_submit_urb(urb, GFP_ATOMIC);
+	if (retval < 0)
+		goto error;
 
 	++line6->line6midi->num_active_send_urbs;
 	return 0;
+
+ error:
+	dev_err(line6->ifcdev, "usb_submit_urb failed\n");
+	usb_free_urb(urb);
+	return retval;
 }
 
 static int line6_midi_output_open(struct snd_rawmidi_substream *substream)