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

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

On 7/7/23 18:07, Alison Schofield wrote:
> 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.

The put_device() can trigger the ->release() of the cxld->dev and 
therefore cxld may no longer be 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.

Yeah, should check here and return rc.

> 
> 
>>          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);