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

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

On Fri, Jul 07, 2023 at 05:33:02PM -0700, Dave Jiang wrote:
> 
> 
> On 7/7/23 15:17, Alison Schofield wrote:
> > 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;
> > 
> 
> Seems like the code can be cleaned up this way. The double check of if (rc)
> is kinda weird anyhow.
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 7e1765b09e04..0573b476d29c 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -291,21 +291,21 @@ 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);

Why do you think the put_device() is making deref of cxld bad here?
That cxld is still present.

Maybe the unrelated bug here is that we are leaking the res. 
Rather that returning directly, take the kfree(res) path out.


> +               return rc;
>         }
> +
> +       rc = cxl_decoder_autoremove(dev, cxld);
> +

Now this rc is not examined.


>         dev_dbg(dev, "add: %s node: %d range [%#llx - %#llx]\n",
>                 dev_name(&cxld->dev),
>                 phys_to_target_node(cxld->hpa_range.start),
>                 cxld->hpa_range.start, cxld->hpa_range.end);
> 
> -       return 0;
> +       return rc;
> 
>  err_insert:
>         kfree(res->name);