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.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: 2870b52bae4c ("greybus: lights: add lights implementation") Signed-off-by: Mikhail Lobanov m.lobanov@rosalinux.ru --- 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.c index 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; } #else static struct gb_channel *get_channel_from_cdev(struct led_classdev *cdev)
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.
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,
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?
If so (and assuming you also fix the check for a null channel pointer inside the loop):
Reviewed-by: Alex Elder elder@linaro.org
-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@lists.linaro.org To unsubscribe send an email to greybus-dev-leave@lists.linaro.org
Hi Mikhail, Thanks for your patch.
Mikhail Lobanov m.lobanov@rosalinux.ru writes:
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.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: 2870b52bae4c ("greybus: lights: add lights implementation") Signed-off-by: Mikhail Lobanov m.lobanov@rosalinux.ru
Yeah, at the time when this was implemented I recall that we could only set the brightness of the torch mode in a flash led, not in the flash only mode. So, if we were getting here was that for sure we had a torch channel and get_channel_from_mode will always find a channel, so never returning null here.
but yeah, this is safer. but maybe just do something like the bellow would be simpler: modified drivers/staging/greybus/light.c @@ -147,6 +147,9 @@ static int __gb_lights_flash_brightness_set(struct gb_channel *channel) channel = get_channel_from_mode(channel->light, GB_CHANNEL_MODE_TORCH);
+ if (!channel) + return -EINVAL; + /* For not flash we need to convert brightness to intensity */ intensity = channel->intensity_uA.min + (channel->intensity_uA.step * channel->led->brightness);
what do you think?
Cheers, Rui
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.c index 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;
} #else static struct gb_channel *get_channel_from_cdev(struct led_classdev *cdev) -- 2.43.0
Alex Elder elder@ieee.org 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@linaro.org
Thanks.
Cheers, Rui
-Alexdiff --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@lists.linaro.org To unsubscribe send an email to greybus-dev-leave@lists.linaro.org
Dan Carpenter dan.carpenter@linaro.org writes: Hi Dan,
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.
Correct, thanks for the fix. Will you or me send a proper patch for this? Taking also the suggestion from Alex.
Thanks in advance.
Cheers, Rui
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,
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 function without 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 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?
-Alex
Fixes: 2870b52bae4c ("greybus: lights: add lights implementation") Signed-off-by: Mikhail Lobanov m.lobanov@rosalinux.ru
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.c index 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; } #else static struct gb_channel *get_channel_from_cdev(struct led_classdev *cdev)
Hi Alex, Alex Elder elder@ieee.org writes:
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 function without 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 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?
As we talked, this email was sent at the same time as my replies to this thread and you think I addressed your concerns in that replies. If not, just go ahead and ask again.
Cheers, Rui
-AlexFixes: 2870b52bae4c ("greybus: lights: add lights implementation") Signed-off-by: Mikhail Lobanov m.lobanov@rosalinux.ru
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.c index 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; } #else static struct gb_channel *get_channel_from_cdev(struct led_classdev *cdev)
On Sat, Mar 02, 2024 at 03:23:03PM +0000, Rui Miguel Silva wrote:
Dan Carpenter dan.carpenter@linaro.org writes: Hi Dan,
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.
Correct, thanks for the fix. Will you or me send a proper patch for this? Taking also the suggestion from Alex.
I'll send it. Thanks!
regards, dan carpenter
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 function without 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?
-Alex
Fixes: 2870b52bae4c ("greybus: lights: add lights implementation") Signed-off-by: Mikhail Lobanov m.lobanov@rosalinux.ru
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.c index 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; } #else static struct gb_channel *get_channel_from_cdev(struct led_classdev *cdev)
On 3/2/24 10:35 AM, Rui Miguel Silva wrote:
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?
As we talked, this email was sent at the same time as my replies to this thread and you think I addressed your concerns in that replies. If not, just go ahead and ask again.
Yes.
You said the intention was to return null if not found (rather than "the passed-in value as default"). So that bug should be fixed. Dan says he'll re-send that.
Either way, even if it's practically impossible, the get_channel_from_mode() *can* return NULL, therefore __gb_lights_flash_brightness_set() should be fixed to avoid dereferencing the return value in such a case.
-Alex
Hi Mikhail, Mikhail Lobanov m.lobanov@rosalinux.ru writes:
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.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: 2870b52bae4c ("greybus: lights: add lights implementation") Signed-off-by: Mikhail Lobanov m.lobanov@rosalinux.ru
Are you sending a new version with the changes suggested in this thread? or do you want me to prepare something with your reported-by tag?
Cheers, Rui
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.c index 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;
} #else static struct gb_channel *get_channel_from_cdev(struct led_classdev *cdev) -- 2.43.0
Hi Rui, if you can, please prepare a patch with my reported-by tag. Thanks!
Yours sincerely, Mikhail!
Hi Mikhail, Mikhail Lobanov m.lobanov@rosalinux.ru writes:
Are you sending a new version with the changes suggested in this thread? or do you want me to prepare something with your reported-by tag?
Cheers, Rui
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.c index 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;
} #else static struct gb_channel *get_channel_from_cdev(struct led_classdev *cdev) -- 2.43.0
-
Alex Elder -
Dan Carpenter -
Mikhail Lobanov -
Rui Miguel Silva -
Михаил Лобанов