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

Leonard Crestez
2020-02-20T12:25:45Z

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