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

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

On Tue,  4 Jun 2024 19:19:28 -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.
> 
> BUG: kernel NULL pointer dereference, address: 0000000000000050
> RIP: 0010:__cxl_dpa_to_region+0x8c/0xc0 [cxl_core]
> Call Trace:
>  <TASK>
>  ? show_regs+0x5f/0x70
>  ? __die+0x1f/0x70
>  ? page_fault_oops+0x14b/0x430
>  ? __cxl_dpa_to_region+0x8c/0xc0 [cxl_core]
>  ? search_exception_tables+0x5b/0x60
>  ? fixup_exception+0x22/0x300
>  ? kernelmode_fixup_or_oops.constprop.0+0x5a/0x70
>  ? __bad_area_nosemaphore+0x166/0x230
>  ? up_read+0x43/0x90
>  ? bad_area_nosemaphore+0x11/0x20
>  ? do_user_addr_fault+0x2cb/0x6b0
>  ? find_held_lock+0x31/0x90
>  ? exc_page_fault+0x6e/0x220
>  ? asm_exc_page_fault+0x27/0x30
>  ? __cxl_dpa_to_region+0x8c/0xc0 [cxl_core]
>  ? __cxl_dpa_to_region+0x35/0xc0 [cxl_core]
>  ? __pfx___cxl_dpa_to_region+0x10/0x10 [cxl_core]
>  device_for_each_child+0x4a/0x70
>  cxl_dpa_to_region+0x61/0x70 [cxl_core]
>  cxl_inject_poison+0xde/0x1e0 [cxl_core]
>  cxl_debugfs_poison_inject+0x9/0x10 [cxl_mem]
> 
> 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>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

This simplified version works for me so keep the tag... However given the spirit of
the patch changed rather a lot would have been reasonable to drop the RB.

> ---
> 
> Changes in v2:
> - emit endpoint dev_dbg() message only, rm cxlr-ness (Dan)
> - rm redundant null cxled check (Dan)
> - add stack trace to commit log (Dan)
> - s/Avoid/Fix in commit msg (Dan)
> - add Reviewed-by Tag (Jonathan)
> 
>  drivers/cxl/core/region.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 3c2b6144be23..d9819650c529 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2700,9 +2700,13 @@ static int __cxl_dpa_to_region(struct device *dev, void *arg)
>  	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.
> +	 */
> +	dev_dbg(dev, "dpa:0x%llx mapped in endpoint:%s\n", dpa,
> +		dev_name(dev));
>  	ctx->cxlr = cxled->cxld.region;
>  
>  	return 1;
> 
> base-commit: 49ba7b515c4c0719b866d16f068e62d16a8a3dd1