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

[Breno Leitao <leitao@debian.org>]

Hello Alison,

On Tue, Jul 11, 2023 at 10:52:39AM -0700, Alison Schofield wrote:
> 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?

Yes, this approach solves the complaints from kfence/kasan. I am
wondering if "cxld" is what is being freed up here at device_put().

> 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",

Do you prefer this patch above, mine patch or a mix of both?

Thanks