* [RFC PATCH] sound: line6: Fix race condition in line6_probe @ 2021-05-17 13:27 Hyeonggon Yoo 2021-05-17 13:43 ` Takashi Iwai 0 siblings, 1 reply; 6+ messages in thread From: Hyeonggon Yoo @ 2021-05-17 13:27 UTC (permalink / raw) To: Jaroslav Kysela, Takashi Iwai, Greg Kroah-Hartman, Oliver Neukum, Hyeonggon Yoo, Vasily Khoruzhick Cc: alsa-devel, linux-kernel 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", -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] sound: line6: Fix race condition in line6_probe 2021-05-17 13:27 [RFC PATCH] sound: line6: Fix race condition in line6_probe Hyeonggon Yoo @ 2021-05-17 13:43 ` Takashi Iwai 2021-05-17 14:48 ` Hyeonggon Yoo 2021-05-17 14:56 ` Hyeonggon Yoo 0 siblings, 2 replies; 6+ messages in thread From: Takashi Iwai @ 2021-05-17 13:43 UTC (permalink / raw) To: Hyeonggon Yoo Cc: Jaroslav Kysela, Takashi Iwai, Greg Kroah-Hartman, Oliver Neukum, Vasily Khoruzhick, alsa-devel, linux-kernel 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)); ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] sound: line6: Fix race condition in line6_probe 2021-05-17 13:43 ` Takashi Iwai @ 2021-05-17 14:48 ` Hyeonggon Yoo 2021-05-17 14:57 ` Takashi Iwai 2021-05-17 14:56 ` Hyeonggon Yoo 1 sibling, 1 reply; 6+ messages in thread From: Hyeonggon Yoo @ 2021-05-17 14:48 UTC (permalink / raw) To: Takashi Iwai Cc: Jaroslav Kysela, Takashi Iwai, Greg Kroah-Hartman, Oliver Neukum, Vasily Khoruzhick, alsa-devel, linux-kernel 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) > + 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)); ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] sound: line6: Fix race condition in line6_probe 2021-05-17 14:48 ` Hyeonggon Yoo @ 2021-05-17 14:57 ` Takashi Iwai 2021-05-17 15:03 ` Hyeonggon Yoo 0 siblings, 1 reply; 6+ messages in thread From: Takashi Iwai @ 2021-05-17 14:57 UTC (permalink / raw) To: Hyeonggon Yoo Cc: Jaroslav Kysela, Takashi Iwai, Greg Kroah-Hartman, Oliver Neukum, Vasily Khoruzhick, alsa-devel, linux-kernel 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) > > + 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)); > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] sound: line6: Fix race condition in line6_probe 2021-05-17 14:57 ` Takashi Iwai @ 2021-05-17 15:03 ` Hyeonggon Yoo 0 siblings, 0 replies; 6+ messages in thread From: Hyeonggon Yoo @ 2021-05-17 15:03 UTC (permalink / raw) To: Takashi Iwai Cc: Jaroslav Kysela, Takashi Iwai, Greg Kroah-Hartman, Oliver Neukum, Vasily Khoruzhick, alsa-devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] sound: line6: Fix race condition in line6_probe 2021-05-17 13:43 ` Takashi Iwai 2021-05-17 14:48 ` Hyeonggon Yoo @ 2021-05-17 14:56 ` Hyeonggon Yoo 1 sibling, 0 replies; 6+ messages in thread From: Hyeonggon Yoo @ 2021-05-17 14:56 UTC (permalink / raw) To: Takashi Iwai Cc: Jaroslav Kysela, Takashi Iwai, Greg Kroah-Hartman, Oliver Neukum, Vasily Khoruzhick, alsa-devel, linux-kernel, 42.hyeyoo I'm kinda new to linux, but tell me any time if there's something I can help! thanks, Hyeonggon ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-05-17 16:26 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-05-17 13:27 [RFC PATCH] sound: line6: Fix race condition in line6_probe Hyeonggon Yoo 2021-05-17 13:43 ` Takashi Iwai 2021-05-17 14:48 ` Hyeonggon Yoo 2021-05-17 14:57 ` Takashi Iwai 2021-05-17 15:03 ` Hyeonggon Yoo 2021-05-17 14:56 ` Hyeonggon Yoo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox