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

[Dave Jiang <dave.jiang@intel.com>]

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);
+               return rc;
         }
+
+       rc = cxl_decoder_autoremove(dev, cxld);
+
         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);