Re: [PATCH v5 1/6] cxl/test: Add test for module parameters

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The statically allocated `hmem_test_device.work` work item is not flushed or cancelled when the `cxl_test` module unloads, leading to a potential use-after-free.
- [High] cxl_test_init() leaks the cxl_acpi platform device on the err_root error path, leading to a use-after-free.
--

commit 3d4ed8e7e71c954497e1a45802d1e8387b3fcfc0
Author: Dave Jiang <dave.jiang@intel.com>

cxl/test: Add test for module parameters

Add a test for module paraters during module init to make sure that
only one is activated.

> Add a test for module paraters during module init to make sure that

[Severity: Low]
This isn't a bug, but there is a typo (paraters) in the commit message.

> diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
> index 296516eecfd6a..1c8ac8bdd8f7a 100644
> --- a/tools/testing/cxl/test/cxl.c
> +++ b/tools/testing/cxl/test/cxl.c

[ ... ]

>  static __init int cxl_test_init(void)
>  {
>  	int rc, i;
>  	struct range mappable;
>  
> +	/* Enforce a single module param active at a time */
> +	if (have_multiple_modparms())
> +		return -EINVAL;
> +
>  	cxl_acpi_test();
>  	cxl_core_test();
>  	cxl_mem_test();

[ ... ]

>  	rc = hmem_test_init();
>  	if (rc)
>  		goto err_mem;
>  
>  	return 0;
>  
>  err_mem:
>  	cxl_mem_exit();
>  err_root:
>  	platform_device_put(cxl_acpi);

[Severity: High]
This is a pre-existing issue, but does cxl_test_init() leak the cxl_acpi
platform device on this err_root error path?

Since cxl_mock_platform_device_add() successfully added the device to the bus,
should this use platform_device_unregister() instead of platform_device_put()
for the already active device?

Dropping the initial allocation reference leaves the device active on the bus.
The device's mock companion data (acpi0017_mock) is statically allocated in
the module. When the module initialization fails and the module is unloaded,
the backing memory is freed, leaving a dangling pointer on the active platform
bus that can cause a kernel panic upon access.

> diff --git a/tools/testing/cxl/test/hmem_test.c b/tools/testing/cxl/test/hmem_test.c
> index 3a1a089e1721b..0fa00f7e16db5 100644
> --- a/tools/testing/cxl/test/hmem_test.c
> +++ b/tools/testing/cxl/test/hmem_test.c
> @@ -3,8 +3,9 @@
>  #include <linux/moduleparam.h>
>  #include <linux/workqueue.h>
>  #include "../../../drivers/dax/bus.h"
> +#include "mock.h"
>  
> -static bool hmem_test;
> +bool hmem_test;

[ ... ]

>  static struct hmem_platform_device hmem_test_device = {
>  	.pdev = {
>  		.name = "hmem_platform",
>  		.id = 1,
>  		.dev = {
>  			.release = hmem_test_release,
>  		},
>  	},
>  	.work = __WORK_INITIALIZER(hmem_test_device.work, hmem_test_work),
>  };

[ ... ]

>  void hmem_test_exit(void)
>  {
>  	if (hmem_test)
>  		platform_device_unregister(&hmem_test_device.pdev);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but can this leak the
hmem_test_device.work item when the cxl_test module unloads, leading to a
potential use-after-free?

The hmem_test_device is statically allocated in the module. When the dax_hmem
driver binds to this device, it queues hpdev->work on dax_hmem_wq.

On module unload, hmem_test_exit() unregisters the device but fails to call
cancel_work_sync() or flush_work(). The device release callback zeroes out
the structure while the work might still be queued. Since the module memory
is then freed, the workqueue will access freed memory when it eventually
processes the work item.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611234409.256765-1-dave.jiang@intel.com?part=1