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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 20.11.2018 22:43, Ken Sloat wrote:
> From: Ken Sloat <ksloat@xxxxxxxxxxxxxx>
> 
> 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 ?

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

Look inline about what I mean...

> 
> To prevent this, add safety checks for this member and handle
> situations accordingly.
> 
> Signed-off-by: Ken Sloat <ksloat@xxxxxxxxxxxxxx>
> ---
>   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 ?


> +	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.

>   
>   	regmap_write(regmap, ISC_WB_CFG, bay_cfg);
>   	regmap_write(regmap, ISC_WB_O_RGR, 0x0);
> @@ -1022,12 +1036,20 @@ static void isc_set_histogram(struct isc_device *isc)
>   {
>   	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	cfa_baycfg;
> +
> +	if (isc->raw_fmt) {
> +		config = get_fmt_config(isc->raw_fmt->fourcc);
> +		cfa_baycfg = config->cfa_baycfg << ISC_HIS_CFG_BAYSEL_SHIFT;
> +	} else {
> +		cfa_baycfg = 0;
> +	}

Ditto

>   
>   	if (ctrls->awb && (ctrls->hist_stat != HIST_ENABLED)) {
>   		regmap_write(regmap, ISC_HIS_CFG,
>   			     ISC_HIS_CFG_MODE_R |
> -			     (config->cfa_baycfg << ISC_HIS_CFG_BAYSEL_SHIFT) |
> +				 cfa_baycfg |
>   			     ISC_HIS_CFG_RAR);
>   		regmap_write(regmap, ISC_HIS_CTRL, ISC_HIS_CTRL_EN);
>   		regmap_write(regmap, ISC_INTEN, ISC_INT_HISDONE);
> @@ -1075,7 +1097,7 @@ static int isc_configure(struct isc_device *isc)
>   	struct regmap *regmap = isc->regmap;
>   	const struct isc_format *current_fmt = isc->current_fmt;
>   	struct fmt_config *curfmt_config = get_fmt_config(current_fmt->fourcc);
> -	struct fmt_config *rawfmt_config = get_fmt_config(isc->raw_fmt->fourcc);
> +	struct fmt_config *rawfmt_config;
>   	struct isc_subdev_entity *subdev = isc->current_subdev;
>   	u32 pfe_cfg0, rlp_mode, dcfg, mask, pipeline;
>   
> @@ -1085,7 +1107,12 @@ static int isc_configure(struct isc_device *isc)
>   		isc_get_param(current_fmt, &rlp_mode, &dcfg);
>   		isc->ctrls.hist_stat = HIST_INIT;
>   	} else {
> -		pfe_cfg0 = rawfmt_config->pfe_cfg0_bps;
> +		if (isc->raw_fmt) {
> +			rawfmt_config = get_fmt_config(isc->raw_fmt->fourcc);
> +			pfe_cfg0 = rawfmt_config->pfe_cfg0_bps;
> +		} else {
> +			pfe_cfg0 = curfmt_config->pfe_cfg0_bps;
> +		}
>   		pipeline = curfmt_config->bits_pipeline;
>   		rlp_mode = curfmt_config->rlp_cfg_mode;
>   		dcfg = curfmt_config->dcfg_imode |
> @@ -1315,10 +1342,7 @@ static int isc_try_fmt(struct isc_device *isc, struct v4l2_format *f,
>   	if (pixfmt->height > ISC_MAX_SUPPORT_HEIGHT)
>   		pixfmt->height = ISC_MAX_SUPPORT_HEIGHT;
>   
> -	if (sensor_is_preferred(isc_fmt))
> -		mbus_code = isc_fmt->mbus_code;
> -	else
> -		mbus_code = isc->raw_fmt->mbus_code;
> +	mbus_code = get_preferred_mbus_code(isc, isc_fmt);
>   
>   	v4l2_fill_mbus_format(&format.format, pixfmt, mbus_code);
>   	ret = v4l2_subdev_call(isc->current_subdev->sd, pad, set_fmt,
> @@ -1442,10 +1466,7 @@ static int isc_enum_framesizes(struct file *file, void *fh,
>   	if (!isc_fmt)
>   		return -EINVAL;
>   
> -	if (sensor_is_preferred(isc_fmt))
> -		fse.code = isc_fmt->mbus_code;
> -	else
> -		fse.code = isc->raw_fmt->mbus_code;
> +	fse.code = get_preferred_mbus_code(isc, isc_fmt);
>   
>   	ret = v4l2_subdev_call(isc->current_subdev->sd, pad, enum_frame_size,
>   			       NULL, &fse);
> @@ -1476,10 +1497,7 @@ static int isc_enum_frameintervals(struct file *file, void *fh,
>   	if (!isc_fmt)
>   		return -EINVAL;
>   
> -	if (sensor_is_preferred(isc_fmt))
> -		fie.code = isc_fmt->mbus_code;
> -	else
> -		fie.code = isc->raw_fmt->mbus_code;
> +	fie.code = get_preferred_mbus_code(isc, isc_fmt);
>   
>   	ret = v4l2_subdev_call(isc->current_subdev->sd, pad,
>   			       enum_frame_interval, NULL, &fie);
> @@ -1668,7 +1686,7 @@ static void isc_awb_work(struct work_struct *w)
>   	struct isc_device *isc =
>   		container_of(w, struct isc_device, awb_work);
>   	struct regmap *regmap = isc->regmap;
> -	struct fmt_config *config = get_fmt_config(isc->raw_fmt->fourcc);
> +	struct fmt_config *config;
>   	struct isc_ctrls *ctrls = &isc->ctrls;
>   	u32 hist_id = ctrls->hist_id;
>   	u32 baysel;
> @@ -1686,7 +1704,13 @@ static void isc_awb_work(struct work_struct *w)
>   	}
>   
>   	ctrls->hist_id = hist_id;
> -	baysel = config->cfa_baycfg << ISC_HIS_CFG_BAYSEL_SHIFT;
> +
> +	if (isc->raw_fmt) {
> +		config = get_fmt_config(isc->raw_fmt->fourcc);
> +		baysel = config->cfa_baycfg << ISC_HIS_CFG_BAYSEL_SHIFT;
> +	} else {
> +		baysel = 0;
> +	}
>   
>   	pm_runtime_get_sync(isc->dev);
>   
> 

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




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux