linux-kernel - Re: [PATCH 03/11] media: exynos4-is: Fix nullptr when no CSIS device present

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 for Android: free password hash cracker in your pocket

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

Date:   Wed, 8 Jul 2020 17:11:46 +0200
From:   Sylwester Nawrocki <s.nawrocki@...sung.com>
To:     Tomasz Figa <tfiga@...omium.org>,
        Jonathan Bakker <xc-racer2@...e.ca>
Cc:     kyungmin.park@...sung.com, mchehab@...nel.org, kgene@...nel.org,
        krzk@...nel.org, linux-media@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 03/11] media: exynos4-is: Fix nullptr when no CSIS
 device present

Hi,

On 07.07.2020 19:55, Tomasz Figa wrote:
> On Sat, Apr 25, 2020 at 07:26:42PM -0700, Jonathan Bakker wrote:
>> Not all devices use the CSIS device, some may use the FIMC directly in
>> which case the CSIS device isn't registered.  This leads to a nullptr
>> exception when starting the stream as the CSIS device is always
>> referenced.  Instead, if getting the CSIS device fails, try getting the
>> FIMC directly to check if we are using the subdev API
>>
>> Signed-off-by: Jonathan Bakker <xc-racer2@...e.ca>
>> ---
>>  drivers/media/platform/exynos4-is/media-dev.c | 17 ++++++++++++++++-
>>  1 file changed, 16 insertions(+), 1 deletion(-)

> Thank you for the patch. Please see my comments inline.

>> diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
>> index 9aaf3b8060d5..5c32abc7251b 100644
>> --- a/drivers/media/platform/exynos4-is/media-dev.c
>> +++ b/drivers/media/platform/exynos4-is/media-dev.c
>> @@ -289,11 +289,26 @@ static int __fimc_pipeline_s_stream(struct exynos_media_pipeline *ep, bool on)
>>  		{ IDX_CSIS, IDX_FLITE, IDX_FIMC, IDX_SENSOR, IDX_IS_ISP },
>>  	};
>>  	struct fimc_pipeline *p = to_fimc_pipeline(ep);
>> -	struct fimc_md *fmd = entity_to_fimc_mdev(&p->subdevs[IDX_CSIS]->entity);
>>  	enum fimc_subdev_index sd_id;
>>  	int i, ret = 0;
>>  
>>  	if (p->subdevs[IDX_SENSOR] == NULL) {
>> +		struct fimc_md *fmd;
>> +		struct v4l2_subdev *sd = p->subdevs[IDX_CSIS];
>> +
>> +		if (!sd)
>> +			sd = p->subdevs[IDX_FIMC];
>> +
>> +		if (!sd) {
>> +			/*
>> +			 * If neither CSIS nor FIMC was set up,
>> +			 * it's impossible to have any sensors
>> +			 */
>> +			return -ENODEV;
>> +		}
>> +
>> +		fmd = entity_to_fimc_mdev(&sd->entity);
>> +
> 
> Are you sure this is the correct thing to do here? In general, the media
> controller should be instantiated only if there are sensors in the system.

The code being changed is only about getting a pointer to the driver private 
data structure 'struct fimc_md', for that we need to get hold of a media 
entity that represents a subdev that is actually available in the pipeline.
In original code it is overlooked that there might camera pipelines without
the CSIS subdev. 

> What do you mean by using "the FIMC directly"? Do you mean using it only as
> an m2m image processor or with a sensor, but without the CSIS, which would
> be the case for parallel I/F sensors?

I think it is about a use case where the sensor is connected directly to the 
FIMC capture interface (parallel), without the MIPI-CSI2 receiverin between.

> Could you point me to the place where CSIS is always dereferenced? A quick
> look through the code only revealed that everywhere it seems to be guarded
> by a NULL check.

It's in this patch, above, the very first line that the patch removes.

> Another thought from looking at the implementation of
> __fimc_pipeline_s_stream() is that it probably shouldn't call s_stream on
> all the subdevices included in seq[], but only on those that are actually
> included as a part of the pipeline. It would be quite a waste of power to
> enable unnecessary hardware.

As we talked on IRC, only subdev in current active media pipeline will be
powered on/off. The p->subdevs[] array is sparsely populated and there is 
a test for NULL in __subdev_set_power(). Perhaps the test should be moved
to the caller instead (fimc_pipeline_s_power()), so we don't ignore any
ENXIO errors from the s_power v4l2_subdev_call. But is a material for 
a separate patch.

-- 
Regards,
Sylwester