Re: [PATCH] nvdimm: ndtest: remove redundant NULL check before vfree()

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

Pre-existing issues:
- [High] Use-after-free / list corruption in ndtest_alloc_resource() error path.
- [High] Missing error check in ndtest_nvdimm_init() leads to NULL pointer dereference.
--

commit 96f1b7f6e50c7c8c92043b84b02d39517266ebe3
Author: Mohammad Shahid <[email protected]>

nvdimm: ndtest: remove redundant NULL check before vfree()

vfree() safely handles NULL pointers, so the explicit NULL check
before calling vfree() is unnecessary. This was reported by
ifnullfree.cocci.

> diff --git a/tools/testing/nvdimm/test/ndtest.c b/tools/testing/nvdimm/test/ndtest.c
> index 8e3b6be53839b..2051ad5d4882d 100644
> --- a/tools/testing/nvdimm/test/ndtest.c
> +++ b/tools/testing/nvdimm/test/ndtest.c
> @@ -376,8 +376,7 @@ static void *ndtest_alloc_resource(struct ndtest_priv *p, size_t size,
>  buf_err:
>  	if (__dma && size >= DIMM_SIZE)
>  		gen_pool_free(ndtest_pool, __dma, size);
> -	if (buf)
> -		vfree(buf);
> +	vfree(buf);
>  	kfree(res);

[Severity: High]
This isn't a bug introduced by this patch, but could this error path cause
list corruption or a use-after-free?

Earlier in ndtest_alloc_resource(), res is added to the p->resources list:

	spin_lock(&ndtest_lock);
	list_add(&res->list, &p->resources);
	spin_unlock(&ndtest_lock);

If devm_add_action() fails, execution falls through to buf_err, where
kfree(res) is called. However, res is not removed from the p->resources
list before being freed.

When the driver core subsequently invokes devres_release_all() on failure,
the cleanup function ndtest_release_resource() for any previously
successful allocations will traverse and modify this corrupted list,
resulting in a use-after-free.

Would it make sense to call list_del() before freeing res here?


[Severity: High]
This is a pre-existing issue, but while looking at this file, I noticed a
potential NULL pointer dereference in ndtest_nvdimm_init():

tools/testing/nvdimm/test/ndtest.c:ndtest_nvdimm_init() {
    ...
    ndtest_dimm_register(p, d, id);
    ...
}

If ndtest_dimm_register() fails, such as due to an allocation failure for
the dimm device, it returns an error but leaves d->nvdimm as NULL. Because
ndtest_nvdimm_init() ignores this error, it successfully returns.

Later, ndtest_create_region() passes the NULL nvdimm pointer to
nvdimm_pmem_region_create(), which dereferences it inside
nd_region_create(), causing a crash.

Should the return value of ndtest_dimm_register() be checked to prevent
this?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1