gb_lights_channel_config() builds the LED classdev name with kasprintf() and stores it in cdev->name for every channel during the configuration phase, which runs before the channel is registered.
That name was only released by __gb_lights_led_unregister(), i.e. only for a normal LED channel that was actually registered. It was therefore leaked in two cases:
- a channel that is configured but never registered, e.g. channel_attr_groups_set() fails, the flash configuration fails, or a later channel in the same light fails to configure and the whole light is torn down; the release path calls gb_lights_channel_unregister() (which returns early because the channel is not registered) and then gb_lights_channel_free();
- a flash, torch or indicator channel, whose unregister path (__gb_lights_flash_led_unregister()) never releases cdev->name at all, so the name leaks even on a successful teardown.
cdev->name is a configuration-phase allocation, like channel->color_name and channel->mode_name. Free it in gb_lights_channel_free() next to those, since that runs unconditionally on every teardown path, and drop the special case free from __gb_lights_led_unregister(). gb_lights_channel_unregister() is only ever called from gb_lights_channel_release(), immediately followed by gb_lights_channel_free(), so the name is still released on the registered path and is freed exactly once.
Commit 04820da21050 ("staging: greybus: light: Release memory obtained by kasprintf") fixed the leak for the registered normal-LED path only; this covers the remaining cases.
Fixes: 2870b52bae4c ("greybus: lights: add lights implementation") Signed-off-by: Cong Nguyen congnt264@gmail.com --- drivers/staging/greybus/light.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c index cab02b5da867..1ecca479b5f4 100644 --- a/drivers/staging/greybus/light.c +++ b/drivers/staging/greybus/light.c @@ -905,8 +905,6 @@ static void __gb_lights_led_unregister(struct gb_channel *channel) return;
led_classdev_unregister(cdev); - kfree(cdev->name); - cdev->name = NULL; channel->led = NULL; }
@@ -1063,6 +1061,9 @@ static int gb_lights_light_register(struct gb_light *light)
static void gb_lights_channel_free(struct gb_channel *channel) { + struct led_classdev *cdev = get_channel_cdev(channel); + + kfree(cdev->name); kfree(channel->attrs); kfree(channel->attr_group); kfree(channel->attr_groups);
On Sun, Jul 26, 2026 at 06:00:39PM +0700, Cong Nguyen wrote:
gb_lights_channel_config() builds the LED classdev name with kasprintf() and stores it in cdev->name for every channel during the configuration phase, which runs before the channel is registered.
That name was only released by __gb_lights_led_unregister(), i.e. only for a normal LED channel that was actually registered. It was therefore leaked in two cases:
a channel that is configured but never registered, e.g. channel_attr_groups_set() fails, the flash configuration fails, or a later channel in the same light fails to configure and the whole light is torn down; the release path calls gb_lights_channel_unregister() (which returns early because the channel is not registered) and then gb_lights_channel_free();
a flash, torch or indicator channel, whose unregister path (__gb_lights_flash_led_unregister()) never releases cdev->name at all, so the name leaks even on a successful teardown.
cdev->name is a configuration-phase allocation, like channel->color_name and channel->mode_name. Free it in gb_lights_channel_free() next to those, since that runs unconditionally on every teardown path, and drop the special case free from __gb_lights_led_unregister(). gb_lights_channel_unregister() is only ever called from gb_lights_channel_release(), immediately followed by gb_lights_channel_free(), so the name is still released on the registered path and is freed exactly once.
Commit 04820da21050 ("staging: greybus: light: Release memory obtained by kasprintf") fixed the leak for the registered normal-LED path only; this covers the remaining cases.
Fixes: 2870b52bae4c ("greybus: lights: add lights implementation") Signed-off-by: Cong Nguyen congnt264@gmail.com
drivers/staging/greybus/light.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
Cool, but how did you find this issue? What tool did you use?
thanks,
greg k-h
Hi Greg, I used an LLM-based agent (Claude) to flag allocations that aren't freed on error paths, then I verified this one by hand: I traced every place cdev->name (from kasprintf()) is freed and found that __gb_lights_led_unregister() misses it on the config-failure and flash channel teardown paths. No static-analysis tool was used - just the agent to surface the candidate plus manual review.
thanks, Cong
On Tue, Jul 28, 2026 at 2:17 PM Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
On Sun, Jul 26, 2026 at 06:00:39PM +0700, Cong Nguyen wrote:
gb_lights_channel_config() builds the LED classdev name with kasprintf() and stores it in cdev->name for every channel during the configuration phase, which runs before the channel is registered.
That name was only released by __gb_lights_led_unregister(), i.e. only for a normal LED channel that was actually registered. It was therefore leaked in two cases:
a channel that is configured but never registered, e.g. channel_attr_groups_set() fails, the flash configuration fails, or a later channel in the same light fails to configure and the whole light is torn down; the release path calls gb_lights_channel_unregister() (which returns early because the channel is not registered) and then gb_lights_channel_free();
a flash, torch or indicator channel, whose unregister path (__gb_lights_flash_led_unregister()) never releases cdev->name at all, so the name leaks even on a successful teardown.
cdev->name is a configuration-phase allocation, like channel->color_name and channel->mode_name. Free it in gb_lights_channel_free() next to those, since that runs unconditionally on every teardown path, and drop the special case free from __gb_lights_led_unregister(). gb_lights_channel_unregister() is only ever called from gb_lights_channel_release(), immediately followed by gb_lights_channel_free(), so the name is still released on the registered path and is freed exactly once.
Commit 04820da21050 ("staging: greybus: light: Release memory obtained by kasprintf") fixed the leak for the registered normal-LED path only; this covers the remaining cases.
Fixes: 2870b52bae4c ("greybus: lights: add lights implementation") Signed-off-by: Cong Nguyen congnt264@gmail.com
drivers/staging/greybus/light.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
Cool, but how did you find this issue? What tool did you use?
thanks,
greg k-h
A: http://en.wikipedia.org/wiki/Top_post Q: Were do I find info about this thing called top-posting? A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? A: Top-posting. Q: What is the most annoying thing in e-mail?
A: No. Q: Should I include quotations after my reply?
http://daringfireball.net/2007/07/on_top
On Thu, Jul 30, 2026 at 05:57:15PM +0700, Nguyễn Công wrote:
Hi Greg, I used an LLM-based agent (Claude) to flag allocations that aren't freed on error paths, then I verified this one by hand: I traced every place cdev->name (from kasprintf()) is freed and found that __gb_lights_led_unregister() misses it on the config-failure and flash channel teardown paths. No static-analysis tool was used - just the agent to surface the candidate plus manual review.
Then please properly document this as required.
thanks,
greg k-h
On Thu, Jul 30, 2026 at 6:07 PM Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
A: http://en.wikipedia.org/wiki/Top_post Q: Were do I find info about this thing called top-posting? A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? A: Top-posting. Q: What is the most annoying thing in e-mail?
A: No. Q: Should I include quotations after my reply?
http://daringfireball.net/2007/07/on_top
On Thu, Jul 30, 2026 at 05:57:15PM +0700, Nguyễn Công wrote:
Hi Greg, I used an LLM-based agent (Claude) to flag allocations that aren't freed on error paths, then I verified this one by hand: I traced every place cdev->name (from kasprintf()) is freed and found that __gb_lights_led_unregister() misses it on the config-failure and flash channel teardown paths. No static-analysis tool was used - just the agent to surface the candidate plus manual review.
Then please properly document this as required.
thanks,
greg k-h
Done - added an Assisted-by: tag per and resent as v2: https://lore.kernel.org/linux-staging/20260802060149.3803224-1-congnt264@gma...
Sorry about the top-posting. thanks, Cong
-
Cong Nguyen -
Greg Kroah-Hartman -
Nguyễn Công