Alex Elder <elder@xxxxxxxx> writes: Hey Alex, > On 3/2/24 3:59 AM, Dan Carpenter wrote: >> On Fri, Mar 01, 2024 at 02:04:24PM -0500, Mikhail Lobanov wrote: >>> Dereference of null pointer in the __gb_lights_flash_brightness_set function. >>> Assigning the channel the result of executing the get_channel_from_mode function >>> without checking for NULL may result in an error. >> >> get_channel_from_mode() can only return NULL when light->channels_count >> is zero. >> >> Although get_channel_from_mode() seems buggy to me. If it can't >> find the correct mode, it just returns the last channel. So potentially >> it should be made to return NULL. > > I agree with you. This looks quite wrong to me, and I > like your fix, *except* there is also no need to check > whether the channel pointer is null inside the loop. > It's the address of an object, and will always be non-null. > > static struct gb_channel * > get_channel_from_mode(struct gb_light *light, u32 mode) > { > struct gb_channel *channel; > u32 i; > > for (i = 0; i < light->channels_count; i++) { > channel = &light->channels[i]; > if (channel->mode == mode) > return channel; > } > return NULL; > } > > > Rui, could you please confirm what Dan says (and his > proposed change) was your intention? Yup, Dan is right. > > If so (and assuming you also fix the check for a null > channel pointer inside the loop): And you also here. > > Reviewed-by: Alex Elder <elder@xxxxxxxxxx> Thanks. Cheers, Rui > > -Alex > >> >> diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c >> index d62f97249aca..acd435f5d25d 100644 >> --- a/drivers/staging/greybus/light.c >> +++ b/drivers/staging/greybus/light.c >> @@ -95,15 +95,15 @@ static struct led_classdev *get_channel_cdev(struct gb_channel *channel) >> static struct gb_channel *get_channel_from_mode(struct gb_light *light, >> u32 mode) >> { >> - struct gb_channel *channel = NULL; >> + struct gb_channel *channel; >> int i; >> >> for (i = 0; i < light->channels_count; i++) { >> channel = &light->channels[i]; >> if (channel && channel->mode == mode) >> - break; >> + return channel; >> } >> - return channel; >> + return NULL; >> } >> >> static int __gb_lights_flash_intensity_set(struct gb_channel *channel, >> _______________________________________________ >> greybus-dev mailing list -- greybus-dev@xxxxxxxxxxxxxxxx >> To unsubscribe send an email to greybus-dev-leave@xxxxxxxxxxxxxxxx _______________________________________________ greybus-dev mailing list -- greybus-dev@xxxxxxxxxxxxxxxx To unsubscribe send an email to greybus-dev-leave@xxxxxxxxxxxxxxxx