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

[Breno Leitao <leitao@debian.org>]

On Fri, Jul 07, 2023 at 09:50:09AM -0700, Dave Jiang wrote:
> 
> 
> On 7/7/23 09:16, 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.
> > 
> > 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;
> 
> I think you will want to change this to 'return rc;' in order to reflect the
> error.

This is a good point also, and I can change it in v2, if there is an
agreement in this patch.