[PATCH] cxl/region: Avoid null pointer dereference in region lookup

[PATCH] cxl/region: Avoid null pointer dereference in region lookup

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

cxl_dpa_to_region() looks up a region based on a memdev and DPA.
It wrongly assumes an endpoint found mapping the DPA is also of
a fully assembled region. When not true it leads to a null pointer
dereference looking up the region name.

This appears during testing of region lookup after a failure to
assemble a BIOS defined region or if the lookup raced with the
assembly of the BIOS defined region.

Failure to clean up BIOS defined regions that fail assembly is an
issue in itself and a fix to that problem will alleviate some of
the impact. It will not alleviate the race condition so let's harden
this path.

The behavior change is that the kernel oops due to a null pointer
dereference is replaced with a dev_dbg() message noting that an
endpoint was mapped.

Additional comments are added so that future users of this function
can more clearly understand what it provides.

Fixes: 0a105ab28a4d ("cxl/memdev: Warn of poison inject or clear to a mapped region")
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/region.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 3c2b6144be23..88051bb673bf 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2688,22 +2688,33 @@ static int __cxl_dpa_to_region(struct device *dev, void *arg)
 {
 	struct cxl_dpa_to_region_context *ctx = arg;
 	struct cxl_endpoint_decoder *cxled;
+	struct cxl_region *cxlr;
 	u64 dpa = ctx->dpa;
 
 	if (!is_endpoint_decoder(dev))
 		return 0;
 
 	cxled = to_cxl_endpoint_decoder(dev);
-	if (!cxled->dpa_res || !resource_size(cxled->dpa_res))
+	if (!cxled || !cxled->dpa_res || !resource_size(cxled->dpa_res))
 		return 0;
 
 	if (dpa > cxled->dpa_res->end || dpa < cxled->dpa_res->start)
 		return 0;
 
-	dev_dbg(dev, "dpa:0x%llx mapped in region:%s\n", dpa,
-		dev_name(&cxled->cxld.region->dev));
+	/*
+	 * Stop the region search (return 1) when an endpoint mapping is
+	 * found. The region may not be fully constructed so offering
+	 * the cxlr in the context structure is not guaranteed.
+	 */
+	cxlr = cxled->cxld.region;
+	if (cxlr)
+		dev_dbg(dev, "dpa:0x%llx mapped in region:%s\n", dpa,
+			dev_name(&cxlr->dev));
+	else
+		dev_dbg(dev, "dpa:0x%llx mapped in endpoint:%s\n", dpa,
+			dev_name(dev));
 
-	ctx->cxlr = cxled->cxld.region;
+	ctx->cxlr = cxlr;
 
 	return 1;
 }

base-commit: 49ba7b515c4c0719b866d16f068e62d16a8a3dd1
-- 
2.37.3

Re: [PATCH] cxl/region: Avoid null pointer dereference in region lookup

[Jonathan Cameron]

On Mon,  3 Jun 2024 17:36:09 -0700
alison.schofield@intel.com wrote:

> From: Alison Schofield <alison.schofield@intel.com>
> 
> cxl_dpa_to_region() looks up a region based on a memdev and DPA.
> It wrongly assumes an endpoint found mapping the DPA is also of
> a fully assembled region. When not true it leads to a null pointer
> dereference looking up the region name.
> 
> This appears during testing of region lookup after a failure to
> assemble a BIOS defined region or if the lookup raced with the
> assembly of the BIOS defined region.
> 
> Failure to clean up BIOS defined regions that fail assembly is an
> issue in itself and a fix to that problem will alleviate some of
> the impact. It will not alleviate the race condition so let's harden
> this path.
> 
> The behavior change is that the kernel oops due to a null pointer
> dereference is replaced with a dev_dbg() message noting that an
> endpoint was mapped.
> 
> Additional comments are added so that future users of this function
> can more clearly understand what it provides.
> 
> Fixes: 0a105ab28a4d ("cxl/memdev: Warn of poison inject or clear to a mapped region")
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Nasty corner case. Fix looks correct to me.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/core/region.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 3c2b6144be23..88051bb673bf 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2688,22 +2688,33 @@ static int __cxl_dpa_to_region(struct device *dev, void *arg)
>  {
>  	struct cxl_dpa_to_region_context *ctx = arg;
>  	struct cxl_endpoint_decoder *cxled;
> +	struct cxl_region *cxlr;
>  	u64 dpa = ctx->dpa;
>  
>  	if (!is_endpoint_decoder(dev))
>  		return 0;
>  
>  	cxled = to_cxl_endpoint_decoder(dev);
> -	if (!cxled->dpa_res || !resource_size(cxled->dpa_res))
> +	if (!cxled || !cxled->dpa_res || !resource_size(cxled->dpa_res))
>  		return 0;
>  
>  	if (dpa > cxled->dpa_res->end || dpa < cxled->dpa_res->start)
>  		return 0;
>  
> -	dev_dbg(dev, "dpa:0x%llx mapped in region:%s\n", dpa,
> -		dev_name(&cxled->cxld.region->dev));
> +	/*
> +	 * Stop the region search (return 1) when an endpoint mapping is
> +	 * found. The region may not be fully constructed so offering
> +	 * the cxlr in the context structure is not guaranteed.
> +	 */
> +	cxlr = cxled->cxld.region;
> +	if (cxlr)
> +		dev_dbg(dev, "dpa:0x%llx mapped in region:%s\n", dpa,
> +			dev_name(&cxlr->dev));
> +	else
> +		dev_dbg(dev, "dpa:0x%llx mapped in endpoint:%s\n", dpa,
> +			dev_name(dev));
>  
> -	ctx->cxlr = cxled->cxld.region;
> +	ctx->cxlr = cxlr;
>  
>  	return 1;
>  }
> 
> base-commit: 49ba7b515c4c0719b866d16f068e62d16a8a3dd1

Re: [PATCH] cxl/region: Avoid null pointer dereference in region lookup

[Dan Williams]

Perhaps change the subject with: s/Avoid/Fix/?

alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> cxl_dpa_to_region() looks up a region based on a memdev and DPA.
> It wrongly assumes an endpoint found mapping the DPA is also of
> a fully assembled region. When not true it leads to a null pointer
> dereference looking up the region name.

Can you place the a snippet of the Call Trace here, that helps support
that the writeup came to the right conclusion, and it helps folks
hitting this error to find this patch in a web search.

> This appears during testing of region lookup after a failure to
> assemble a BIOS defined region or if the lookup raced with the
> assembly of the BIOS defined region.
> 
> Failure to clean up BIOS defined regions that fail assembly is an
> issue in itself and a fix to that problem will alleviate some of
> the impact. It will not alleviate the race condition so let's harden
> this path.
> 
> The behavior change is that the kernel oops due to a null pointer
> dereference is replaced with a dev_dbg() message noting that an
> endpoint was mapped.
> 
> Additional comments are added so that future users of this function
> can more clearly understand what it provides.
> 
> Fixes: 0a105ab28a4d ("cxl/memdev: Warn of poison inject or clear to a mapped region")
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  drivers/cxl/core/region.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 3c2b6144be23..88051bb673bf 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2688,22 +2688,33 @@ static int __cxl_dpa_to_region(struct device *dev, void *arg)
>  {
>  	struct cxl_dpa_to_region_context *ctx = arg;
>  	struct cxl_endpoint_decoder *cxled;
> +	struct cxl_region *cxlr;
>  	u64 dpa = ctx->dpa;
>  
>  	if (!is_endpoint_decoder(dev))
>  		return 0;
>  
>  	cxled = to_cxl_endpoint_decoder(dev);
> -	if (!cxled->dpa_res || !resource_size(cxled->dpa_res))
> +	if (!cxled || !cxled->dpa_res || !resource_size(cxled->dpa_res))

Why add the !cxled check? to_cxl_endpoint_decoder() only returns NULL if
someone messes up and sends a !is_endpoint_decoder(@dev) to this
conversion routine and that check was already performed above.

>  		return 0;
>  
>  	if (dpa > cxled->dpa_res->end || dpa < cxled->dpa_res->start)
>  		return 0;
>  
> -	dev_dbg(dev, "dpa:0x%llx mapped in region:%s\n", dpa,
> -		dev_name(&cxled->cxld.region->dev));
> +	/*
> +	 * Stop the region search (return 1) when an endpoint mapping is
> +	 * found. The region may not be fully constructed so offering
> +	 * the cxlr in the context structure is not guaranteed.
> +	 */

Comment looks good...

> +	cxlr = cxled->cxld.region;
> +	if (cxlr)
> +		dev_dbg(dev, "dpa:0x%llx mapped in region:%s\n", dpa,
> +			dev_name(&cxlr->dev));
> +	else
> +		dev_dbg(dev, "dpa:0x%llx mapped in endpoint:%s\n", dpa,
> +			dev_name(dev));

...but I am not sure gymanistics to improve a debug message are worth
it, especially because all of the callers either emit the found region
in a tracepoint or print a warning, so I would just be in favor of
deleting the debug message altogether, or at most just trimming to the
"mapped in endpoint" one.

Re: [PATCH] cxl/region: Avoid null pointer dereference in region lookup

[Alison Schofield]

On Tue, Jun 04, 2024 at 10:47:35AM -0700, Dan Williams wrote:
> Perhaps change the subject with: s/Avoid/Fix/?

OK

> 
> alison.schofield@ wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > cxl_dpa_to_region() looks up a region based on a memdev and DPA.
> > It wrongly assumes an endpoint found mapping the DPA is also of
> > a fully assembled region. When not true it leads to a null pointer
> > dereference looking up the region name.
> 
> Can you place the a snippet of the Call Trace here, that helps support
> that the writeup came to the right conclusion, and it helps folks
> hitting this error to find this patch in a web search.

Will add in v2

snip...
> 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 3c2b6144be23..88051bb673bf 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -2688,22 +2688,33 @@ static int __cxl_dpa_to_region(struct device *dev, void *arg)
> >  {
> >  	struct cxl_dpa_to_region_context *ctx = arg;
> >  	struct cxl_endpoint_decoder *cxled;
> > +	struct cxl_region *cxlr;
> >  	u64 dpa = ctx->dpa;
> >  
> >  	if (!is_endpoint_decoder(dev))
> >  		return 0;
> >  
> >  	cxled = to_cxl_endpoint_decoder(dev);
> > -	if (!cxled->dpa_res || !resource_size(cxled->dpa_res))
> > +	if (!cxled || !cxled->dpa_res || !resource_size(cxled->dpa_res))
> 
> Why add the !cxled check? to_cxl_endpoint_decoder() only returns NULL if
> someone messes up and sends a !is_endpoint_decoder(@dev) to this
> conversion routine and that check was already performed above.
> 

Abundance of caution I suppose.  I'll remove.

> >  		return 0;
> >  
> >  	if (dpa > cxled->dpa_res->end || dpa < cxled->dpa_res->start)
> >  		return 0;
> >  
> > -	dev_dbg(dev, "dpa:0x%llx mapped in region:%s\n", dpa,
> > -		dev_name(&cxled->cxld.region->dev));
> > +	/*
> > +	 * Stop the region search (return 1) when an endpoint mapping is
> > +	 * found. The region may not be fully constructed so offering
> > +	 * the cxlr in the context structure is not guaranteed.
> > +	 */
> 
> Comment looks good...
> 
> > +	cxlr = cxled->cxld.region;
> > +	if (cxlr)
> > +		dev_dbg(dev, "dpa:0x%llx mapped in region:%s\n", dpa,
> > +			dev_name(&cxlr->dev));
> > +	else
> > +		dev_dbg(dev, "dpa:0x%llx mapped in endpoint:%s\n", dpa,
> > +			dev_name(dev));
> 
> ...but I am not sure gymanistics to improve a debug message are worth
> it, especially because all of the callers either emit the found region
> in a tracepoint or print a warning, so I would just be in favor of
> deleting the debug message altogether, or at most just trimming to the
> "mapped in endpoint" one.

Yeah, probably went overboard here. Previously was offering the name
of the region so wanted to keep same - but seeing as this is a debug
only message, I think the endpoint alone, as you suggest is good.

Thanks for the review,
Alison