Skip to contentSkip to conversation

sound: line6: Fix race condition in line6_probe

RFC PATCH In review Hyeonggon Yoo 2 participants 6 messages May 17, 2021 Sound

lore
Share & tools
lore

Hyeonggon Yoo

author
#0 lore raw

RFC PATCH 1/1 sound: line6: Fix race condition in line6_probe

syzbot reported general protection fault in midibuf_is_full. the cause is linemidi pointer in struct usb_line6 isn't properly initialized.

the pointer isn't initialized because there is race condition in line6_probe. it calls line6_init_cap_control first, which submits urb. and then it initializes it's data using private_init function.

so it's possible line6_data_received is called before it's data isn't initialized.

Link https://lkml.org/lkml/2021/5/17/543
Reported-by [email protected]
Signed-off-by Hyeonggon Yoo <[email protected]>
---
 sound/usb/line6/driver.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
sound/usb/line6/driver.c +5 -5
index a030dd65eb28..2c183a2a30f0 100644
@@ -788,17 +788,17 @@ int line6_probe(struct usb_interface *interface,
 
 	line6_get_usb_properties(line6);
 
+	/* initialize device data based on device: */
+	ret = private_init(line6, id);
+	if (ret < 0)
+		goto error;
+
 	if (properties->capabilities & LINE6_CAP_CONTROL) {
 		ret = line6_init_cap_control(line6);
 		if (ret < 0)
 			goto error;
 	}
 
-	/* initialize device data based on device: */
-	ret = private_init(line6, id);
-	if (ret < 0)
-		goto error;
-
 	/* creation of additional special files should go here */
 
 	dev_info(&interface->dev, "Line 6 %s now attached\n",
Signature · 1 line
2.25.1
#1 lore raw
Quoted 9 lines · Hyeonggon Yoo

syzbot reported general protection fault in midibuf_is_full. the cause is linemidi pointer in struct usb_line6 isn't properly initialized.

the pointer isn't initialized because there is race condition in line6_probe. it calls line6_init_cap_control first, which submits urb. and then it initializes it's data using private_init function.

Quoted 6 lines

so it's possible line6_data_received is called before it's data isn't initialized.

Link https://lkml.org/lkml/2021/5/17/543
Reported-by [email protected]
Signed-off-by Hyeonggon Yoo <[email protected]>

Thanks for the patch! It's an issue I'm tracking now, too, and you submitted quicker. But, unfortunately, this swap does seem yet another potential race between the private_init call and line6_init_cap_control(). The actually needed initialization is line6_init_mid() call, and this can be fixed by moving to the appropriate place instead of inside each private_init callback.

So my current fix is like below.

thanks,

Takashi

---

sound/usb/line6/driver.c +4 -0
index a030dd65eb28..9602929b7de9 100644
@@ -699,6 +699,10 @@ static int line6_init_cap_control(struct usb_line6 *line6)
 		line6->buffer_message = kmalloc(LINE6_MIDI_MESSAGE_MAXLEN, GFP_KERNEL);
 		if (!line6->buffer_message)
 			return -ENOMEM;
+
+		ret = line6_init_midi(line6);
+		if (ret < 0)
+			return ret;
 	} else {
 		ret = line6_hwdep_init(line6);
 		if (ret < 0)
sound/usb/line6/pod.c +0 -5
index cd44cb5f1310..16e644330c4d 100644
@@ -376,11 +376,6 @@ static int pod_init(struct usb_line6 *line6,
 	if (err < 0)
 		return err;
 
-	/* initialize MIDI subsystem: */
-	err = line6_init_midi(line6);
-	if (err < 0)
-		return err;
-
 	/* initialize PCM subsystem: */
 	err = line6_init_pcm(line6, &pod_pcm_properties);
 	if (err < 0)
sound/usb/line6/variax.c +0 -5
index ed158f04de80..1376fc405c7f 100644
@@ -172,11 +172,6 @@ static int variax_init(struct usb_line6 *line6,
 	if (variax->buffer_activate == NULL)
 		return -ENOMEM;
 
-	/* initialize MIDI subsystem: */
-	err = line6_init_midi(&variax->line6);
-	if (err < 0)
-		return err;
-
 	/* initiate startup procedure: */
 	schedule_delayed_work(&line6->startup_work,
 			      msecs_to_jiffies(VARIAX_STARTUP_DELAY1));
author
#2 lore raw
Quoted 3 lines · Takashi Iwai

The actually needed initialization is line6_init_mid() call, and this can be fixed by moving to the appropriate place instead of inside each private_init callback.

Oh, I missed it! there was another caller of line6_init_midi. your fix seems promising to me. it's putting line6_init_midi to the right place.

by the way looking at code, I think this driver needs some refactoring... it doesn't handle exceptions well.

Thanks,

Hyeonggon

Quoted 48 lines

---

sound/usb/line6/driver.c +4 -0
index a030dd65eb28..9602929b7de9 100644
@@ -699,6 +699,10 @@ static int line6_init_cap_control(struct usb_line6 *line6)
 		line6->buffer_message = kmalloc(LINE6_MIDI_MESSAGE_MAXLEN, GFP_KERNEL);
 		if (!line6->buffer_message)
 			return -ENOMEM;
+
+		ret = line6_init_midi(line6);
+		if (ret < 0)
+			return ret;
 	} else {
 		ret = line6_hwdep_init(line6);
 		if (ret < 0)
sound/usb/line6/pod.c +0 -5
index cd44cb5f1310..16e644330c4d 100644
@@ -376,11 +376,6 @@ static int pod_init(struct usb_line6 *line6,
 	if (err < 0)
 		return err;
 
-	/* initialize MIDI subsystem: */
-	err = line6_init_midi(line6);
-	if (err < 0)
-		return err;
-
 	/* initialize PCM subsystem: */
 	err = line6_init_pcm(line6, &pod_pcm_properties);
 	if (err < 0)
sound/usb/line6/variax.c +0 -5
index ed158f04de80..1376fc405c7f 100644
@@ -172,11 +172,6 @@ static int variax_init(struct usb_line6 *line6,
 	if (variax->buffer_activate == NULL)
 		return -ENOMEM;
 
-	/* initialize MIDI subsystem: */
-	err = line6_init_midi(&variax->line6);
-	if (err < 0)
-		return err;
-
 	/* initiate startup procedure: */
 	schedule_delayed_work(&line6->startup_work,
 			      msecs_to_jiffies(VARIAX_STARTUP_DELAY1));
#3 lore raw
Quoted 6 lines · Hyeonggon Yoo
Quoted 3 lines · Takashi Iwai

The actually needed initialization is line6_init_mid() call, and this can be fixed by moving to the appropriate place instead of inside each private_init callback.

Quoted 6 lines

Oh, I missed it! there was another caller of line6_init_midi. your fix seems promising to me. it's putting line6_init_midi to the right place.

by the way looking at code, I think this driver needs some refactoring... it doesn't handle exceptions well.

Yes, there can be likely a few other holes in this driver, but for fixing them, we'd need an actual test device. The initialization procedure of this device seems complex (multi-staged) and very sensitive.

Takashi

Quoted 53 lines

Thanks,

Hyeonggon

Quoted 48 lines

---

sound/usb/line6/driver.c +4 -0
index a030dd65eb28..9602929b7de9 100644
@@ -699,6 +699,10 @@ static int line6_init_cap_control(struct usb_line6 *line6)
 		line6->buffer_message = kmalloc(LINE6_MIDI_MESSAGE_MAXLEN, GFP_KERNEL);
 		if (!line6->buffer_message)
 			return -ENOMEM;
+
+		ret = line6_init_midi(line6);
+		if (ret < 0)
+			return ret;
 	} else {
 		ret = line6_hwdep_init(line6);
 		if (ret < 0)
sound/usb/line6/pod.c +0 -5
index cd44cb5f1310..16e644330c4d 100644
@@ -376,11 +376,6 @@ static int pod_init(struct usb_line6 *line6,
 	if (err < 0)
 		return err;
 
-	/* initialize MIDI subsystem: */
-	err = line6_init_midi(line6);
-	if (err < 0)
-		return err;
-
 	/* initialize PCM subsystem: */
 	err = line6_init_pcm(line6, &pod_pcm_properties);
 	if (err < 0)
sound/usb/line6/variax.c +0 -5
index ed158f04de80..1376fc405c7f 100644
@@ -172,11 +172,6 @@ static int variax_init(struct usb_line6 *line6,
 	if (variax->buffer_activate == NULL)
 		return -ENOMEM;
 
-	/* initialize MIDI subsystem: */
-	err = line6_init_midi(&variax->line6);
-	if (err < 0)
-		return err;
-
 	/* initiate startup procedure: */
 	schedule_delayed_work(&line6->startup_work,
 			      msecs_to_jiffies(VARIAX_STARTUP_DELAY1));
author
#4 lore raw
Quoted 5 lines · Takashi Iwai

Yes, there can be likely a few other holes in this driver, but for fixing them, we'd need an actual test device. The initialization procedure of this device seems complex (multi-staged) and very sensitive.
Takashi

Yeap, this driver is so complex,
and I agree that we need actual device to test
if we do that big scale of refactoring.

Sadly I don't have one :(

Thanks,
Hyeonggon