[PATCH] line6: add midibuf init failure handling in line6_init_midi()

[PATCH] line6: add midibuf init failure handling in line6_init_midi()

[yskelg]

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>
---
 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

Re: [PATCH] line6: add midibuf init failure handling in line6_init_midi()

[Bjorn Helgaas]

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
> 

Re: [PATCH] line6: add midibuf init failure handling in line6_init_midi()

[Takashi Iwai]

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
> 

Re: [PATCH] line6: add midibuf init failure handling in line6_init_midi()

[Yunseong Kim]

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
>>

Re: [PATCH] line6: add midibuf init failure handling in line6_init_midi()

[Takashi Iwai]

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
> >>

Re: [PATCH] line6: add midibuf init failure handling in line6_init_midi()

[Yunseong Kim]

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
>>>>

Re: [PATCH] line6: add midibuf init failure handling in line6_init_midi()

[Bjorn Helgaas]

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
> > 

Re: [PATCH] line6: add midibuf init failure handling in line6_init_midi()

[Yunseong Kim]

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
>>>