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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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