gb_lights_light_config() stores channel_count before allocating the channels array. If kcalloc() fails, gb_lights_release() iterates the non-zero count and dereferences light->channels, which is NULL.
Allocate channels first and only then publish channels_count so the cleanup path can't walk a NULL pointer.
Fixes: 2870b52bae4c ("greybus: lights: add lights implementation") Signed-off-by: Chaitanya Mishra chaitanyamishra.ai@gmail.com --- drivers/staging/greybus/light.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c index e509fdc715db..4c9ad9ea8827 100644 --- a/drivers/staging/greybus/light.c +++ b/drivers/staging/greybus/light.c @@ -1008,14 +1008,14 @@ static int gb_lights_light_config(struct gb_lights *glights, u8 id) if (!strlen(conf.name)) return -EINVAL;
- light->channels_count = conf.channel_count; light->name = kstrndup(conf.name, NAMES_MAX, GFP_KERNEL); if (!light->name) return -ENOMEM; - light->channels = kcalloc(light->channels_count, + light->channels = kcalloc(conf.channel_count, sizeof(struct gb_channel), GFP_KERNEL); if (!light->channels) return -ENOMEM; + light->channels_count = conf.channel_count;
/* First we collect all the configurations for all channels */ for (i = 0; i < light->channels_count; i++) {
On Thu, Jan 08, 2026 at 04:07:00PM +0530, Chaitanya Mishra wrote:
gb_lights_light_config() stores channel_count before allocating the channels array. If kcalloc() fails, gb_lights_release() iterates the non-zero count and dereferences light->channels, which is NULL.
Allocate channels first and only then publish channels_count so the cleanup path can't walk a NULL pointer.
How was this issue found? How was the fix generated? How was it tested?
Fixes: 2870b52bae4c ("greybus: lights: add lights implementation") Signed-off-by: Chaitanya Mishra chaitanyamishra.ai@gmail.com
drivers/staging/greybus/light.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c index e509fdc715db..4c9ad9ea8827 100644 --- a/drivers/staging/greybus/light.c +++ b/drivers/staging/greybus/light.c @@ -1008,14 +1008,14 @@ static int gb_lights_light_config(struct gb_lights *glights, u8 id) if (!strlen(conf.name)) return -EINVAL;
- light->channels_count = conf.channel_count; light->name = kstrndup(conf.name, NAMES_MAX, GFP_KERNEL); if (!light->name) return -ENOMEM;
- light->channels = kcalloc(light->channels_count,
- light->channels = kcalloc(conf.channel_count, sizeof(struct gb_channel), GFP_KERNEL); if (!light->channels) return -ENOMEM;
- light->channels_count = conf.channel_count;
This is "tricky", perhaps add a comment here as to why you are only assigning this now and not before?
thanks,
greg k-h
gb_lights_light_config() stores channel_count before allocating the channels array. If kcalloc() fails, gb_lights_release() iterates the non-zero count and dereferences light->channels, which is NULL.
Allocate channels first and only then publish channels_count so the cleanup path can't walk a NULL pointer.
Fixes: 2870b52bae4c ("greybus: lights: add lights implementation") Signed-off-by: Chaitanya Mishra chaitanyamishra.ai@gmail.com --- drivers/staging/greybus/light.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c index e509fdc715db..38c233a706c4 100644 --- a/drivers/staging/greybus/light.c +++ b/drivers/staging/greybus/light.c @@ -1008,14 +1008,18 @@ static int gb_lights_light_config(struct gb_lights *glights, u8 id) if (!strlen(conf.name)) return -EINVAL;
- light->channels_count = conf.channel_count; light->name = kstrndup(conf.name, NAMES_MAX, GFP_KERNEL); if (!light->name) return -ENOMEM; - light->channels = kcalloc(light->channels_count, + light->channels = kcalloc(conf.channel_count, sizeof(struct gb_channel), GFP_KERNEL); if (!light->channels) return -ENOMEM; + /* + * Publish channels_count only after channels allocation so cleanup + * doesn't walk a NULL channels pointer on allocation failure. + */ + light->channels_count = conf.channel_count;
/* First we collect all the configurations for all channels */ for (i = 0; i < light->channels_count; i++) {
On Thu, Jan 08, 2026 at 04:19:47PM +0530, Chaitanya Mishra wrote:
gb_lights_light_config() stores channel_count before allocating the channels array. If kcalloc() fails, gb_lights_release() iterates the non-zero count and dereferences light->channels, which is NULL.
Allocate channels first and only then publish channels_count so the cleanup path can't walk a NULL pointer.
Fixes: 2870b52bae4c ("greybus: lights: add lights implementation") Signed-off-by: Chaitanya Mishra chaitanyamishra.ai@gmail.com
drivers/staging/greybus/light.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c index e509fdc715db..38c233a706c4 100644 --- a/drivers/staging/greybus/light.c +++ b/drivers/staging/greybus/light.c @@ -1008,14 +1008,18 @@ static int gb_lights_light_config(struct gb_lights *glights, u8 id) if (!strlen(conf.name)) return -EINVAL;
- light->channels_count = conf.channel_count; light->name = kstrndup(conf.name, NAMES_MAX, GFP_KERNEL); if (!light->name) return -ENOMEM;
- light->channels = kcalloc(light->channels_count,
- light->channels = kcalloc(conf.channel_count, sizeof(struct gb_channel), GFP_KERNEL); if (!light->channels) return -ENOMEM;
- /*
* Publish channels_count only after channels allocation so cleanup* doesn't walk a NULL channels pointer on allocation failure.*/- light->channels_count = conf.channel_count;
/* First we collect all the configurations for all channels */ for (i = 0; i < light->channels_count; i++) { -- 2.50.1 (Apple Git-155)
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him a patch that has triggered this response. He used to manually respond to these common problems, but in order to save his sanity (he kept writing the same thing over and over, yet to different people), I was created. Hopefully you will not take offence and will fix the problem in your patch and resubmit it so that it can be accepted into the Linux kernel tree.
You are receiving this message because of the following common error(s) as indicated below:
- This looks like a new version of a previously submitted patch, but you did not list below the --- line any changes from the previous version. Please read the section entitled "The canonical patch format" in the kernel file, Documentation/process/submitting-patches.rst for what needs to be done here to properly describe this.
If you wish to discuss this problem further, or you have questions about how to resolve this issue, please feel free to respond to this email and Greg will reply once he has dug out from the pending patches received from other developers.
thanks,
greg k-h's patch email bot
Hi Greg,
Found by manual code review while walking the error paths in Gb_lights_light_config(): channels_count is set before channels allocation, but cleanup uses channels_count to iterate and dereference light->channels. If kcalloc() fails, that becomes a NULL deref.
Fix is simply deferring channels_count publication until after the allocation succeeds; v2 includes the requested comment.
Tested with: ./scripts/checkpatch.pl --strict -g HEAD ./scripts/checkpatch.pl outgoing/0001-staging-greybus-lights-avoid-NULL-deref.patch
I couldn't build-test locally on macOS due to missing <elf.h> for kernel host tools.
Thanks, Chaitanya
On Thu, Jan 08, 2026 at 04:33:51PM +0530, Chaitanya Mishra wrote:
Hi Greg,
Found by manual code review while walking the error paths in Gb_lights_light_config(): channels_count is set before channels allocation, but cleanup uses channels_count to iterate and dereference light->channels. If kcalloc() fails, that becomes a NULL deref.
Might I ask why are you manually reviewing the error code paths of this driver? Do you have this hardware somewhere?
Fix is simply deferring channels_count publication until after the allocation succeeds; v2 includes the requested comment.
Tested with: ./scripts/checkpatch.pl --strict -g HEAD ./scripts/checkpatch.pl outgoing/0001-staging-greybus-lights-avoid-NULL-deref.patch
I couldn't build-test locally on macOS due to missing <elf.h> for kernel host tools.
For obvious reasons, sending out patches that you didn't even build test is probably not a good idea :)
thanks,
greg k-h
Hey Chaitanya, Thanks for the patch.
On Thu Jan 8, 2026 at 10:49 AM WET, Chaitanya Mishra wrote:
gb_lights_light_config() stores channel_count before allocating the channels array. If kcalloc() fails, gb_lights_release() iterates the non-zero count and dereferences light->channels, which is NULL.
Allocate channels first and only then publish channels_count so the cleanup path can't walk a NULL pointer.
Good catch, going through the error path, does looks this fix the issue.
But you need to add the changes between versions bellow -- and maybe a link to the first version.
Reviewed-by: Rui Miguel Silva rui.silva@linaro.org
Cheers, Rui
Fixes: 2870b52bae4c ("greybus: lights: add lights implementation") Signed-off-by: Chaitanya Mishra chaitanyamishra.ai@gmail.com
drivers/staging/greybus/light.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c index e509fdc715db..38c233a706c4 100644 --- a/drivers/staging/greybus/light.c +++ b/drivers/staging/greybus/light.c @@ -1008,14 +1008,18 @@ static int gb_lights_light_config(struct gb_lights *glights, u8 id) if (!strlen(conf.name)) return -EINVAL;
- light->channels_count = conf.channel_count; light->name = kstrndup(conf.name, NAMES_MAX, GFP_KERNEL); if (!light->name) return -ENOMEM;
- light->channels = kcalloc(light->channels_count,
- light->channels = kcalloc(conf.channel_count, sizeof(struct gb_channel), GFP_KERNEL); if (!light->channels) return -ENOMEM;
- /*
* Publish channels_count only after channels allocation so cleanup* doesn't walk a NULL channels pointer on allocation failure.*/- light->channels_count = conf.channel_count;
/* First we collect all the configurations for all channels */ for (i = 0; i < light->channels_count; i++) { -- 2.50.1 (Apple Git-155)
gb_lights_light_config() stores channel_count before allocating the channels array. If kcalloc() fails, gb_lights_release() iterates the non-zero count and dereferences light->channels, which is NULL.
Allocate channels first and only then publish channels_count so the cleanup path can't walk a NULL pointer.
Fixes: 2870b52bae4c ("greybus: lights: add lights implementation") Signed-off-by: Chaitanya Mishra chaitanyamishra.ai@gmail.com --- Changes in v3: - Add version changelog below the --- line (no code changes).
drivers/staging/greybus/light.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c index e509fdc715db..38c233a706c4 100644 --- a/drivers/staging/greybus/light.c +++ b/drivers/staging/greybus/light.c @@ -1008,14 +1008,18 @@ static int gb_lights_light_config(struct gb_lights *glights, u8 id) if (!strlen(conf.name)) return -EINVAL;
- light->channels_count = conf.channel_count; light->name = kstrndup(conf.name, NAMES_MAX, GFP_KERNEL); if (!light->name) return -ENOMEM; - light->channels = kcalloc(light->channels_count, + light->channels = kcalloc(conf.channel_count, sizeof(struct gb_channel), GFP_KERNEL); if (!light->channels) return -ENOMEM; + /* + * Publish channels_count only after channels allocation so cleanup + * doesn't walk a NULL channels pointer on allocation failure. + */ + light->channels_count = conf.channel_count;
/* First we collect all the configurations for all channels */ for (i = 0; i < light->channels_count; i++) {
Hi Rui,
Thanks for the review.
I sent v3 with the version history below the --- line (no code changes). I couldn't add a lore link yet since the first version doesn't appear to be indexed; if it shows up and a respin is needed, I can add a Link: tag and carry your Reviewed-by.
Thanks, Chaitanya
On Thu, Jan 08, 2026 at 04:39:26PM +0530, Chaitanya Mishra wrote:
Hi Rui,
Thanks for the review.
I sent v3 with the version history below the --- line (no code changes). I couldn't add a lore link yet since the first version doesn't appear to be indexed;
I see it there: https://lore.kernel.org/all/20260108103700.15384-1-chaitanyamishra.ai@gmail....
if it shows up and a respin is needed, I can add a Link: tag and carry your Reviewed-by.
You should have added it from v2.
Also, slow down, there's no rush here. Usually you should wait at least a day between new versions, right?
thanks,
greg k-h
Hi Greg,
You're right on both points. I don't have the hardware; I was walking error paths in staging drivers to learn and look for obvious bugs, but I shouldn't have sent a patch without a build test. Sorry for the noise.
I'll set up a Linux build environment, rerun at least a module build with checkpatch, and wait before posting any v4. If a respin is still needed, I'll add a Link: tag to v1, include Rui's Reviewed-by, and keep a proper changelog below the --- line. Please ignore v3 for now.
Thanks, Chaitanya
-
Chaitanya Mishra -
Greg KH -
Rui Miguel Silva