| Message ID | 20240516174737.415912-1-yskelg@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | line6: add midibuf init failure handling in line6_init_midi() | expand |
On Fri, May 17, 2024 at 02:47:38AM +0900, yskelg@gmail.com wrote: > From: Yunseong Kim <yskelg@gmail.com> > > This patch fixes potential memory allocation failures in the > line6_midibuf_init(). If either midibuf_in, midibuf_out allocation > line6_midibuf_init call failed, the allocated memory for line6midi > might have been leaked. > > This patch introduces an error handling label and uses goto to jump there > in case of allocation failures. A kfree call is added to release any > partially allocated memory before returning the error code. > > Signed-off-by: Yunseong Kim <yskelg@gmail.com> Hi Yunseong, I don't maintain this area, but since you asked for feedback on IRC: For the subject line, run "git log --oneline sound/usb/line6/midi.c" and match the style, i.e., in this case it should be: ALSA: line6: <Capitalized verb> ... "Add init failure handling" is not very specific; I think it's worth including the key word "leak" in the subject line. Remove text like "this patch". We already know which patch the commit log refers to. Use imperative mood in the commit log, not "introduces", "uses", "is added", etc. Details: https://chris.beams.io/posts/git-commit/ https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=v6.9#n94 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/maintainer-tip.rst?id=v6.9#n134 The second paragraph ("... introduces an error handling label ...") basically translates the patch from C to English. No need for that since we can read the C. The commit log can be higher level to explain why the patch should be merged. In this case, the error paths leak the snd_line6_midi allocation (not "might"; it's definitely a leak). This case is so simple that you don't need much of a commit log. Adding too much detail almost obscures the point. Something like this would probably be sufficient: Free line6midi in error paths to avoid leaking the allocation. Nice work, good luck! Bjorn > --- > sound/usb/line6/midi.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/sound/usb/line6/midi.c b/sound/usb/line6/midi.c > index 0838632c788e..abcf58f46673 100644 > --- a/sound/usb/line6/midi.c > +++ b/sound/usb/line6/midi.c > @@ -283,13 +283,18 @@ int line6_init_midi(struct usb_line6 *line6) > > err = line6_midibuf_init(&line6midi->midibuf_in, MIDI_BUFFER_SIZE, 0); > if (err < 0) > - return err; > + goto error; > > err = line6_midibuf_init(&line6midi->midibuf_out, MIDI_BUFFER_SIZE, 1); > if (err < 0) > - return err; > + goto error; > > line6->line6midi = line6midi; > return 0; > + > +error: > + kfree(line6midi); > + return err; > + > } > EXPORT_SYMBOL_GPL(line6_init_midi); > -- > 2.34.1 >
On Thu, 16 May 2024 19:47:38 +0200, yskelg@gmail.com wrote: > > From: Yunseong Kim <yskelg@gmail.com> > > This patch fixes potential memory allocation failures in the > line6_midibuf_init(). If either midibuf_in, midibuf_out allocation > line6_midibuf_init call failed, the allocated memory for line6midi > might have been leaked. > > This patch introduces an error handling label and uses goto to jump there > in case of allocation failures. A kfree call is added to release any > partially allocated memory before returning the error code. > > Signed-off-by: Yunseong Kim <yskelg@gmail.com> The allocated object is already freed by snd_line6_midi_free() that is called via rawmidi private_free at its destruction. So your change would lead to a double-free. thanks, Takashi > --- > sound/usb/line6/midi.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/sound/usb/line6/midi.c b/sound/usb/line6/midi.c > index 0838632c788e..abcf58f46673 100644 > --- a/sound/usb/line6/midi.c > +++ b/sound/usb/line6/midi.c > @@ -283,13 +283,18 @@ int line6_init_midi(struct usb_line6 *line6) > > err = line6_midibuf_init(&line6midi->midibuf_in, MIDI_BUFFER_SIZE, 0); > if (err < 0) > - return err; > + goto error; > > err = line6_midibuf_init(&line6midi->midibuf_out, MIDI_BUFFER_SIZE, 1); > if (err < 0) > - return err; > + goto error; > > line6->line6midi = line6midi; > return 0; > + > +error: > + kfree(line6midi); > + return err; > + > } > EXPORT_SYMBOL_GPL(line6_init_midi); > -- > 2.34.1 >
On 5/17/24 5:32 오후, Takashi Iwai wrote: > The allocated object is already freed by snd_line6_midi_free() that is > called via rawmidi private_free at its destruction. So your change > would lead to a double-free. > > > thanks, > > Takashi Thanks for the review Takashi. I have one question: line6_midibuf_destroy() have no NULL check for the memory to free. If line6midi->midibuf_in is in this->buf = NULL from line6_midibuf_init() with memory allocation failed, won't the free(NULL) when accessed by snd_line6_midi_free() to line6_midibuf_destroy()? In the first patch, I was making a misunderstanding Now that you mention it, I can see where it's freeing! It helped me a lot in analyzing the driver code. Please let me know if I've misunderstood anything. Warm Regards, Yunseong Kim >> --- >> sound/usb/line6/midi.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/sound/usb/line6/midi.c b/sound/usb/line6/midi.c >> index 0838632c788e..abcf58f46673 100644 >> --- a/sound/usb/line6/midi.c >> +++ b/sound/usb/line6/midi.c >> @@ -283,13 +283,18 @@ int line6_init_midi(struct usb_line6 *line6) >> >> err = line6_midibuf_init(&line6midi->midibuf_in, MIDI_BUFFER_SIZE, 0); >> if (err < 0) >> - return err; >> + goto error; >> >> err = line6_midibuf_init(&line6midi->midibuf_out, MIDI_BUFFER_SIZE, 1); >> if (err < 0) >> - return err; >> + goto error; >> >> line6->line6midi = line6midi; >> return 0; >> + >> +error: >> + kfree(line6midi); >> + return err; >> + >> } >> EXPORT_SYMBOL_GPL(line6_init_midi); >> -- >> 2.34.1 >>
On Fri, May 17, 2024 at 10:32:13AM +0200, Takashi Iwai wrote: > On Thu, 16 May 2024 19:47:38 +0200, > yskelg@gmail.com wrote: > > > > From: Yunseong Kim <yskelg@gmail.com> > > > > This patch fixes potential memory allocation failures in the > > line6_midibuf_init(). If either midibuf_in, midibuf_out allocation > > line6_midibuf_init call failed, the allocated memory for line6midi > > might have been leaked. > > > > This patch introduces an error handling label and uses goto to jump there > > in case of allocation failures. A kfree call is added to release any > > partially allocated memory before returning the error code. > > > > Signed-off-by: Yunseong Kim <yskelg@gmail.com> > > The allocated object is already freed by snd_line6_midi_free() that is > called via rawmidi private_free at its destruction. So your change > would lead to a double-free. I stand corrected, sorry that I missed that! That said, it seems unreasonably hard to verify this path to snd_line6_midi_free(). Sorry for misleading you, Yunseong. > > --- > > sound/usb/line6/midi.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/sound/usb/line6/midi.c b/sound/usb/line6/midi.c > > index 0838632c788e..abcf58f46673 100644 > > --- a/sound/usb/line6/midi.c > > +++ b/sound/usb/line6/midi.c > > @@ -283,13 +283,18 @@ int line6_init_midi(struct usb_line6 *line6) > > > > err = line6_midibuf_init(&line6midi->midibuf_in, MIDI_BUFFER_SIZE, 0); > > if (err < 0) > > - return err; > > + goto error; > > > > err = line6_midibuf_init(&line6midi->midibuf_out, MIDI_BUFFER_SIZE, 1); > > if (err < 0) > > - return err; > > + goto error; > > > > line6->line6midi = line6midi; > > return 0; > > + > > +error: > > + kfree(line6midi); > > + return err; > > + > > } > > EXPORT_SYMBOL_GPL(line6_init_midi); > > -- > > 2.34.1 > >
On Fri, 17 May 2024 19:10:44 +0200, Yunseong Kim wrote: > > On 5/17/24 5:32 오후, Takashi Iwai wrote: > > The allocated object is already freed by snd_line6_midi_free() that is > > called via rawmidi private_free at its destruction. So your change > > would lead to a double-free. > > > > > > thanks, > > > > Takashi > > Thanks for the review Takashi. > > I have one question: > > line6_midibuf_destroy() have no NULL check for the memory to free. > > If line6midi->midibuf_in is in this->buf = NULL from > line6_midibuf_init() with memory allocation failed, > won't the free(NULL) when accessed by snd_line6_midi_free() to > line6_midibuf_destroy()? free(NULL) is fine, it's defined to be noop. Many codes just call free() unconditionally for that, as it reduces the code size. thanks, Takashi > In the first patch, I was making a misunderstanding > Now that you mention it, I can see where it's freeing! > It helped me a lot in analyzing the driver code. > > Please let me know if I've misunderstood anything. > > Warm Regards, > Yunseong Kim > > >> --- > >> sound/usb/line6/midi.c | 9 +++++++-- > >> 1 file changed, 7 insertions(+), 2 deletions(-) > >> > >> diff --git a/sound/usb/line6/midi.c b/sound/usb/line6/midi.c > >> index 0838632c788e..abcf58f46673 100644 > >> --- a/sound/usb/line6/midi.c > >> +++ b/sound/usb/line6/midi.c > >> @@ -283,13 +283,18 @@ int line6_init_midi(struct usb_line6 *line6) > >> > >> err = line6_midibuf_init(&line6midi->midibuf_in, MIDI_BUFFER_SIZE, 0); > >> if (err < 0) > >> - return err; > >> + goto error; > >> > >> err = line6_midibuf_init(&line6midi->midibuf_out, MIDI_BUFFER_SIZE, 1); > >> if (err < 0) > >> - return err; > >> + goto error; > >> > >> line6->line6midi = line6midi; > >> return 0; > >> + > >> +error: > >> + kfree(line6midi); > >> + return err; > >> + > >> } > >> EXPORT_SYMBOL_GPL(line6_init_midi); > >> -- > >> 2.34.1 > >>
On 5/18/24 2:44 오전, Bjorn Helgaas wrote: > On Fri, May 17, 2024 at 10:32:13AM +0200, Takashi Iwai wrote: >> On Thu, 16 May 2024 19:47:38 +0200, >> yskelg@gmail.com wrote: >>> >>> From: Yunseong Kim <yskelg@gmail.com> >>> >>> This patch fixes potential memory allocation failures in the >>> line6_midibuf_init(). If either midibuf_in, midibuf_out allocation >>> line6_midibuf_init call failed, the allocated memory for line6midi >>> might have been leaked. >>> >>> This patch introduces an error handling label and uses goto to jump there >>> in case of allocation failures. A kfree call is added to release any >>> partially allocated memory before returning the error code. >>> >>> Signed-off-by: Yunseong Kim <yskelg@gmail.com> >> >> The allocated object is already freed by snd_line6_midi_free() that is >> called via rawmidi private_free at its destruction. So your change >> would lead to a double-free. > > I stand corrected, sorry that I missed that! > > That said, it seems unreasonably hard to verify this path to > snd_line6_midi_free(). > > Sorry for misleading you, Yunseong. Thank you Bjorn for your detailed advice. I will take them to heart and work on them in the future. Warm Regards, Yunseong Kim >>> --- >>> sound/usb/line6/midi.c | 9 +++++++-- >>> 1 file changed, 7 insertions(+), 2 deletions(-) >>> >>> diff --git a/sound/usb/line6/midi.c b/sound/usb/line6/midi.c >>> index 0838632c788e..abcf58f46673 100644 >>> --- a/sound/usb/line6/midi.c >>> +++ b/sound/usb/line6/midi.c >>> @@ -283,13 +283,18 @@ int line6_init_midi(struct usb_line6 *line6) >>> >>> err = line6_midibuf_init(&line6midi->midibuf_in, MIDI_BUFFER_SIZE, 0); >>> if (err < 0) >>> - return err; >>> + goto error; >>> >>> err = line6_midibuf_init(&line6midi->midibuf_out, MIDI_BUFFER_SIZE, 1); >>> if (err < 0) >>> - return err; >>> + goto error; >>> >>> line6->line6midi = line6midi; >>> return 0; >>> + >>> +error: >>> + kfree(line6midi); >>> + return err; >>> + >>> } >>> EXPORT_SYMBOL_GPL(line6_init_midi); >>> -- >>> 2.34.1 >>>
On 5/18/24 2:51 오전, Takashi Iwai wrote: > On Fri, 17 May 2024 19:10:44 +0200, > Yunseong Kim wrote: >> >> On 5/17/24 5:32 오후, Takashi Iwai wrote: >>> The allocated object is already freed by snd_line6_midi_free() that is >>> called via rawmidi private_free at its destruction. So your change >>> would lead to a double-free. >>> >>> >>> thanks, >>> >>> Takashi >> >> Thanks for the review Takashi. >> >> I have one question: >> >> line6_midibuf_destroy() have no NULL check for the memory to free. >> >> If line6midi->midibuf_in is in this->buf = NULL from >> line6_midibuf_init() with memory allocation failed, >> won't the free(NULL) when accessed by snd_line6_midi_free() to >> line6_midibuf_destroy()? > > free(NULL) is fine, it's defined to be noop. > Many codes just call free() unconditionally for that, as it reduces > the code size. > > > thanks, > > Takashi Thak you Takashi, I've been studying the kfree behavior in detail since you mentioned it, so thanks for advicing the details. Warm Regards, Yunseong Kim >> >>>> --- >>>> sound/usb/line6/midi.c | 9 +++++++-- >>>> 1 file changed, 7 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/sound/usb/line6/midi.c b/sound/usb/line6/midi.c >>>> index 0838632c788e..abcf58f46673 100644 >>>> --- a/sound/usb/line6/midi.c >>>> +++ b/sound/usb/line6/midi.c >>>> @@ -283,13 +283,18 @@ int line6_init_midi(struct usb_line6 *line6) >>>> >>>> err = line6_midibuf_init(&line6midi->midibuf_in, MIDI_BUFFER_SIZE, 0); >>>> if (err < 0) >>>> - return err; >>>> + goto error; >>>> >>>> err = line6_midibuf_init(&line6midi->midibuf_out, MIDI_BUFFER_SIZE, 1); >>>> if (err < 0) >>>> - return err; >>>> + goto error; >>>> >>>> line6->line6midi = line6midi; >>>> return 0; >>>> + >>>> +error: >>>> + kfree(line6midi); >>>> + return err; >>>> + >>>> } >>>> EXPORT_SYMBOL_GPL(line6_init_midi); >>>> -- >>>> 2.34.1 >>>>
diff --git a/sound/usb/line6/midi.c b/sound/usb/line6/midi.c index 0838632c788e..abcf58f46673 100644 --- a/sound/usb/line6/midi.c +++ b/sound/usb/line6/midi.c @@ -283,13 +283,18 @@ int line6_init_midi(struct usb_line6 *line6) err = line6_midibuf_init(&line6midi->midibuf_in, MIDI_BUFFER_SIZE, 0); if (err < 0) - return err; + goto error; err = line6_midibuf_init(&line6midi->midibuf_out, MIDI_BUFFER_SIZE, 1); if (err < 0) - return err; + goto error; line6->line6midi = line6midi; return 0; + +error: + kfree(line6midi); + return err; + } EXPORT_SYMBOL_GPL(line6_init_midi);