MICROCHIP ISC DRIVER Bug: Possible NULL struct pointer dereference case

MICROCHIP ISC DRIVER Bug: Possible NULL struct pointer dereference case

[Ken Sloat]

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

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 <wenyou.yang@microchip.com>
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 <wenyou.yang@microchip.com>
    Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
    Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>


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, &mbus_code)) {
                mbus_code.index++;
+
                fmt = find_format_by_code(mbus_code.code, &i);
-               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 00000004
pgd = a9560d56
[00000004] *pgd=232d7831, *pte=00000000, *ppte=00000000
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 : [<c0526748>]    lr : [<c0526574>]    psr: 200e0013
sp : c29a3a68  ip : 00000008  fp : c0d0f3b8
r10: c0c03008  r9 : 00000000  r8 : 00000000
r7 : c0d0f010  r6 : c0c03008  r5 : c29a3b38  r4 : c0c2ce88
r3 : 00000280  r2 : 00000000  r1 : 000001e0  r0 : c29a3abc
Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 10c53c7d  Table: 22864059  DAC: 00000051
Process insmod (pid: 519, stack limit = 0x5f911b4f)
Stack: (0xc29a3a68 to 0xc29a4000)
3a60:                   00000000 c0c0a8e0 c0c03008 c29a1c1c c29a3ad4 c07590c4
3a80: 0c000000 00000000 400e0013 83edcaea c0c03008 c29a2000 c0c0fb80 c0c03008
3aa0: c0c0fb60 ffffb5af c0c0fb80 c29a3ad4 c29a3ac4 c07594a0 400e0013 00000000
3ac0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
3ae0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
3b00: 00000000 00000000 00000000 00000000 00000000 83edcaea c0c03008 c0d0f010
3b20: 00000008 00000001 c0d0f010 c0d0fe74 c0c03008 c05268f4 00000001 00000280
3b40: 000001e0 32315559 00000001 00000000 00000000 00000000 00000000 00000000
3b60: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
3b80: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
3ba0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
3bc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
3be0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
3c00: 00000000 83edcaea c0c2cf08 c0d0f060 00000008 c0527fe0 00000051 c0d4e040
3c20: 00000035 00000001 c29a3c24 83edcaea c0d4e040 00000000 00000002 c0d0f0b8
3c40: 00000035 00000000 00000001 00002008 00000001 00000000 00000000 00000000
3c60: 00000000 00000000 00000000 00000000 00000000 83edcaea c225aa14 c225aa14
3c80: c0c2cb18 c0d06118 c225aac0 00000000 00000009 00000000 c0c03008 c051bb40
3ca0: bf001094 c0d53400 c225aa10 c225aa14 00000005 bf00036c 00000088 c0d4e1fc
3cc0: c0d53420 bf000174 bf00201c c0d53400 00000000 c04fd490 c0c63f08 c0d53420
3ce0: 00000000 c0c63f0c bf00201c c041d778 c0d53420 bf00201c c0d53454 c0c03008
3d00: 00000000 c4cf6000 bf002200 c041dab0 00000000 c056ac08 c0d53400 c0d53420
3d20: bf00201c c0d53454 c0c03008 00000000 c4cf6000 bf002200 c0c03008 c041dc94
3d40: 00000000 bf00201c c041dbb8 c041bb64 00000000 c3b4adcc c0d519b0 83edcaea
3d60: c0c2be90 bf00201c c2998b80 c0c2be90 00000000 c041cc24 bf0012b8 bf002200
3d80: bf00201c bf00201c c0c03008 ffffe000 bf0003a8 c041e5b4 bf002000 c0c03008
3da0: ffffe000 c04fdd80 c0c41940 c0c03008 ffffe000 bf0003cc c0c41940 c01025d8
3dc0: c307ce40 00000000 00000001 00000000 00000001 83edcaea 00000000 c3b3d540
3de0: c307ce40 83edcaea a00b0013 a00b0013 c307ce40 006000c0 c3b4cc00 c4cf8fff
3e00: ffe00000 fffff000 00000000 83edcaea bf002200 00000001 c29963c0 c27e8f00
3e20: c29963e4 bf002200 c0c03008 c016de28 00000001 c29963e4 c4cf6000 c29a3f40
3e40: 00000001 c29963c0 00000001 c016cff8 bf00220c 00007fff bf002200 c016a470
3e60: bf002248 c0c03008 bf0022e8 c09d5d90 bf002368 c0801e70 c0969e44 c0969e50
3e80: c0969ea8 c0c03008 00000000 00000000 00000000 ffffe000 bf000000 00001d30
3ea0: 00000000 00000000 00000000 00000000 00000000 00000000 6e72656b 00006c65
3ec0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
3ee0: 00000000 00000000 00000000 00000000 00000000 83edcaea 7fffffff c0c03008
3f00: 00000003 00000003 00028df0 c0101204 c29a2000 0000017b 00000000 c016d744
3f20: 7fffffff 00000000 00000003 00000001 bebd7bdc c4cf6000 00001d30 00000000
3f40: c4cf662a c4cf6a00 c4cf6000 00001d30 c4cf7948 c4cf7864 c4cf7188 00003000
3f60: 000032f0 00000000 00000000 00000000 0000096c 00000017 00000018 0000000e
3f80: 00000000 00000009 00000000 83edcaea 00000000 0003c150 00000003 00026ab4
3fa0: 0000017b c0101000 0003c150 00000003 00000003 00028df0 00000003 00000000
3fc0: 0003c150 00000003 00026ab4 0000017b 00000000 00000003 b6fbcfa4 00000000
3fe0: bebd7be0 bebd7bd0 0001fe10 b6ef2130 60060010 00000003 00000000 00000000
[<c0526748>] (isc_try_fmt) from [<c05268f4>] (isc_set_default_fmt+0x6c/0xb4)
[<c05268f4>] (isc_set_default_fmt) from [<c0527fe0>] (isc_async_complete+0x260/0x53c)
[<c0527fe0>] (isc_async_complete) from [<c051bb40>] (v4l2_async_register_subdev+0x1a4/0x1b8)
[<c051bb40>] (v4l2_async_register_subdev) from [<bf00036c>] (tw9990_probe+0x1f8/0x234 [tw9990])
[<bf00036c>] (tw9990_probe [tw9990]) from [<c04fd490>] (i2c_device_probe+0x1dc/0x248)
[<c04fd490>] (i2c_device_probe) from [<c041d778>] (really_probe+0xf8/0x2cc)
[<c041d778>] (really_probe) from [<c041dab0>] (driver_probe_device+0x60/0x168)
[<c041dab0>] (driver_probe_device) from [<c041dc94>] (__driver_attach+0xdc/0xe0)
[<c041dc94>] (__driver_attach) from [<c041bb64>] (bus_for_each_dev+0x68/0xb4)
[<c041bb64>] (bus_for_each_dev) from [<c041cc24>] (bus_add_driver+0x100/0x20c)
[<c041cc24>] (bus_add_driver) from [<c041e5b4>] (driver_register+0x78/0x10c)
[<c041e5b4>] (driver_register) from [<c04fdd80>] (i2c_register_driver+0x3c/0x7c)
[<c04fdd80>] (i2c_register_driver) from [<bf0003cc>] (tw9990_init+0x24/0x2c [tw9990])
[<bf0003cc>] (tw9990_init [tw9990]) from [<c01025d8>] (do_one_initcall+0x54/0x194)
[<c01025d8>] (do_one_initcall) from [<c016de28>] (do_init_module+0x60/0x1d8)
[<c016de28>] (do_init_module) from [<c016cff8>] (load_module+0x1de8/0x22e4)
[<c016cff8>] (load_module) from [<c016d744>] (sys_finit_module+0xc8/0xd8)
[<c016d744>] (sys_finit_module) from [<c0101000>] (ret_fast_syscall+0x0/0x54)
Exception stack(0xc29a3fa8 to 0xc29a3ff0)
3fa0:                   0003c150 00000003 00000003 00028df0 00000003 00000000
3fc0: 0003c150 00000003 00026ab4 0000017b 00000000 00000003 b6fbcfa4 00000000
3fe0: bebd7be0 bebd7bd0 0001fe10 b6ef2130
Code: e5d4200e e3520000 0affffbe e59725c0 (e592a004)
---[ end trace fdc3db2f91ac07c6 ]---

Thanks,
Ken Sloat

Re: MICROCHIP ISC DRIVER Bug: Possible NULL struct pointer dereference case

[Eugen.Hristev]

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 <wenyou.yang@microchip.com>
> 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 <wenyou.yang@microchip.com>
>      Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>      Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> 
> 
> 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, &mbus_code)) {
>                  mbus_code.index++;
> +
>                  fmt = find_format_by_code(mbus_code.code, &i);
> -               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 00000004
> pgd = a9560d56
> [00000004] *pgd=232d7831, *pte=00000000, *ppte=00000000
> 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 : [<c0526748>]    lr : [<c0526574>]    psr: 200e0013
> sp : c29a3a68  ip : 00000008  fp : c0d0f3b8
> r10: c0c03008  r9 : 00000000  r8 : 00000000
> r7 : c0d0f010  r6 : c0c03008  r5 : c29a3b38  r4 : c0c2ce88
> r3 : 00000280  r2 : 00000000  r1 : 000001e0  r0 : c29a3abc
> Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> Control: 10c53c7d  Table: 22864059  DAC: 00000051
> Process insmod (pid: 519, stack limit = 0x5f911b4f)
> Stack: (0xc29a3a68 to 0xc29a4000)
> 3a60:                   00000000 c0c0a8e0 c0c03008 c29a1c1c c29a3ad4 c07590c4
> 3a80: 0c000000 00000000 400e0013 83edcaea c0c03008 c29a2000 c0c0fb80 c0c03008
> 3aa0: c0c0fb60 ffffb5af c0c0fb80 c29a3ad4 c29a3ac4 c07594a0 400e0013 00000000
> 3ac0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 3ae0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 3b00: 00000000 00000000 00000000 00000000 00000000 83edcaea c0c03008 c0d0f010
> 3b20: 00000008 00000001 c0d0f010 c0d0fe74 c0c03008 c05268f4 00000001 00000280
> 3b40: 000001e0 32315559 00000001 00000000 00000000 00000000 00000000 00000000
> 3b60: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 3b80: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 3ba0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 3bc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 3be0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 3c00: 00000000 83edcaea c0c2cf08 c0d0f060 00000008 c0527fe0 00000051 c0d4e040
> 3c20: 00000035 00000001 c29a3c24 83edcaea c0d4e040 00000000 00000002 c0d0f0b8
> 3c40: 00000035 00000000 00000001 00002008 00000001 00000000 00000000 00000000
> 3c60: 00000000 00000000 00000000 00000000 00000000 83edcaea c225aa14 c225aa14
> 3c80: c0c2cb18 c0d06118 c225aac0 00000000 00000009 00000000 c0c03008 c051bb40
> 3ca0: bf001094 c0d53400 c225aa10 c225aa14 00000005 bf00036c 00000088 c0d4e1fc
> 3cc0: c0d53420 bf000174 bf00201c c0d53400 00000000 c04fd490 c0c63f08 c0d53420
> 3ce0: 00000000 c0c63f0c bf00201c c041d778 c0d53420 bf00201c c0d53454 c0c03008
> 3d00: 00000000 c4cf6000 bf002200 c041dab0 00000000 c056ac08 c0d53400 c0d53420
> 3d20: bf00201c c0d53454 c0c03008 00000000 c4cf6000 bf002200 c0c03008 c041dc94
> 3d40: 00000000 bf00201c c041dbb8 c041bb64 00000000 c3b4adcc c0d519b0 83edcaea
> 3d60: c0c2be90 bf00201c c2998b80 c0c2be90 00000000 c041cc24 bf0012b8 bf002200
> 3d80: bf00201c bf00201c c0c03008 ffffe000 bf0003a8 c041e5b4 bf002000 c0c03008
> 3da0: ffffe000 c04fdd80 c0c41940 c0c03008 ffffe000 bf0003cc c0c41940 c01025d8
> 3dc0: c307ce40 00000000 00000001 00000000 00000001 83edcaea 00000000 c3b3d540
> 3de0: c307ce40 83edcaea a00b0013 a00b0013 c307ce40 006000c0 c3b4cc00 c4cf8fff
> 3e00: ffe00000 fffff000 00000000 83edcaea bf002200 00000001 c29963c0 c27e8f00
> 3e20: c29963e4 bf002200 c0c03008 c016de28 00000001 c29963e4 c4cf6000 c29a3f40
> 3e40: 00000001 c29963c0 00000001 c016cff8 bf00220c 00007fff bf002200 c016a470
> 3e60: bf002248 c0c03008 bf0022e8 c09d5d90 bf002368 c0801e70 c0969e44 c0969e50
> 3e80: c0969ea8 c0c03008 00000000 00000000 00000000 ffffe000 bf000000 00001d30
> 3ea0: 00000000 00000000 00000000 00000000 00000000 00000000 6e72656b 00006c65
> 3ec0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 3ee0: 00000000 00000000 00000000 00000000 00000000 83edcaea 7fffffff c0c03008
> 3f00: 00000003 00000003 00028df0 c0101204 c29a2000 0000017b 00000000 c016d744
> 3f20: 7fffffff 00000000 00000003 00000001 bebd7bdc c4cf6000 00001d30 00000000
> 3f40: c4cf662a c4cf6a00 c4cf6000 00001d30 c4cf7948 c4cf7864 c4cf7188 00003000
> 3f60: 000032f0 00000000 00000000 00000000 0000096c 00000017 00000018 0000000e
> 3f80: 00000000 00000009 00000000 83edcaea 00000000 0003c150 00000003 00026ab4
> 3fa0: 0000017b c0101000 0003c150 00000003 00000003 00028df0 00000003 00000000
> 3fc0: 0003c150 00000003 00026ab4 0000017b 00000000 00000003 b6fbcfa4 00000000
> 3fe0: bebd7be0 bebd7bd0 0001fe10 b6ef2130 60060010 00000003 00000000 00000000
> [<c0526748>] (isc_try_fmt) from [<c05268f4>] (isc_set_default_fmt+0x6c/0xb4)
> [<c05268f4>] (isc_set_default_fmt) from [<c0527fe0>] (isc_async_complete+0x260/0x53c)
> [<c0527fe0>] (isc_async_complete) from [<c051bb40>] (v4l2_async_register_subdev+0x1a4/0x1b8)
> [<c051bb40>] (v4l2_async_register_subdev) from [<bf00036c>] (tw9990_probe+0x1f8/0x234 [tw9990])
> [<bf00036c>] (tw9990_probe [tw9990]) from [<c04fd490>] (i2c_device_probe+0x1dc/0x248)
> [<c04fd490>] (i2c_device_probe) from [<c041d778>] (really_probe+0xf8/0x2cc)
> [<c041d778>] (really_probe) from [<c041dab0>] (driver_probe_device+0x60/0x168)
> [<c041dab0>] (driver_probe_device) from [<c041dc94>] (__driver_attach+0xdc/0xe0)
> [<c041dc94>] (__driver_attach) from [<c041bb64>] (bus_for_each_dev+0x68/0xb4)
> [<c041bb64>] (bus_for_each_dev) from [<c041cc24>] (bus_add_driver+0x100/0x20c)
> [<c041cc24>] (bus_add_driver) from [<c041e5b4>] (driver_register+0x78/0x10c)
> [<c041e5b4>] (driver_register) from [<c04fdd80>] (i2c_register_driver+0x3c/0x7c)
> [<c04fdd80>] (i2c_register_driver) from [<bf0003cc>] (tw9990_init+0x24/0x2c [tw9990])
> [<bf0003cc>] (tw9990_init [tw9990]) from [<c01025d8>] (do_one_initcall+0x54/0x194)
> [<c01025d8>] (do_one_initcall) from [<c016de28>] (do_init_module+0x60/0x1d8)
> [<c016de28>] (do_init_module) from [<c016cff8>] (load_module+0x1de8/0x22e4)
> [<c016cff8>] (load_module) from [<c016d744>] (sys_finit_module+0xc8/0xd8)
> [<c016d744>] (sys_finit_module) from [<c0101000>] (ret_fast_syscall+0x0/0x54)
> Exception stack(0xc29a3fa8 to 0xc29a3ff0)
> 3fa0:                   0003c150 00000003 00000003 00028df0 00000003 00000000
> 3fc0: 0003c150 00000003 00026ab4 0000017b 00000000 00000003 b6fbcfa4 00000000
> 3fe0: bebd7be0 bebd7bd0 0001fe10 b6ef2130
> Code: e5d4200e e3520000 0affffbe e59725c0 (e592a004)
> ---[ end trace fdc3db2f91ac07c6 ]---
> 
> Thanks,
> Ken Sloat
> 
> 
> 
> 

RE: MICROCHIP ISC DRIVER Bug: Possible NULL struct pointer dereference case

[Ken Sloat]

> 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

Hi Eugen,

Thanks for your quick reply. I will see what I can come up with. I may reply with additional questions.

Thanks,
Ken Sloat

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

[Ken Sloat]

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.

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;
+	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;
+	}
 
 	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;
+	}
 
 	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);
 
-- 
2.17.1

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

[Eugen.Hristev]

On 20.11.2018 22:43, 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 ?

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@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 ?


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

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

[Ken Sloat]

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

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

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

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

> 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,
},

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.

Thanks,
Ken

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

[Eugen.Hristev]

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
> 

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

[Ken Sloat]

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

I think this flag is referring specifically to RAW bayer formats, so that the ISC can set registers properly for other modules in the pipeline in those cases - see comments in code for this flag:

/* Indicate a Raw Bayer format */
#define FMT_FLAG_RAW_FORMAT		BIT(2)

> >> 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 ?
Doing this will indeed prevent a crash and allow the sensor to function in my use case.

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

Sounds good Eugen, thanks for your help. Let me know how I can be of assistance going forward.

Thanks,
Ken