Received-SPF: pass (google.com: domain of linux-kernel+bounces-89508-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: Rui Miguel Silva <rmfrfs@gmail.com>
To: Alex Elder <elder@ieee.org>, Mikhail Lobanov <m.lobanov@rosalinux.ru>
Cc: greybus-dev@lists.linaro.org, linux-staging@lists.linux.dev,
 linux-kernel@vger.kernel.org
Subject: Re: [greybus-dev] [PATCH] greybus: Fix deref of NULL in
 __gb_lights_flash_brightness_set
In-Reply-To: <07df4b96-70c2-41de-9d76-1deb80447a79@ieee.org>
References: <20240301190425.120605-1-m.lobanov@rosalinux.ru>
 <07df4b96-70c2-41de-9d76-1deb80447a79@ieee.org>
Date: Sat, 02 Mar 2024 16:35:44 +0000
Message-ID: <m3o7bwlhgf.fsf@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

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
>
> 					-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)