linux-kernel - Re: [greybus-dev] [PATCH] greybus: Fix deref of NULL in __gb_lights_flash_brightness

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e435724c-33ac-446a-81ed-fc31b56d6206@ieee.org>
Date: Mon, 4 Mar 2024 07:30:01 -0600
From: Alex Elder <elder@...e.org>
To: Mikhail Lobanov <m.lobanov@...alinux.ru>,
 Rui Miguel Silva <rmfrfs@...il.com>
Cc: greybus-dev@...ts.linaro.org, linux-staging@...ts.linux.dev,
 linux-kernel@...r.kernel.org
Subject: Re: [greybus-dev] [PATCH] greybus: Fix deref of NULL in
 __gb_lights_flash_brightness_set

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@...alinux.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)
>