Re: [PATCH] media: mtk-mdp: Fix Null pointer dereference when calling list_add

From: Enric Balletbo i Serra
Date: Fri Sep 04 2020 - 03:59:53 EST


Hi Dafna,

Thank you to work on this fix.

On 28/8/20 15:55, Dafna Hirschfeld wrote:
> In list_add, the first variable is the new node and the second
> is the list head. The function is called with a wrong order causing
> NULL dereference:
>
> [ 15.527030] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000008
> [ 15.542317] Mem abort info:
> [ 15.545152] ESR = 0x96000044
> [ 15.548248] EC = 0x25: DABT (current EL), IL = 32 bits
> [ 15.553624] SET = 0, FnV = 0
> [ 15.556715] EA = 0, S1PTW = 0
> [ 15.559892] Data abort info:
> [ 15.562799] ISV = 0, ISS = 0x00000044
> [ 15.566678] CM = 0, WnR = 1
> [ 15.569683] user pgtable: 4k pages, 48-bit VAs, pgdp=00000001373f0000
> [ 15.576196] [0000000000000008] pgd=0000000000000000, p4d=0000000000000000
> [ 15.583101] Internal error: Oops: 96000044 [#1] PREEMPT SMP
> [ 15.588747] Modules linked in: mtk_mdp(+) cfg80211 v4l2_mem2mem videobuf2_vmalloc videobuf2_dma_contig videobuf2_memops videobuf2_v4l2 videobuf2_common vide
> odev mt8173_rt5650 smsc95xx usbnet ecdh_generic ecc snd_soc_rt5645 mc mt8173_afe_pcm rfkill cros_ec_sensors snd_soc_mtk_common elan_i2c crct10dif_ce cros_ec_se
> nsors_core snd_soc_rl6231 elants_i2c industrialio_triggered_buffer kfifo_buf mtk_vpu cros_ec_chardev cros_usbpd_charger cros_usbpd_logger sbs_battery display_c
> onnector pwm_bl ip_tables x_tables ipv6
> [ 15.634295] CPU: 0 PID: 188 Comm: systemd-udevd Not tainted 5.9.0-rc2+ #69
> [ 15.641242] Hardware name: Google Elm (DT)
> [ 15.645381] pstate: 20000005 (nzCv daif -PAN -UAO BTYPE=--)
> [ 15.651022] pc : mtk_mdp_probe+0x134/0x3a8 [mtk_mdp]
> [ 15.656041] lr : mtk_mdp_probe+0x128/0x3a8 [mtk_mdp]
> [ 15.661055] sp : ffff80001255b910
> [ 15.669548] x29: ffff80001255b910 x28: 0000000000000000
> [ 15.679973] x27: ffff800009089bf8 x26: ffff0000fafde800
> [ 15.690347] x25: ffff0000ff7d2768 x24: ffff800009089010
> [ 15.700670] x23: ffff0000f01a7cd8 x22: ffff0000fafde810
> [ 15.710940] x21: ffff0000f01a7c80 x20: ffff0000f0c3c180
> [ 15.721148] x19: ffff0000ff7f1618 x18: 0000000000000010
> [ 15.731289] x17: 0000000000000000 x16: 0000000000000000
> [ 15.741375] x15: 0000000000aaaaaa x14: 0000000000000020
> [ 15.751399] x13: 00000000ffffffff x12: 0000000000000020
> [ 15.761363] x11: 0000000000000028 x10: 0101010101010101
> [ 15.771279] x9 : 0000000000000004 x8 : 7f7f7f7f7f7f7f7f
> [ 15.781148] x7 : 646bff6171606b2b x6 : 0000000000806d65
> [ 15.790981] x5 : ffff0000ff7f8360 x4 : 0000000000000000
> [ 15.800767] x3 : 0000000000000004 x2 : 0000000000000001
> [ 15.810501] x1 : 0000000000000005 x0 : 0000000000000000
> [ 15.820171] Call trace:
> [ 15.826944] mtk_mdp_probe+0x134/0x3a8 [mtk_mdp]
> [ 15.835908] platform_drv_probe+0x54/0xa8
> [ 15.844247] really_probe+0xe4/0x3b0
> [ 15.852104] driver_probe_device+0x58/0xb8
> [ 15.860457] device_driver_attach+0x74/0x80
> [ 15.868854] __driver_attach+0x58/0xe0
> [ 15.876770] bus_for_each_dev+0x70/0xc0
> [ 15.884726] driver_attach+0x24/0x30
> [ 15.892374] bus_add_driver+0x14c/0x1f0
> [ 15.900295] driver_register+0x64/0x120
> [ 15.908168] __platform_driver_register+0x48/0x58
> [ 15.916864] mtk_mdp_driver_init+0x20/0x1000 [mtk_mdp]
> [ 15.925943] do_one_initcall+0x54/0x1b4
> [ 15.933662] do_init_module+0x54/0x200
> [ 15.941246] load_module+0x1cf8/0x22d0
> [ 15.948798] __do_sys_finit_module+0xd8/0xf0
> [ 15.956829] __arm64_sys_finit_module+0x20/0x30
> [ 15.965082] el0_svc_common.constprop.0+0x6c/0x168
> [ 15.973527] do_el0_svc+0x24/0x90
> [ 15.980403] el0_sync_handler+0x90/0x198
> [ 15.987867] el0_sync+0x158/0x180
> [ 15.994653] Code: 9400014b 2a0003fc 35000920 f9400280 (f9000417)
> [ 16.004299] ---[ end trace 76fee0203f9898e5 ]---
>
> Fixes: 86698b9505bbc ("media: mtk-mdp: convert mtk_mdp_dev.comp array to list")
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@xxxxxxxxxxxxx>
> ---
> drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> index f96c8b3bf861..976aa1f4829b 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> @@ -94,7 +94,7 @@ static void mtk_mdp_reset_handler(void *priv)
> void mtk_mdp_register_component(struct mtk_mdp_dev *mdp,
> struct mtk_mdp_comp *comp)
> {
> - list_add(&mdp->comp_list, &comp->node);
> + list_add(&comp->node, &mdp->comp_list);
> }
>
> void mtk_mdp_unregister_component(struct mtk_mdp_dev *mdp,
>

FWIW this also fixes my problems with suspend/resume on my Acer Chromebook R13, so:

Tested-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>