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

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

On Tue, Jul 11, 2023 at 09:17:04AM -0700, Breno Leitao wrote:
> Kfence detected an user-after-free in the CXL driver. This happens in
> the cxl_decoder_add() fail path. Kfence drops this message:
> 
>   BUG: KFENCE: use-after-free read in resource_string
> 
> This is happening in cxl_parse_cfmws(), where put_device() is called,
> releasing cxld->dev, and then, later, dev_err(cxld->dev) is called
> referencing the released device.
> 
> Just release the device after the message is printed/dev_err().  On top
> of that, cxl_parse_cfmws() must fail (returns rc) in case of
> cxl_decoder_add() or cxl_decoder_autoremove() failing (rc != 0), instead
> of swallowing the error and returning 0.

Breno,

Is kfence satisfied if you do this?

iff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 658e6b84a769..642983da01cb 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -297,7 +297,7 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
                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);
+                       res->start, res->end);
                return 0;
        }
        dev_dbg(dev, "add: %s node: %d range [%#llx - %#llx]\n",

Alison


> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
> v1 -> v2
>  * Return the error (rc) instead of swalling it
> 
> ---
>  drivers/cxl/acpi.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 658e6b84a769..efead5cc8a89 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -291,14 +291,16 @@ 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);
> -		return 0;
> +		put_device(&cxld->dev);
> +		return rc;
> +	}
> +	rc = cxl_decoder_autoremove(dev, cxld);
> +	if (rc) {
> +		dev_err(dev, "Failed to register autoremove action\n");
> +		return rc;
>  	}
>  	dev_dbg(dev, "add: %s node: %d range [%#llx - %#llx]\n",
>  		dev_name(&cxld->dev),
> -- 
> 2.34.1
>