Re: [PATCH] firmware: imx: Align imx SC msg structs to 4

[Leonard Crestez <leonard.crestez@nxp.com>]

On 20.02.2020 01:57, Stephen Boyd wrote:
> Quoting Leonard Crestez (2020-02-11 13:24:33)
>> The imx SC api strongly assumes that messages are composed out of
>> 4-bytes words but some of our message structs have sizeof "6" and "7".
>>
>> This produces many oopses with CONFIG_KASAN=y:
>>
>>          BUG: KASAN: stack-out-of-bounds in imx_mu_send_data+0x108/0x1f0
> 
> Can you share the full kasan bug report instead of the single line?

[    1.606708] imx-scu scu: NXP i.MX SCU Initialized
[    1.635265] random: fast init done
[    1.652200] 
==================================================================
[    1.659118] BUG: KASAN: stack-out-of-bounds in 
imx_mu_send_data+0x108/0x1f0
[    1.666046] Read of size 4 at addr ffff0008c80e6bc4 by task swapper/0/1
[    1.672642]
[    1.674134] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 
5.4.3-03848-g13efcd6 #54
[    1.681335] Hardware name: Freescale i.MX8QM MEK (DT)
[    1.686373] Call trace:
[    1.688815]  dump_backtrace+0x0/0x1e8
[    1.692458]  show_stack+0x14/0x20
[    1.695766]  dump_stack+0xe8/0x140
[    1.699155]  print_address_description.isra.11+0x64/0x348
[    1.704532]  __kasan_report+0x11c/0x230
[    1.708356]  kasan_report+0xc/0x18
[    1.711743]  __asan_load4+0x90/0xb0
[    1.715218]  imx_mu_send_data+0x108/0x1f0
[    1.719215]  msg_submit+0x104/0x180
[    1.722689]  mbox_send_message+0xa8/0x1a0
[    1.726696]  imx_scu_call_rpc+0x168/0x310
[    1.730679]  imx_sc_pd_power+0x180/0x1e0
[    1.734589]  imx_sc_pd_power_on+0x10/0x18
[    1.738598]  genpd_power_on.part.23+0x118/0x2a8
[    1.743105]  genpd_runtime_resume+0x138/0x320
[    1.747454]  __rpm_callback+0xb0/0x1a0
[    1.751184]  rpm_callback+0x34/0xe0
[    1.754659]  rpm_resume+0x5b8/0x7e8
[    1.758137]  __pm_runtime_resume+0x38/0x90
[    1.762222]  imx_clk_scu_probe+0x5c/0x1c8
[    1.766218]  platform_drv_probe+0x6c/0xc8
[    1.770217]  really_probe+0x148/0x428
[    1.773861]  driver_probe_device+0x74/0x130
[    1.778031]  __device_attach_driver+0xc4/0xe8
[    1.782380]  bus_for_each_drv+0xf0/0x158
[    1.786283]  __device_attach+0x158/0x1d8
[    1.790195]  device_initial_probe+0x10/0x18
[    1.794362]  bus_probe_device+0xe0/0xf0
[    1.798185]  device_add+0x660/0x998
[    1.801659]  platform_device_add+0x198/0x340
[    1.805916]  imx_clk_scu_alloc_dev+0x1b8/0x1e8
[    1.810347]  imx8qxp_clk_probe+0x19d0/0x28b8
[    1.814601]  platform_drv_probe+0x6c/0xc8
[    1.818601]  really_probe+0x148/0x428
[    1.822250]  driver_probe_device+0x74/0x130
[    1.826423]  __device_attach_driver+0xc4/0xe8
[    1.830763]  bus_for_each_drv+0xf0/0x158
[    1.834675]  __device_attach+0x158/0x1d8
[    1.838583]  device_initial_probe+0x10/0x18
[    1.842752]  bus_probe_device+0xe0/0xf0
[    1.846572]  device_add+0x660/0x998
[    1.850058]  of_device_add+0x74/0x98
[    1.853610]  of_platform_device_create_pdata+0x11c/0x178
[    1.858908]  of_platform_bus_create+0x404/0x4f0
[    1.863425]  of_platform_populate+0x74/0x110
[    1.867688]  devm_of_platform_populate+0x54/0xb8
[    1.872291]  imx_scu_probe+0x1b8/0x220
[    1.876022]  platform_drv_probe+0x6c/0xc8
[    1.880021]  really_probe+0x148/0x428
[    1.883671]  driver_probe_device+0x74/0x130
[    1.887841]  device_driver_attach+0x94/0xa0
[    1.892010]  __driver_attach+0x70/0x110
[    1.895832]  bus_for_each_dev+0xe8/0x158
[    1.899741]  driver_attach+0x30/0x40
[    1.903303]  bus_add_driver+0x1b0/0x2b8
[    1.907129]  driver_register+0xbc/0x1d0
[    1.910947]  __platform_driver_register+0x7c/0x88
[    1.915653]  imx_scu_driver_init+0x18/0x20
[    1.919725]  do_one_initcall+0xd4/0x244
[    1.923552]  kernel_init_freeable+0x238/0x2d4
[    1.927889]  kernel_init+0x10/0x114
[    1.931365]  ret_from_fork+0x10/0x18
[    1.934917]
[    1.936399] The buggy address belongs to the page:
[    1.941184] page:fffffe0023003980 refcount:0 mapcount:0 
mapping:0000000000000000 index:0x0
[    1.949447] raw: 1ffff00000000000 fffffe0023003988 fffffe0023003988 
0000000000000000
[    1.957170] raw: 0000000000000000 0000000000000000 00000000ffffffff 
0000000000000000
[    1.964894] page dumped because: kasan: bad access detected
[    1.970449]
[    1.971933] addr ffff0008c80e6bc4 is located in stack of task 
swapper/0/1 at offset 36 in frame:
[    1.980708]  imx_sc_pd_power+0x0/0x1e0
[    1.984442]
[    1.985917] this frame has 1 object:
[    1.989481]  [32, 39) 'msg'
[    1.989484]
[    1.993732] Memory state around the buggy address:
[    1.998520]  ffff0008c80e6a80: 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 
f1 00 00
[    2.005730]  ffff0008c80e6b00: 00 00 f3 f3 f3 f3 00 00 00 00 00 00 00 
00 00 00
[    2.012940] >ffff0008c80e6b80: 00 00 00 00 f1 f1 f1 f1 07 f2 f2 f2 00 
00 00 00
[    2.020141]                                            ^
[    2.025448]  ffff0008c80e6c00: 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00
[    2.032659]  ffff0008c80e6c80: 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00
[    2.039862] 
==================================================================

This is actually from an NXP release branch but code is very close up 
upstream.

>> It shouldn't cause an issues in normal use because these structs are
>> always allocated on the stack.
> 
> Is packed necessary for these? I thought that if the beginning of the
> struct was naturally aligned and there was sometimes a byte or two at
> the end then having __packed wasn't useful. So maybe it's better to just
> drop __packed on all these structs and let the compiler decide it can
> add some padding on the stack? Or do we have arrays of these structs
> sitting in memory all next to each other and they need to be that way to
> be sent through the mailbox?

I'm not sure I understand the question: the structs are __packed because 
they represent the binary protocol for communicating with the "System 
Controller".

Without __packed the compiler could insert padding inside the structs 
and break the protocol.

As far as I understand compilers are still allowed to use padding on 
stack since that padding is outside the message struct itself.

--
Regards,
Leonard