When obtaining one or more optional resets, non-existent resets are
stored as NULL pointers, and all related error and cleanup paths need to
take this into account.
Currently only reset_control_put() and reset_control_bulk_put()
get this right. All of __reset_control_bulk_get(),
of_reset_control_array_get(), and reset_control_array_put() lack the
proper checking, causing NULL pointer dereferences on failure or
release.
Fix this by moving the existing check from reset_control_bulk_put() to
__reset_control_put_internal(), so it applies to all callers.
The double check in reset_control_put() doesn't hurt.
Fixes: 17c82e206d2a3cd8 ("reset: Add APIs to manage array of resets")
Fixes: 48d71395896d54ee ("reset: Add reset_control_bulk API")
Signed-off-by: Geert Uytterhoeven <[email protected]>
---
Crash seen on the Renesas R-Car S4-based Spider development board, using
a (buggy) out-of-tree patch[1]. As the (mutually-exclusive) PCIe host and
endpoint nodes share the same reset,
devm_reset_control_bulk_get_optional_exclusive() fails, and crashes in
the error path:
/soc/pcie@e65d0000 requests exclusive control over reset pwr shared with /soc/pcie-ep@e65d0000 on /soc/clock-controller@e6150000
Unable to handle kernel NULL pointer dereference at virtual address 000000000000001c
Mem abort info:
ESR = 0x0000000096000005
EC = 0x25: DABT (current EL), IL = 32 bits
SET = 0, FnV = 0
EA = 0, S1PTW = 0
FSC = 0x05: level 1 translation fault
Data abort info:
ISV = 0, ISS = 0x00000005, ISS2 = 0x00000000
CM = 0, WnR = 0, TnD = 0, TagAccess = 0
GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
user pgtable: 4k pages, 39-bit VAs, pgdp=00000004810b4000
[000000000000001c] pgd=0000000000000000, p4d=0000000000000000, pud=0000000000000000
Internal error: Oops: 0000000096000005 [#1] SMP
pstate: 00400005 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : __reset_control_put_internal+0x90/0xc8
lr : __reset_control_bulk_get+0xd4/0xd8
sp : ffffffc08174ba20
x29: ffffffc08174ba20 x28: ffffff8440010cd0 x27: ffffff8440a6f8c0
x26: 0000000000000001 x25: 0000000000000001 x24: 0000000000000000
x23: ffffff84409d4410 x22: ffffff8440a6fe48 x21: ffffffc080e8fae0
x20: 0000000000000005 x19: 0000000000000000 x18: 0000000000000000
x17: 6168732072777020 x16: 7465736572207265 x15: 766f206c6f72746e
x14: 6f63206576697375 x13: 3030303035313665 x12: 4072656c6c6f7274
x11: 6e6f632d6b636f6c x10: 632f636f732f206e x9 : 6c63786520737473
x8 : 6575716572203030 x7 : 205d383432323136 x6 : ffffffc080e8fae0
x5 : ffffffc080e8fae0 x4 : 0000000000000004 x3 : 0000000000000000
x2 : 000000000000001c x1 : 00000000ffffffff x0 : 000000000000001c
Call trace:
__reset_control_put_internal+0x90/0xc8
__reset_control_bulk_get+0xd4/0xd8
__devm_reset_control_bulk_get+0x78/0xcc
dw_pcie_get_resources+0x2e8/0x344
dw_pcie_host_init+0x64/0x538
rcar_gen4_pcie_probe+0x1a8/0x1ec
platform_probe+0x68/0xb8
really_probe+0x140/0x278
__driver_probe_device+0xf4/0x10c
driver_probe_device+0x50/0x100
__device_attach_driver+0xb0/0xd0
bus_for_each_drv+0xa8/0xd0
__device_attach_async_helper+0x70/0xc4
async_run_entry_fn+0x38/0x108
process_scheduled_works+0x1c4/0x270
worker_thread+0x1fc/0x26c
kthread+0xbc/0xcc
ret_from_fork+0x10/0x20
Code: 97f88daf 14000009 91007262 12800001 (b8610041)
---[ end trace 0000000000000000 ]---
[1] "[PATCH v3] reset: Exclusive resets must be dedicated to a single
hardware block"
https://lore.kernel.org/r/[email protected]
---
drivers/reset/core.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 7ece6a8e9858555d..4d5a78d3c085bc76 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -807,6 +807,9 @@ static void __reset_control_put_internal(struct reset_control *rstc)
{
lockdep_assert_held(&reset_list_mutex);
+ if (IS_ERR_OR_NULL(rstc))
+ return;
+
kref_put(&rstc->refcnt, __reset_control_release);
}
@@ -1017,11 +1020,8 @@ EXPORT_SYMBOL_GPL(reset_control_put);
void reset_control_bulk_put(int num_rstcs, struct reset_control_bulk_data *rstcs)
{
mutex_lock(&reset_list_mutex);
- while (num_rstcs--) {
- if (IS_ERR_OR_NULL(rstcs[num_rstcs].rstc))
- continue;
+ while (num_rstcs--)
__reset_control_put_internal(rstcs[num_rstcs].rstc);
- }
mutex_unlock(&reset_list_mutex);
}
EXPORT_SYMBOL_GPL(reset_control_bulk_put);
--
2.34.1
On Wed, 29 Nov 2023 17:55:33 +0100, Geert Uytterhoeven wrote:
> When obtaining one or more optional resets, non-existent resets are
> stored as NULL pointers, and all related error and cleanup paths need to
> take this into account.
>
> Currently only reset_control_put() and reset_control_bulk_put()
> get this right. All of __reset_control_bulk_get(),
> of_reset_control_array_get(), and reset_control_array_put() lack the
> proper checking, causing NULL pointer dereferences on failure or
> release.
>
> [...]
Applied to reset/fixes, thanks!
[1/1] reset: Fix crash when freeing non-existent optional resets
https://git.pengutronix.de/cgit/pza/linux/commit/?id=4a6756f56bcf
regards
Philipp