linux-kernel - Re: [PATCH v2] nvdimm: ndtest: Return -ENOMEM if devm_kcalloc() fails in ndtest

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: <aNLsXewwa0LXcRUk@aschofie-mobl2.lan>
Date: Tue, 23 Sep 2025 11:52:13 -0700
From: Alison Schofield <alison.schofield@...el.com>
To: Dave Jiang <dave.jiang@...el.com>
CC: Guangshuo Li <lgs201920130244@...il.com>, Dan Williams
	<dan.j.williams@...el.com>, Vishal Verma <vishal.l.verma@...el.com>, "Ira
 Weiny" <ira.weiny@...el.com>, Santosh Sivaraj <santosh@...six.org>,
	<nvdimm@...ts.linux.dev>, <linux-kernel@...r.kernel.org>,
	<stable@...r.kernel.org>
Subject: Re: [PATCH v2] nvdimm: ndtest: Return -ENOMEM if devm_kcalloc()
 fails in ndtest_probe()

On Tue, Sep 23, 2025 at 09:38:33AM -0700, Dave Jiang wrote:
> 
> 
> On 9/23/25 5:59 AM, Guangshuo Li wrote:
> > devm_kcalloc() may fail. ndtest_probe() allocates three DMA address
> > arrays (dcr_dma, label_dma, dimm_dma) and later unconditionally uses
> > them in ndtest_nvdimm_init(), which can lead to a NULL pointer
> > dereference under low-memory conditions.
> > 
> > Check all three allocations and return -ENOMEM if any allocation fails.
> > Do not emit an extra error message since the allocator already warns on
> > allocation failure.
> > 
> > Fixes: 9399ab61ad82 ("ndtest: Add dimms to the two buses")
> > Cc: stable@...r.kernel.org
> > Signed-off-by: Guangshuo Li <lgs201920130244@...il.com>
> > ---
> > Changes in v2:
> > - Drop pr_err() on allocation failure; only NULL-check and return -ENOMEM.
> > - No other changes.
> > ---
> >  tools/testing/nvdimm/test/ndtest.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/tools/testing/nvdimm/test/ndtest.c b/tools/testing/nvdimm/test/ndtest.c
> > index 68a064ce598c..abdbe0c1cb63 100644
> > --- a/tools/testing/nvdimm/test/ndtest.c
> > +++ b/tools/testing/nvdimm/test/ndtest.c
> > @@ -855,6 +855,9 @@ static int ndtest_probe(struct platform_device *pdev)
> >  	p->dimm_dma = devm_kcalloc(&p->pdev.dev, NUM_DCR,
> >  				  sizeof(dma_addr_t), GFP_KERNEL);
> >  
> > +	if (!p->dcr_dma || !p->label_dma || !p->dimm_dma)
> > +		return -ENOMEM;
> 
> Why not just check as it allocates instead of doing it all at the end? If an allocation failed, no need to attempt to allocate more (which probably leads to more failures).

Following on Dave's feedback and looking at the function as a whole,
it does have a pattern that this patch can replicate:

It does this now:
	rc = do_something();
	if (rc)
		goto err;

So, continue that pattern with the added NULL checks:

	p->dcr_dma = devm_kcalloc(&p->pdev.dev, NUM_DCR,
				  sizeof(dma_addr_t), GFP_KERNEL);
	if (!p->dcr_dma) {
		rc = -ENOMEM;
		goto err;
	}
and repeat above for all 3 allocs.

And, maybe even change that first ndtest_bus_register() failure
to follow the same pattern.

--Alison

> 
> DJ
> 
> > +
> >  	rc = ndtest_nvdimm_init(p);
> >  	if (rc)
> >  		goto err;
> 
>