Re: [PATCH v1 1/1] media: atmel-isc: Add safety checks for NULL isc->raw_fmt struct

[<Eugen.Hristev@microchip.com>]

On 21.11.2018 16:50, Ken Sloat wrote:
>>> From: Ken Sloat <ksloat@aampglobal.com>
>>>
>>> In some usages isc->raw_fmt will not be initialized. If this is the
>>> case, it is very possible that a NULL struct de-reference will occur,
>>> as this member is referenced many times.
> 
>> Hello  Ken,
> 
>> Do you have any confidence that just by avoiding the NULL situation, this fix makes things right for adding new sensors that for example, do not offer a raw format ?
> 
> Hi Eugen,
> 
> Thanks for your comments. The primary goal of my patch is to the solve the immediate issue of NULL de-reference of the that struct member. My current sensors actually do not offer a RAW format, which is why this bug happens in my case (see more details below).

I am not sure if I am correct, but your sensor surely provides data in 
some format. This format might be the 'raw' one that ISC receives. So, 
adjustments to the format list might solve this.

> 
>> My feeling is that the method of adding this variable (raw_fmt) is very unfortunate, and I did not completely understand the situations where it's needed.
> 
> I agree that the current method of setting a struct member based on a RAW flag is flawed and ideally there needs to be a more fundamental change to the architecture of the driver so that this situation would never possibly occur, however I will present one below that can very likely happen as it does for me:
> 
>> The check that actually sets the raw_fmt comes from an iteration through the formats, and the one having the RAW flag gets put into this variable. One could just alter the formats table and get the raw_fmt that is needed.
> 
> Right, so in the initial iteration in isc_formats_init() the driver calls the sub-device/sensor enum_mbus_code function to step through all its supported formats and try and find them in the list of supported ISC formats. If none of the formats in the sub-device/sensor are of RAW type, then isc-raw_fmt will not be set. This is the fundamental flaw in using this member.
> 
> Following this, the driver will attempt to set a default format for the ISC in isc_set_default_fmt(). This appears to be based on the first format in the list of ISC formats. The driver then does a check to see if the sensor is preferred to the ISC. If the default format is not supported by the sub-device/sensor, it will not be preferred and we will get a resulting crash because it will assume that we must use the raw_fmt member that never got set.

I saw this part of the code as well. I was thinking to rewrite it to 
have it iterate through all formats until a suitable one is found 
(instead of just taking the first and win or fail directly).

> 
>>>
>>> To prevent this, add safety checks for this member and handle
>>> situations accordingly.
>>>
>>> Signed-off-by: Ken Sloat <ksloat@aampglobal.com>
>>> ---
>>>    drivers/media/platform/atmel/atmel-isc.c | 64 ++++++++++++++++--------
>>>    1 file changed, 44 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/atmel/atmel-isc.c
>>> b/drivers/media/platform/atmel/atmel-isc.c
>>> index 50178968b8a6..4cccaa4f2ce9 100644
>>> --- a/drivers/media/platform/atmel/atmel-isc.c
>>> +++ b/drivers/media/platform/atmel/atmel-isc.c
>>> @@ -902,6 +902,15 @@ static inline bool sensor_is_preferred(const struct isc_format *isc_fmt)
>>>    		!isc_fmt->isc_support;
>>>    }
>>>    
>>> +static inline u32 get_preferred_mbus_code(const struct isc_device *isc,
>>> +		const struct isc_format *isc_fmt)
>>> +{
>>> +	if (sensor_is_preferred(isc_fmt) || !isc->raw_fmt)
>>> +		return isc_fmt->mbus_code;
> 
>> For example here, if we do _not_ have a raw format, what makes us believe that the right format is the one from the mbus_code from the isc_fmt ? Is there anything useful there at all ?
> 
> It's more of a safe case for where this occurs in my example above. As you mentioned yourself, raw_fmt could possibly set to any of the RAW flag formats supported by the sub-device.  Assuming the sub-device did indeed support a RAW format of some sort, but did not necessarily support the current format, the driver as of today would be referencing this alternative mbus code anyways. In the example above, this occurred while setting the default format, and then subsequently will always occur when setting the pipeline in isc_set_pipeline() as this function always de-references this member to set the pointer even if a RAW format isn't necessarily being used (and so do others as seen in my patch).

I tend to disagree, the driver of today will fail if the sensor does not 
provide a RAW format of some sort, so it will not use alternative mbus code.

> 
>>> +	else
>>> +		return isc->raw_fmt->mbus_code;
>>> +}
>>> +
>>>    static struct fmt_config *get_fmt_config(u32 fourcc)
>>>    {
>>>    	struct fmt_config *config;
>>> @@ -955,7 +964,7 @@ static void isc_set_pipeline(struct isc_device *isc, u32 pipeline)
>>>    {
>>>    	struct regmap *regmap = isc->regmap;
>>>    	struct isc_ctrls *ctrls = &isc->ctrls;
>>> -	struct fmt_config *config = get_fmt_config(isc->raw_fmt->fourcc);
>>> +	struct fmt_config *config;
>>>    	u32 val, bay_cfg;
>>>    	const u32 *gamma;
>>>    	unsigned int i;
>>> @@ -969,7 +978,12 @@ static void isc_set_pipeline(struct isc_device *isc, u32 pipeline)
>>>    	if (!pipeline)
>>>    		return;
>>>    
>>> -	bay_cfg = config->cfa_baycfg;
>>> +	if (isc->raw_fmt) {
>>> +		config = get_fmt_config(isc->raw_fmt->fourcc);
>>> +		bay_cfg = config->cfa_baycfg;
>>> +	} else {
>>> +		bay_cfg = 0;
>>> +	}
> 
>> Having bay_cfg zero, in the case when we do not have a raw format, is the real proper way to do this ? it is possible that this bay cfg is required at a different value, or corresponding to different formats in the pipeline of the ISC.
> I should probably make config point to the current_fmt in the else case here so that it uses its bay_cfg, however I believe the WB module would be disabled anyways in this case. Regarding if this would be proper or useful, similar comments to above.

I am not sure that the current_fmt is the actual format that the sensor 
is set to use, and if this format is OK, however you may be right that 
the bay_cfg is not needed

> 
>> So , in short, I am not convinced that this is a proper way to solve it, so we have to dig in further to see if this is OK or not.
>> Which sensors do you have and how did you test this, which board and setup?
> 
>> Thanks for your help,
> 
>> Eugen
> 
> My sensor inputs a ITU-R 656 interface to the ISC, so this would be the format:
> 
> {
> 	.fourcc		= V4L2_PIX_FMT_YUYV,
> 	.mbus_code	= MEDIA_BUS_FMT_YUYV8_2X8,
> 	.flags		= FMT_FLAG_FROM_CONTROLLER |
> 			  FMT_FLAG_FROM_SENSOR,
> 	.bpp		= 16,
> },

If this format is added with a RAW flag, how does this affect your output ?

> 
> Note that the driver as of today does not support ITU-R 656 without modifications but rather ITU-R 601. However, this is as simple as setting some additional bits and I plan to submit a separate patch soon that allows this to occur from device tree in a standard way.
> I am using a custom board that is based on the SAMA5D27-SOM1-EK1 board so I tested my sensor with this board using gstreamer to direct the image to the display. Happy to help out as I am able, let me know what you think.
Which sensor is it?
You managed to get it working OK with just this patch?

My general feeling is that this workaround patch will hide the problem, 
and not solve the issue we are having here, we add more code to cope 
around with this raw_fmt NULL issue.

Anyway I am thinking to get more opinions on this issue, about which is 
the best way we can go further with it.

Thanks,
Eugen

> 
> Thanks,
> Ken
>