On 6/3/26 10:57 AM, Rafael J. Wysocki wrote:
> From: "Rafael J. Wysocki" <[email protected]>
>
> If acpi_nfit_init() fails after adding the acpi_desc object to the
> acpi_descs list, that object is never removed from that list because
> the acpi_nfit_shutdown() devm action is not added for the NFIT device
> in that case. Next, the acpi_nfit_init() failure causes
> acpi_nfit_probe() to fail, the acpi_desc object is freed, and a
> dangling pointer is left behind in the acpi_descs. Any subsequent
> ACPI Machine Check Exception will trigger nfit_handle_mce() which
> iterates over acpi_descs and so a use-after-free will occur.
>
> Moreover, if acpi_nfit_probe() returns 0 after installing a notify
> handler for the NFIT device and without allocating the acpi_desc
> object and setting the NFIT device's driver data pointer, the
> acpi_desc object will be allocated by acpi_nfit_update_notify()
> and acpi_nfit_init() will be called to initialize it. Regardless
> of whether or not acpi_nfit_init() fails in that case, the
> acpi_nfit_shutdown() devm action is not added for the NFIT device
> and acpi_desc is never removed from the acpi_descs list. If the
> acpi_desc object is freed subsequently on driver removal, any
> subsequent ACPI MCE will lead to a use-after-free like in the
> previous case.
>
> To address the first issue mentioned above, make acpi_nfit_probe()
> call acpi_nfit_shutdown() directly on acpi_nfit_init() failures and
> to address the other one, add a remove callback to the driver and
> make it call acpi_nfit_shutdown(). Also, since it is now possible to
> pass NULL to acpi_nfit_shutdown() or the acpi_desc object passed to it
> may not have been initialized, add checks against NULL for acpi_desc and
> its nvdimm_bus field to that function and make acpi_nfit_unregister()
> clear the latter after unregistering the NVDIMM bus.
>
> Fixes: a61fe6f7902e ("nfit, tools/testing/nvdimm: unify common init for
> acpi_nfit_desc")
> Fixes: fbabd829fe76 ("acpi, nfit: fix module unload vs workqueue shutdown
> race")
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> Cc: All applicable <[email protected]>
Reviewed-by: Dave Jiang <[email protected]>
> ---
> drivers/acpi/nfit/core.c | 18 +++++++++++++++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 8024cd3cad14..01c73be0bd00 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -3069,6 +3069,8 @@ static void acpi_nfit_unregister(void *data)
> struct acpi_nfit_desc *acpi_desc = data;
>
> nvdimm_bus_unregister(acpi_desc->nvdimm_bus);
> + /* The nvdimm_bus object may have been freed, so clear the pointer. */
> + acpi_desc->nvdimm_bus = NULL;
> }
>
> int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, void *data, acpi_size
> sz)
> @@ -3301,7 +3303,10 @@ static void acpi_nfit_notify(acpi_handle handle, u32
> event, void *data)
> void acpi_nfit_shutdown(void *data)
> {
> struct acpi_nfit_desc *acpi_desc = data;
> - struct device *bus_dev = to_nvdimm_bus_dev(acpi_desc->nvdimm_bus);
> + struct device *bus_dev;
> +
> + if (!acpi_desc || !acpi_desc->nvdimm_bus)
> + return;
>
> /*
> * Destruct under acpi_desc_lock so that nfit_handle_mce does not
> @@ -3316,6 +3321,7 @@ void acpi_nfit_shutdown(void *data)
> mutex_unlock(&acpi_desc->init_mutex);
> cancel_delayed_work_sync(&acpi_desc->dwork);
>
> + bus_dev = to_nvdimm_bus_dev(acpi_desc->nvdimm_bus);
> /*
> * Bounce the nvdimm bus lock to make sure any in-flight
> * acpi_nfit_ars_rescan() submissions have had a chance to
> @@ -3388,9 +3394,14 @@ static int acpi_nfit_probe(struct platform_device
> *pdev)
> sz - sizeof(struct acpi_table_nfit));
>
> if (rc)
> - return rc;
> + acpi_nfit_shutdown(acpi_desc);
>
> - return devm_add_action_or_reset(dev, acpi_nfit_shutdown, acpi_desc);
> + return rc;
> +}
> +
> +static void acpi_nfit_remove(struct platform_device *pdev)
> +{
> + acpi_nfit_shutdown(platform_get_drvdata(pdev));
> }
>
> static void acpi_nfit_update_notify(struct device *dev, acpi_handle handle)
> @@ -3474,6 +3485,7 @@ MODULE_DEVICE_TABLE(acpi, acpi_nfit_ids);
>
> static struct platform_driver acpi_nfit_driver = {
> .probe = acpi_nfit_probe,
> + .remove = acpi_nfit_remove,
> .driver = {
> .name = "acpi-nfit",
> .acpi_match_table = acpi_nfit_ids,
