linux-kernel - Re: [PATCH v2] cxl: Check for invalid addresses returned from translation functions on errors

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: <aV_ySgCVvEjLoxZ2@aschofie-mobl2.lan>
Date: Thu, 8 Jan 2026 10:07:06 -0800
From: Alison Schofield <alison.schofield@...el.com>
To: Robert Richter <rrichter@....com>
CC: Davidlohr Bueso <dave@...olabs.net>, Jonathan Cameron
	<jonathan.cameron@...wei.com>, Dave Jiang <dave.jiang@...el.com>, "Vishal
 Verma" <vishal.l.verma@...el.com>, Ira Weiny <ira.weiny@...el.com>, "Dan
 Williams" <dan.j.williams@...el.com>, <linux-cxl@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] cxl: Check for invalid addresses returned from
 translation functions on errors

On Wed, Jan 07, 2026 at 01:03:00PM +0100, Robert Richter wrote:
> On 06.01.26 10:42:36, Alison Schofield wrote:
> > On Tue, Jan 06, 2026 at 06:23:58PM +0100, Robert Richter wrote:
> 
> > > @@ -3207,8 +3220,13 @@ static int region_offset_to_dpa_result(struct cxl_region *cxlr, u64 offset,
> > >  		cxled = p->targets[i];
> > >  		if (cxled->pos != pos)
> > >  			continue;
> > > +
> > > +		dpa = cxl_dpa_resource_start(cxled);
> > 
> > We want to return -ENXIO, not 0 in this case.
> > So jump out here immediately - right?
> > 
> > 		if (dpa == REsOURCE_SIZE_MAX)
> > 			return -ENXIO;
> 
> The users of region_offset_to_dpa_result() handle the
> RESOURCE_SIZE_MAX case. For that reason the dpa is not checked here.
> I did not want to change the function interface with that fix.

region_offset_to_dpa_result() intends to return an rc when it knows
it would return a bad result. This is that case. With this proposed
change, we avoid adding to a RESOURCE_SIZE_MAX, which is defensive,
but isn't quitting immediately correct? These fixups are all about
stopping when any ingredient to the calc looks bad. It looks bad,
let's stop right away.

Seeing that the callsites do this:

	rc = region_offset_to_dpa_result(cxlr, offset, &result);
        if (rc || !result.cxlmd || result.dpa == ULLONG_MAX) {
		...this failed

shows that they do some defensive programming and don't rely only 
on the rc. But the inverse does not seem right - relying on that.

Staring at it more, I guess you could refactor the function to be
a void and get rid of the rc entirely.

Is that the real simplification needed here?

-- Alison

> 
> Thanks for review.
> 
> -Robert
> 
> > 
> > 
> > > +		if (dpa != RESOURCE_SIZE_MAX)
> > > +			dpa += dpa_offset;
> > > +
> > >  		result->cxlmd = cxled_to_memdev(cxled);
> > > -		result->dpa = cxl_dpa_resource_start(cxled) + dpa_offset;
> > > +		result->dpa = dpa;
> > >  
> > >  		return 0;
> > >  	}