netdev - Re: [PATCH v5 10/27] cxl: harden resource_contains checks to handle zero size resources

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zz-fVWhTOFG4Nek-@aschofie-mobl2.lan>
Date: Thu, 21 Nov 2024 13:00:05 -0800
From: Alison Schofield <alison.schofield@...el.com>
To: Alejandro Lucero Palau <alucerop@....com>
Cc: alejandro.lucero-palau@....com, linux-cxl@...r.kernel.org,
	netdev@...r.kernel.org, dan.j.williams@...el.com,
	martin.habets@...inx.com, edward.cree@....com, davem@...emloft.net,
	kuba@...nel.org, pabeni@...hat.com, edumazet@...gle.com
Subject: Re: [PATCH v5 10/27] cxl: harden resource_contains checks to handle
 zero size resources

On Thu, Nov 21, 2024 at 09:22:33AM +0000, Alejandro Lucero Palau wrote:
> 
> On 11/21/24 02:46, Alison Schofield wrote:
> > On Mon, Nov 18, 2024 at 04:44:17PM +0000, alejandro.lucero-palau@....com wrote:
> > > From: Alejandro Lucero <alucerop@....com>
> > > 
> > > For a resource defined with size zero, resource_contains returns
> > > always true.
> > > 
> > I'm not following the premise above -
> > 
> > Looking at resource_contains() and the changes made below,
> > it seems the concern is with &cxlds->ram_res or &cxlds->pmem_res
> > being zero - because we already checked that the second param
> > 'res' is not zero a few lines above.
> > 
> > Looking at what happens when r1 is of size 0, I don't see how
> > resource_contains() returns always true.
> > 
> > In resource_contains(r1, r2), if r1 is of size 0, r1->start == r1->end.
> > The func can only return true if r2 is also of size 0 and located at
> > exactly r1->start. But, in this case, we are not going to get there
> > because we never send an r2 of size 0.
> > 
> > For any non-zero size r2 the func will always return false because
> > the size 0 r1 cannot encompass any range.
> > 
> > I could be misreading it all ;)
> 
> 
> The key is to know how a resource with size 0 is initialized, what can be
> understood looking at DEFINE_RES_NAMED macro. The end field is set as  size
> - 1.
> 
> With unsigned variables, as it is the case here, it means to have a resource
> as big as possible ... if you do not check first the size is not 0.
> 
> The pmem resource is explicitly initialized inside cxl_accel_state_create in
> the previous patch, so it has:
> 
> pmem_res->start = 0, pmem_res.end = 0xffffffffffffffff
> 
> the resource checked against is defined with, for example, a 256MB size:
> 
> res.start =0, res.end = 0xfffffff
> 
> 
> if you then use resource_contains(pmem_res, res), that implies always true,
> whatever the res range defined.
> 
> 
> All this confused me as well when facing it initially. I hope this
> explanation makes sense.
> 

Thanks for the explanation! I'm wondering if we are leaving a trap for the next
developer.

resource_contains() seems to have intended that a check for IORESOURCE_UNSET
would take care of the zero size case:

(5edb93b89f6c resource: Add resource_contains)

and it would if folks used _UNSET. Some check r1->start before calling
resource_contains().

One option would be to use _UNSET in this case, but that only covers us here,
and doesn't remove the trap ;)

How about hardening resource_contains():

ie: make resource_contains() return false if either res empty

 /* True iff r1 completely contains r2 */
 static inline bool resource_contains(const struct resource *r1, const struct resource *r2)
 {
+       if (!resource_size(r1) || !resource_size(r2))
+               return false;
        if (resource_type(r1) != resource_type(r2))
                return false;
        if (r1->flags & IORESOURCE_UNSET || r2->flags & IORESOURCE_UNSET)
		return false;
	return r1->start <= r2->start && r1->end >= r2->end;
}

-- Alison

> > 
> > --Alison
> > 
> > 
> > > Add resource size check before using it.
> > > 
> > > Signed-off-by: Alejandro Lucero <alucerop@....com>
> > > ---
> > >   drivers/cxl/core/hdm.c | 7 +++++--
> > >   1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > > index 223c273c0cd1..c58d6b8f9b58 100644
> > > --- a/drivers/cxl/core/hdm.c
> > > +++ b/drivers/cxl/core/hdm.c
> > > @@ -327,10 +327,13 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
> > >   	cxled->dpa_res = res;
> > >   	cxled->skip = skipped;
> > > -	if (resource_contains(&cxlds->pmem_res, res))
> > > +	if (resource_size(&cxlds->pmem_res) &&
> > > +	    resource_contains(&cxlds->pmem_res, res)) {
> > >   		cxled->mode = CXL_DECODER_PMEM;
> > > -	else if (resource_contains(&cxlds->ram_res, res))
> > > +	} else if (resource_size(&cxlds->ram_res) &&
> > > +		   resource_contains(&cxlds->ram_res, res)) {
> > >   		cxled->mode = CXL_DECODER_RAM;
> > > +	}
> > >   	else {
> > >   		dev_warn(dev, "decoder%d.%d: %pr mixed mode not supported\n",
> > >   			 port->id, cxled->cxld.id, cxled->dpa_res);
> > > -- 
> > > 2.17.1
> > > 
> > >