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

[Breno Leitao <leitao@debian.org>]

On Fri, Jul 07, 2023 at 03:17:58PM -0700, 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 ???

Unfortunately I do not lines that match anything public. Collecting them
might be hard also, since kfence problems during failure mode are not
easy to reproduce.

> 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.

Any chance that "struct device_type->release"() might be touching
ctx->dev? This is because "struct device_type->release"() is calling
during the put operation, which seems to be the one de-allocating the
resource.

> 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.

Oh, the problems is on dev_err() not on dev_dbg(). Basically on the
return path.

Here is what dmesg says:

	cxl root0: Failed to populate active decoder targets
	cxl_acpi ACPI0017:00: Failed to add decoder for [mem 0x4080000000-0x2baffffffff flags 0x200]
	cxl root0: Failed to populate active decoder targets
	cxl_acpi ACPI0017:00: Failed to add decoder for [mem 0x2bb00000000-0x5357fffffff flags 0x200]
	cxl root0: Failed to populate active decoder targets
	==================================================================
	BUG: KFENCE: use-after-free read in resource_string+0x80/0x570\x0a
	Use-after-free read at 0x(____ptrval____) (in kfence-#111):
	 resource_string+0x80/0x570
	 pointer+0x389/0x3c0
	 vsnprintf+0x214/0x670
	 pointer+0x1b2/0x3c0
	 vsnprintf+0x214/0x670
	 vprintk_store+0x102/0x450
	 vprintk_emit+0x6f/0x1b0
	 dev_vprintk_emit+0x117/0x163
	 dev_printk_emit+0x51/0x6b
	 _dev_err+0x6e/0x88
	 cxl_parse_cfmws+0x2a0/0x2d
	 acpi_table_parse_entries_array+0x1fc/0x330
	 acpi_table_parse_cedt+0x4f/0x70
	 cxl_acpi_probe+0xd6/0x150
	 platform_probe+0x2f/0x60
	 really_probe+0x1f5/0x340
	 driver_probe_device+0x1e/0x80
	 __driver_attach+0xfc/0x190
	 bus_for_each_dev+0x76/0xb0
	 bus_add_driver+0x1bb/0x230
	 driver_register+0x85/0x120
	 do_one_initcall+0xbe/0x240
	 kernel_init_freeable+0x1cc/0x2d2
	 kernel_init+0x16/0x1a0
	 ret_from_fork+0x1f/0x30