Re: [PATCH] cxl/acpi: Release device after dev_err

[Alison Schofield <alison.schofield@intel.com>]

On Fri, Jul 07, 2023 at 09:16:16AM -0700, Breno Leitao wrote:
> Kfence is detecting a user-after-free in the CXL, when cxl_decoder_add()
> fails. Kfence drops this message, after the following:
> 
>   BUG: KFENCE: use-after-free read in resource_string
> 
> This is happening in cxl_parse_cfmws(), and here is a simplified flow
> that is coming from Kfence.
> 
> Use-after-free:
> 	_dev_err
> 	cxl_parse_cfmws
> 	acpi_table_parse_entries_array
> 	acpi_table_parse_cedt
> 	cxl_acpi_probe
> 
> Free:
> 	cxl_decoder_release
> 	device_release
> 	kobject_put
> 	cxl_parse_cfmws
> 	acpi_table_parse_entries_array
> 	acpi_table_parse_cedt
> 	cxl_acpi_probe
> 
> Alloc:
> 	cxl_decoder_alloc
> 	cxl_parse_cfmws
> 	acpi_table_parse_entries_array
> 	acpi_table_parse_cedt
> 	cxl_acpi_probe
> 	platform_probe
> 
> From my reading of the issue, the device struct being used by
> dev_err() was removed in the put_device() before.

Hi Breno, 

I'm not familiar w kfence, but I don't follow what it finds
suspect here. Does kfence point to exact offensive lines of
code, or ???

The put_device() removed &cxld->dev and the dev_err() that
this patch moves the put after, was using 'dev', which was
assigned from ctx.dev. It is not the same as &cxld->dev. I
wonder if Kfence thinks we can get to the next dev_dbg()
statement and misuse &cxld->dev.

More below...


> 
> Put the device just after the message is printed.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  drivers/cxl/acpi.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 658e6b84a769..5179bf4211d8 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -291,14 +291,13 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
>  	}
>  	rc = cxl_decoder_add(cxld, target_map);
>  err_xormap:
> -	if (rc)
> -		put_device(&cxld->dev);
> -	else
> -		rc = cxl_decoder_autoremove(dev, cxld);
>  	if (rc) {
>  		dev_err(dev, "Failed to add decode range [%#llx - %#llx]\n",
>  			cxld->hpa_range.start, cxld->hpa_range.end);
> +		put_device(&cxld->dev);
>  		return 0;
> +	} else {
> +		rc = cxl_decoder_autoremove(dev, cxld);
>  	}
>  	dev_dbg(dev, "add: %s node: %d range [%#llx - %#llx]\n",
>  		dev_name(&cxld->dev),
> -- 
> 2.34.1
> 

(pulled in fresh code snippet to get the dev_dbg() in view.)

>	}
>	rc = cxl_decoder_add(cxld, target_map);
>err_xormap:
>	if (rc)
>		put_device(&cxld->dev);
>

This puts &cxld->dev, not dev.

>
>	else
>		rc = cxl_decoder_autoremove(dev, cxld);
>	if (rc) {
>		dev_err(dev, "Failed to add decode range [%#llx - %#llx]\n",
>			cxld->hpa_range.start, cxld->hpa_range.end);
>		return 0;

This return avoids getting to the next dev_dbg() statement after
put_device(). We cannot get to the next dev_dbg() statement when
rc is non zero, but it seems kfence thinks we can.

>	}
>	dev_dbg(dev, "add: %s node: %d range [%#llx - %#llx]\n",
>		dev_name(&cxld->dev),

If we could get here, after the put_device(), that would be bad.

>		phys_to_target_node(cxld->hpa_range.start),
>		cxld->hpa_range.start, cxld->hpa_range.end);
>
>	return 0;