nvdimm: check for null return of devm_kmalloc in nd_pfn_probe

[PATCH] nvdimm: check for null return of devm_kmalloc in nd_pfn_probe

Posted by Kang Chen 2 years, 6 months ago

devm_kmalloc may fails, pfn_sb might be null and will cause
null pointer dereference later.

Signed-off-by: Kang Chen <void0red@gmail.com>
---
 drivers/nvdimm/pfn_devs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
index af7d93015..d24fad175 100644
--- a/drivers/nvdimm/pfn_devs.c
+++ b/drivers/nvdimm/pfn_devs.c
@@ -640,6 +640,8 @@ int nd_pfn_probe(struct device *dev, struct nd_namespace_common *ndns)
 	if (!pfn_dev)
 		return -ENOMEM;
 	pfn_sb = devm_kmalloc(dev, sizeof(*pfn_sb), GFP_KERNEL);
+	if (!pfn_sb)
+		return -ENOMEM;
 	nd_pfn = to_nd_pfn(pfn_dev);
 	nd_pfn->pfn_sb = pfn_sb;
 	rc = nd_pfn_validate(nd_pfn, PFN_SIG);
-- 
2.34.1

Re: [PATCH] nvdimm: check for null return of devm_kmalloc in nd_pfn_probe

Posted by Alison Schofield 2 years, 6 months ago

On Sun, Feb 26, 2023 at 01:56:15PM +0800, Kang Chen wrote:
> devm_kmalloc may fails, pfn_sb might be null and will cause
> null pointer dereference later.
> 
> Signed-off-by: Kang Chen <void0red@gmail.com>
> ---
>  drivers/nvdimm/pfn_devs.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
> index af7d93015..d24fad175 100644
> --- a/drivers/nvdimm/pfn_devs.c
> +++ b/drivers/nvdimm/pfn_devs.c
> @@ -640,6 +640,8 @@ int nd_pfn_probe(struct device *dev, struct nd_namespace_common *ndns)
>  	if (!pfn_dev)
>  		return -ENOMEM;
>  	pfn_sb = devm_kmalloc(dev, sizeof(*pfn_sb), GFP_KERNEL);
> +	if (!pfn_sb)
> +		return -ENOMEM;
>  	nd_pfn = to_nd_pfn(pfn_dev);
>  	nd_pfn->pfn_sb = pfn_sb;
>  	rc = nd_pfn_validate(nd_pfn, PFN_SIG);

Hi Kang,

I too, think the code is clearer if the failure to alloc is addressed
immediately. In this case, it seems we can't just return -ENOMEM.
The original code is detecting that NULL pfn_sb in nd_pfn_validate(),
and then doing this cleanup upon return:

	if (rc < 0) {
                nd_detach_ndns(pfn_dev, &nd_pfn->ndns);
                put_device(pfn_dev);

Perhaps refactor a bit to go right to the cleanup, as opposed to calling
nd_pfn_validate() when !pfn_sb.

Alison

> -- 
> 2.34.1
> 
>