On 3/2/24 9:31 AM, Alex Elder wrote:
On 3/1/24 1:04 PM, 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 functionwithout checking for NULL may result in an error. Found by Linux Verification Center (linuxtesting.org) with SVACE.I think this is an actual problem but this might not be the right fix.
The current API for get_channel_from_mode() allows a null pointer to be returned, but it seems that there is at least one case where that should never happen. gb_lights_light_v4l2_register() issues a WARN_ON() if get_channel_from_mode returns NULL (and then proceeds to dereference it). I know BUG_ON() isn't cool, but maybe we should avoid the dereference there. And other than __gb_lights_flash_brightness_set(), all callers properly handle a null pointer return. Regardless of what I said before about commenting for an impossible situation, I think your fix is generally the right thing to do, but it should not return 0 if there is no torch mode channel, it should return -EINVAL or something. Please consider, and post a new version. You could incorporate a similar change in the same patch for gb_lights_light_v4l2_register(). -Alex
The point of the call to get_channel_from_mode() is to get the attached torch channel if the passed-in channel is a flash channel. It's *possible* that any flash channel will *always* have an attached torch channel, but if so there ought to be a comment to that effect near this call (to explain why there's no need for the null pointer check). I think Dan's suggestion should be implemented as well. It's possible the intention really *was* to have get_channel_from_mode() return the original channel pointer if there is no attached channel with the requested mode. But if so, that should be done differently. I.e., Dan's suggestion should be taken, and the callers should use the passed-in channel if the call to get_channel_from_mode() returns NULL. (I hope that's clear.) So anyway, I think this (and Dan's suggestion) should be addressed, but your fix might not be correct. Rui, can you please shed some light on the situation? -AlexFixes: 2870b52bae4c ("greybus: lights: add lights implementation") Signed-off-by: Mikhail Lobanov <m.lobanov@xxxxxxxxxxxx> --- drivers/staging/greybus/light.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.cindex 87d36948c610..929514350947 100644 --- a/drivers/staging/greybus/light.c +++ b/drivers/staging/greybus/light.c@@ -148,10 +148,15 @@ static int __gb_lights_flash_brightness_set(struct gb_channel *channel)GB_CHANNEL_MODE_TORCH); /* For not flash we need to convert brightness to intensity */ - intensity = channel->intensity_uA.min + + + if (channel) { + intensity = channel->intensity_uA.min + (channel->intensity_uA.step * channel->led->brightness); - return __gb_lights_flash_intensity_set(channel, intensity); + return __gb_lights_flash_intensity_set(channel, intensity); + } + + return 0; } #elsestatic struct gb_channel *get_channel_from_cdev(struct led_classdev *cdev)
_______________________________________________ greybus-dev mailing list -- greybus-dev@xxxxxxxxxxxxxxxx To unsubscribe send an email to greybus-dev-leave@xxxxxxxxxxxxxxxx
|