linux-kernel - Re: [PATCH] nvdimm: check for null return of devm_kmalloc in nd_pfn

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Y/vFFDiXw6J5LD3X@aschofie-mobl2>
Date:   Sun, 26 Feb 2023 12:46:12 -0800
From:   Alison Schofield <alison.schofield@...el.com>
To:     Kang Chen <void0red@...il.com>
Cc:     dan.j.williams@...el.com, vishal.l.verma@...el.com,
        dave.jiang@...el.com, ira.weiny@...el.com, nvdimm@...ts.linux.dev,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] nvdimm: check for null return of devm_kmalloc in
 nd_pfn_probe

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@...il.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
> 
>