soc: xilinx: Fix race condition in event registration
PATCH v2 1/2 soc: xilinx: Fix race condition in event registration
The zynqmp_power driver registers handlers for suspend and subsystem restart events using register_event(). However, the work structures (zynqmp_pm_init_suspend_work and zynqmp_pm_init_restart_work) used by these handlers were allocated and initialized after the registration call.
This created a race window where, if the firmware triggered an event immediately after registration but before allocation, the callback (suspend_event_callback or subsystem_restart_event_callback) would dereference a NULL pointer in work_pending(), leading to a crash.
Fix this by allocating and initializing the work structures before registering the events.
--- drivers/soc/xilinx/zynqmp_power.c | 43 ++++++++++++------------------- 1 file changed, 17 insertions(+), 26 deletions(-)
drivers/soc/xilinx/zynqmp_power.c +17 -26
@@ -303,18 +303,18 @@ static int zynqmp_pm_probe(struct platform_device *pdev)* is not available to use) or -ENODEV(Xilinx Event Manager not compiled), * then use ipi-mailbox or interrupt method. */ + zynqmp_pm_init_suspend_work =devm_kzalloc (&pdev->dev, +sizeof (struct zynqmp_pm_work_struct ), +GFP_KERNEL ); +if (!zynqmp_pm_init_suspend_work) +return -ENOMEM ; + +INIT_WORK (&zynqmp_pm_init_suspend_work->callback_work, + zynqmp_pm_init_suspend_work_fn); + ret =register_event (&pdev->dev,PM_INIT_SUSPEND_CB ,0 ,0 , false, suspend_event_callback);if (!ret) { - zynqmp_pm_init_suspend_work =devm_kzalloc (&pdev->dev, -sizeof (struct zynqmp_pm_work_struct ), -GFP_KERNEL ); -if (!zynqmp_pm_init_suspend_work) -return -ENOMEM ; - -INIT_WORK (&zynqmp_pm_init_suspend_work->callback_work, - zynqmp_pm_init_suspend_work_fn); - ret =zynqmp_pm_get_family_info (&pm_family_code);if (ret <0 )return ret;
@@ -326,14 +326,6 @@ static int zynqmp_pm_probe(struct platform_device *pdev)else return -ENODEV ; - ret =register_event (&pdev->dev,PM_NOTIFY_CB , node_id,EVENT_SUBSYSTEM_RESTART , - false, subsystem_restart_event_callback); -if (ret) { -dev_err (&pdev->dev,"Failed to Register with Xilinx Event manager %d\n" , - ret); -return ret; - } - zynqmp_pm_init_restart_work =devm_kzalloc (&pdev->dev,sizeof (struct zynqmp_pm_work_struct ),GFP_KERNEL );
@@ -342,19 +334,18 @@ static int zynqmp_pm_probe(struct platform_device *pdev)INIT_WORK (&zynqmp_pm_init_restart_work->callback_work, zynqmp_pm_subsystem_restart_work_fn); + + ret =register_event (&pdev->dev,PM_NOTIFY_CB , node_id,EVENT_SUBSYSTEM_RESTART , + false, subsystem_restart_event_callback); +if (ret) { +dev_err (&pdev->dev,"Failed to Register with Xilinx Event manager %d\n" , + ret); +return ret; + } }else if (ret != -EACCES && ret != -ENODEV ) {dev_err (&pdev->dev,"Failed to Register with Xilinx Event manager %d\n" , ret);return ret; }else if (of_property_present (pdev->dev.of_node,"mboxes" )) { - zynqmp_pm_init_suspend_work = -devm_kzalloc (&pdev->dev, -sizeof (struct zynqmp_pm_work_struct ), -GFP_KERNEL ); -if (!zynqmp_pm_init_suspend_work) -return -ENOMEM ; - -INIT_WORK (&zynqmp_pm_init_suspend_work->callback_work, - zynqmp_pm_init_suspend_work_fn); client =devm_kzalloc (&pdev->dev,sizeof (*client),GFP_KERNEL );if (!client)return -ENOMEM ;
Signature · 1 line
2.49.0
PATCH v2 2/2 soc: xilinx: Shutdown and free rx mailbox channel
A mbox rx channel is requested using mbox_request_channel_byname() in probe. In remove callback, the rx mailbox channel is cleaned up when the rx_chan is NULL due to incorrect condition check. The mailbox channel is not shutdown and it can receive messages even after the device removal. This leads to use after free. Also the channel resources are not freed. Fix this by checking the rx_chan correctly.
--- drivers/soc/xilinx/zynqmp_power.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/soc/xilinx/zynqmp_power.c +3 -1
@@ -389,8 +389,10 @@ static void zynqmp_pm_remove(struct platform_device *pdev) {sysfs_remove_file (&pdev->dev.kobj, &dev_attr_suspend_mode.attr); -if (!rx_chan) +if (rx_chan) {mbox_free_channel (rx_chan); + rx_chan =NULL ; + } }static const struct of_device_id pm_of_match[] = {
Signature · 1 line
2.49.0
Michal Simek
maintainerQuoted 85 lines · Prasanna Kumar T S M
The
zynqmp_powerdriver registers handlers for suspend and subsystem restart events usingregister_event(). However, the work structures (zynqmp_pm_init_suspend_workandzynqmp_pm_init_restart_work) used by these handlers were allocated and initialized after the registration call.This created a race window where, if the firmware triggered an event immediately after registration but before allocation, the callback (
suspend_event_callbackorsubsystem_restart_event_callback) would dereference a NULL pointer inwork_pending(), leading to a crash.Fix this by allocating and initializing the work structures before registering the events.
Fixes fcf544ac6439 ("soc: xilinx: Add cb event for subsystem restart")Signed-off-by Prasanna Kumar T S M <[email protected]>--- drivers/soc/xilinx/zynqmp_power.c | 43 ++++++++++++------------------- 1 file changed, 17 insertions(+), 26 deletions(-)drivers/soc/xilinx/zynqmp_power.c +17
-19@@ -303,18 +303,18 @@ static int zynqmp_pm_probe(struct platform_device *pdev)* is not available to use) or -ENODEV(Xilinx Event Manager not compiled), * then use ipi-mailbox or interrupt method. */ + zynqmp_pm_init_suspend_work =devm_kzalloc (&pdev->dev, +sizeof (struct zynqmp_pm_work_struct ), +GFP_KERNEL ); +if (!zynqmp_pm_init_suspend_work) +return -ENOMEM ; + +INIT_WORK (&zynqmp_pm_init_suspend_work->callback_work, + zynqmp_pm_init_suspend_work_fn); + ret =register_event (&pdev->dev,PM_INIT_SUSPEND_CB ,0 ,0 , false, suspend_event_callback);if (!ret) { - zynqmp_pm_init_suspend_work =devm_kzalloc (&pdev->dev, -sizeof (struct zynqmp_pm_work_struct ), -GFP_KERNEL ); -if (!zynqmp_pm_init_suspend_work) -return -ENOMEM ; - -INIT_WORK (&zynqmp_pm_init_suspend_work->callback_work, - zynqmp_pm_init_suspend_work_fn); - ret =zynqmp_pm_get_family_info (&pm_family_code);if (ret <0 )return ret;@@ -326,14 +326,6 @@ static int zynqmp_pm_probe(struct platform_device *pdev)else return -ENODEV ; - ret =register_event (&pdev->dev,PM_NOTIFY_CB , node_id,EVENT_SUBSYSTEM_RESTART , - false, subsystem_restart_event_callback); -if (ret) { -dev_err (&pdev->dev,"Failed to Register with Xilinx Event manager %d\n" , - ret); -return ret; - } - zynqmp_pm_init_restart_work =devm_kzalloc (&pdev->dev,sizeof (struct zynqmp_pm_work_struct ),GFP_KERNEL );@@ -342,19 +334,18 @@ static int zynqmp_pm_probe(struct platform_device *pdev)INIT_WORK (&zynqmp_pm_init_restart_work->callback_work, zynqmp_pm_subsystem_restart_work_fn); + + ret =register_event (&pdev->dev,PM_NOTIFY_CB , node_id,EVENT_SUBSYSTEM_RESTART , + false, subsystem_restart_event_callback); +if (ret) { +dev_err (&pdev->dev,"Failed to Register with Xilinx Event manager %d\n" , + ret); +return ret; + } }else if (ret != -EACCES && ret != -ENODEV ) {dev_err (&pdev->dev,"Failed to Register with Xilinx Event manager %d\n" , ret);return ret; }else if (of_property_present (pdev->dev.of_node,"mboxes" )) { - zynqmp_pm_init_suspend_work = - devm_kzalloc(&pdev->dev,
drivers/soc/xilinx/zynqmp_power.c static int zynqmp_pm_probe(struct platform_device *pdev) · line 351
Quoted 10 lines
drivers/soc/xilinx/zynqmp_power.c +0
-7-sizeof (struct zynqmp_pm_work_struct ), - GFP_KERNEL); -if (!zynqmp_pm_init_suspend_work) -return -ENOMEM ; - -INIT_WORK (&zynqmp_pm_init_suspend_work->callback_work, - zynqmp_pm_init_suspend_work_fn); client =devm_kzalloc (&pdev->dev,sizeof (*client),GFP_KERNEL );if (!client)return -ENOMEM ;
Applied both.
Thanks,
Michal