[RFC PATCH] 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: syzbot+0d2b3feb0a2887862e06@syzkallerlkml..appspotmail.com Signed-off-by: Hyeonggon Yoo 42.hyeyoo@gmail.com --- sound/usb/line6/driver.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c index a030dd65eb28..2c183a2a30f0 100644 --- a/sound/usb/line6/driver.c +++ b/sound/usb/line6/driver.c @@ -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",
On Mon, 17 May 2021 15:27:25 +0200, Hyeonggon Yoo wrote:
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: syzbot+0d2b3feb0a2887862e06@syzkallerlkml..appspotmail.com Signed-off-by: Hyeonggon Yoo 42.hyeyoo@gmail.com
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
--- diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c index a030dd65eb28..9602929b7de9 100644 --- a/sound/usb/line6/driver.c +++ b/sound/usb/line6/driver.c @@ -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) diff --git a/sound/usb/line6/pod.c b/sound/usb/line6/pod.c index cd44cb5f1310..16e644330c4d 100644 --- a/sound/usb/line6/pod.c +++ b/sound/usb/line6/pod.c @@ -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) diff --git a/sound/usb/line6/variax.c b/sound/usb/line6/variax.c index ed158f04de80..1376fc405c7f 100644 --- a/sound/usb/line6/variax.c +++ b/sound/usb/line6/variax.c @@ -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));
On Mon, May 17, 2021 at 03:43:24PM +0200, Takashi Iwai wrote:
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
diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c index a030dd65eb28..9602929b7de9 100644 --- a/sound/usb/line6/driver.c +++ b/sound/usb/line6/driver.c @@ -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)
} else { ret = line6_hwdep_init(line6); if (ret < 0)return ret;
diff --git a/sound/usb/line6/pod.c b/sound/usb/line6/pod.c index cd44cb5f1310..16e644330c4d 100644 --- a/sound/usb/line6/pod.c +++ b/sound/usb/line6/pod.c @@ -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)
diff --git a/sound/usb/line6/variax.c b/sound/usb/line6/variax.c index ed158f04de80..1376fc405c7f 100644 --- a/sound/usb/line6/variax.c +++ b/sound/usb/line6/variax.c @@ -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));
On Mon, 17 May 2021 16:48:11 +0200, Hyeonggon Yoo wrote:
On Mon, May 17, 2021 at 03:43:24PM +0200, Takashi Iwai wrote:
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.
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
Thanks,
Hyeonggon
diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c index a030dd65eb28..9602929b7de9 100644 --- a/sound/usb/line6/driver.c +++ b/sound/usb/line6/driver.c @@ -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)
} else { ret = line6_hwdep_init(line6); if (ret < 0)return ret;
diff --git a/sound/usb/line6/pod.c b/sound/usb/line6/pod.c index cd44cb5f1310..16e644330c4d 100644 --- a/sound/usb/line6/pod.c +++ b/sound/usb/line6/pod.c @@ -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)
diff --git a/sound/usb/line6/variax.c b/sound/usb/line6/variax.c index ed158f04de80..1376fc405c7f 100644 --- a/sound/usb/line6/variax.c +++ b/sound/usb/line6/variax.c @@ -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));
On Mon, May 17, 2021 at 04:57:28PM +0200, Takashi Iwai wrote:
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
I'm kinda new to linux, but tell me any time if there's something I can help!
thanks,
Hyeonggon
participants (2)
-
Hyeonggon Yoo
-
Takashi Iwai