Re: [PATCH v1 1/1] media: atmel-isc: Add safety checks for NULL isc->raw_fmt struct
On 21.11.2018 16:50, Ken Sloat wrote:
>>> From: Ken Sloat
>>>
>>> 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
>>> ---
>>>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
Re: [PATCH v1 1/1] media: atmel-isc: Add safety checks for NULL isc->raw_fmt struct
On 20.11.2018 22:43, Ken Sloat wrote:
> From: Ken Sloat
>
> 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
> ---
> 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 = >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 = >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, _mode, );
> isc->ctrls.hist_stat = HIST_INIT;
> } else {
> - pfe_cfg0 = rawfmt_config->pfe_cfg0_bps;
> + if
Re: MICROCHIP ISC DRIVER Bug: Possible NULL struct pointer dereference case
On 20.11.2018 01:06, Ken Sloat wrote:
> Hello,
>
> I have discovered a bug in the Atmel/Microchip ISC driver while developing a
> i2c v4l2 sub-device driver. This bug did not previously occur for me on an
> older version of the driver (more details below), but I have confirmed this
> bug on a platform based on the SAMA5D2 running a newer kernel v4.20 from the
> media tree. I am running the Atmel ISC drivers as built-in and the module I
> am developing as an LKM (though the problem will still occur if built-in as
> well). Specifically I get a kernel oops when my driver is loaded and the ISC
> driver attempts to initialize my sub-device.
>
> The bug summary is as follows:
> There is a case where a NULL pointer dereference can possibly occur in
> regards to isc->raw_fmt
Hello Ken,
Indeed this is a bug, I saw it before as well, but so far, this has not
appeared with the sensors we have connected. I have been trying to get
around to fix it, but it's not a simple fix, much rather a rework of the
driver part that handles the raw formats.
Feel free to submit patches if you find a good fix/rework and I will
have a look and test it for the sensors which I currently have.
Thanks again,
Eugen
>
> Details:
> isc->raw_fmt is NULL by default. It is referenced in functions such as
> isc_try_fmt()
>
> i.e.
> if (sensor_is_preferred(isc_fmt))
> mbus_code = isc_fmt->mbus_code;
> else
> mbus_code = isc->raw_fmt->mbus_code;
>
>
> and is only set in one place as far as I can see:
> isc_formats_init()
>
> if (fmt->flags & FMT_FLAG_RAW_FORMAT)
> isc->raw_fmt = fmt;
>
> These statements and the others are missing checks or handling for a possible
> NULL pointer, so if they are hit this will cause a kernel oops. According to
> git bisect, in my current use case, this does not occur until the following
> commit:
>
> commit f103ea11cd037943b511fa71c852ff14cc6de8ee
> Author: Wenyou Yang
> Date: Tue Oct 10 04:46:40 2017 +0200
>
> media: atmel-isc: Rework the format list
>
> To improve the readability of code, split the format array into two,
> one for the format description, other for the register configuration.
> Meanwhile, add the flag member to indicate the format can be achieved
> from the sensor or be produced by the controller, and rename members
> related to the register configuration.
>
> Also add more formats support: GREY, ARGB444, ARGB555 and ARGB32.
>
> Signed-off-by: Wenyou Yang
> Signed-off-by: Hans Verkuil
> Signed-off-by: Mauro Carvalho Chehab
>
>
> Specifically, this is caused by the introduction of new format flag
> statements which possibly prevent isc-raw_fmt from being set:
>
> while (!v4l2_subdev_call(subdev, pad, enum_mbus_code,
> NULL, _code)) {
> mbus_code.index++;
> +
> fmt = find_format_by_code(mbus_code.code, );
> - if (!fmt)
> + if ((!fmt) || (!(fmt->flags & FMT_FLAG_FROM_SENSOR)))
> continue;
>
> fmt->sd_support = true;
>
> - if (i <= RAW_FMT_IND_END) {
> - for (j = ISC_FMT_IND_START; j <= ISC_FMT_IND_END; j++)
> - isc_formats[j].isc_support = true;
> -
> + if (fmt->flags & FMT_FLAG_RAW_FORMAT)
> isc->raw_fmt = fmt;
> - }
> }
>
> I am happy to provide any more details as needed as well as submit a patch if
> I can understand a correct fix. A log of my kernel oops message follows:
>
> Unable to handle kernel NULL pointer dereference at virtual address 0004
> pgd = a9560d56
> [0004] *pgd=232d7831, *pte=, *ppte=
> Internal error: Oops: 17 [#1] ARM
> Modules linked in: tw9990(FO+)
> CPU: 0 PID: 519 Comm: insmod Tainted: GF O
> 4.20.0-rc1-00084-gd1a71a33591d-dirty #2
> Hardware name: Atmel SAMA5
> PC is at isc_try_fmt+0x20c/0x22c
> LR is at isc_try_fmt+0x38/0x22c
> pc : []lr : []psr: 200e0013
> sp : c29a3a68 ip : 0008 fp : c0d0f3b8
> r10: c0c03008 r9 : r8 :
> r7 : c0d0f010 r6 : c0c03008 r5 : c29a3b38 r4 : c0c2ce88
> r3 : 0280 r2 : r1 : 01e0 r0 : c29a3abc
> Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
> Control: 10c53c7d Table: 22864059 DAC: 0051
> Process insmod (pid: 519, stack limit = 0x5f911b4f)
> Stack: (0xc29a3a68 to 0xc29a4000)
> 3a60: c0c0a8e0 c0c03008 c29a1c1c c29a3ad4 c07590c4
> 3a80: 0c00 400e0013 83edcaea c0c03008 c29a2000 c0c0fb80 c0c03008
> 3aa0: c0c0fb60 b5af c0c0fb80 c29a3ad4 c29a3ac4 c07594a0 400e0013
> 3ac0:
> 3ae0:
> 3b00: 83edcaea c0c03008
