Skip to contentSkip to conversation

line6: add midibuf init failure handling in line6_init_midi()

PATCH In review [email protected] 3 participants 8 messages May 16, 2024 – May 18, 2024 Sound

lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/1 line6: add midibuf init failure handling in line6_init_midi()

From: Yunseong Kim <[email protected]>

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 <[email protected]>
---
 sound/usb/line6/midi.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
sound/usb/line6/midi.c +7 -2
index 0838632c788e..abcf58f46673 100644
@@ -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);
Signature · 1 line
2.34.1
#1 lore raw
Quoted 7 lines · On Fri, May 17, 2024 at 02:47:38AM +0900, [email protected] wrote:

From: Yunseong Kim <[email protected]>

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.

Quoted 5 lines

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 <[email protected]>

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

Quoted 32 lines
---
 sound/usb/line6/midi.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
sound/usb/line6/midi.c +7 -2
index 0838632c788e..abcf58f46673 100644
@@ -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);
Signature · 1 line
2.34.1
#2 lore raw
In reply to [email protected]
Quoted 8 lines · On Thu, 16 May 2024 19:47:38 +0200, [email protected] wrote:

From: Yunseong Kim <[email protected]>

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.

Quoted 5 lines

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 <[email protected]>

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

Quoted 32 lines
---
 sound/usb/line6/midi.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
sound/usb/line6/midi.c +7 -2
index 0838632c788e..abcf58f46673 100644
@@ -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);
Signature · 1 line
2.34.1
author
#3 lore raw
Quoted 8 lines · Takashi Iwai

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

Quoted 32 lines
Quoted 32 lines
---
 sound/usb/line6/midi.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
sound/usb/line6/midi.c +7 -2
index 0838632c788e..abcf58f46673 100644
@@ -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);
Signature · 1 line
2.34.1
#4 lore raw
Quoted 13 lines · Yunseong Kim
Quoted 8 lines · Takashi Iwai

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.

Quoted 8 lines

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

Quoted 41 lines

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

Quoted 32 lines
Quoted 32 lines
---
 sound/usb/line6/midi.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
sound/usb/line6/midi.c +7 -2
index 0838632c788e..abcf58f46673 100644
@@ -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);
Signature · 1 line
2.34.1
author
#5 lore raw
Quoted 24 lines · Takashi Iwai
Quoted 13 lines · Yunseong Kim
Quoted 8 lines · Takashi Iwai

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.

Quoted 8 lines

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()?

Quoted 8 lines

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

Quoted 33 lines
Quoted 33 lines
Quoted 33 lines
Quoted 33 lines
---
 sound/usb/line6/midi.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
sound/usb/line6/midi.c +7 -2
index 0838632c788e..abcf58f46673 100644
@@ -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);
Signature · 1 line
2.34.1
#6 lore raw
In reply to Takashi Iwai
Quoted 14 lines · Takashi Iwai
Quoted 12 lines · On Thu, 16 May 2024 19:47:38 +0200, [email protected] wrote:

From: Yunseong Kim <[email protected]>

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.

Quoted 5 lines
Quoted 1 line
Signed-off-by Yunseong Kim <[email protected]>

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.

Quoted 32 lines
Quoted 32 lines
---
 sound/usb/line6/midi.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
sound/usb/line6/midi.c +7 -2
index 0838632c788e..abcf58f46673 100644
@@ -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);
Signature · 1 line
2.34.1
author
#7 lore raw
Quoted 21 lines · Bjorn Helgaas
Quoted 14 lines · Takashi Iwai
Quoted 12 lines · On Thu, 16 May 2024 19:47:38 +0200, [email protected] wrote:

From: Yunseong Kim <[email protected]>

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.

Quoted 5 lines
Quoted 1 line
Signed-off-by Yunseong Kim <[email protected]>

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.

Quoted 6 lines

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

Quoted 32 lines
Quoted 32 lines
Quoted 32 lines
---
 sound/usb/line6/midi.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
sound/usb/line6/midi.c +7 -2
index 0838632c788e..abcf58f46673 100644
@@ -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);
Signature · 1 line
2.34.1