[PATCH 2/5] drm/xe/configfs: Enforce canonical device names

[Michal Wajdeczko]

On 17.07.2025 21:43, Lucas De Marchi wrote:
> On Thu, Jul 17, 2025 at 08:48:22PM +0200, Michal Wajdeczko wrote:
>> While we expect config directory names to match PCI device name,
>> currently we are only scanning provided names for domain, bus,
>> device and function numbers, without checking their format.
>> This would pass slightly broken entries like:
>>
>>  /sys/kernel/config/xe/
>>  ├── 0000:00:02.0000000000000
>>  │   └── ...
>>  ├── 0000:00:02.0x
>>  │   └── ...
>>  ├──  0: 0: 2. 0
>>  │   └── ...
>>  └── 0:0:2.0
>>      └── ...
>>
>> To avoid such mistakes, check if the name provided exactly matches
>> the canonical PCI device address format, which we recreated from
>> the parsed BDF data. Also simplify scanf format as it can't really
>> catch all formatting errors.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>> drivers/gpu/drm/xe/xe_configfs.c | 9 ++++++++-
>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_configfs.c b/drivers/gpu/drm/xe/
>> xe_configfs.c
>> index e9b46a2d0019..90b4fe92a611 100644
>> --- a/drivers/gpu/drm/xe/xe_configfs.c
>> +++ b/drivers/gpu/drm/xe/xe_configfs.c
>> @@ -259,12 +259,19 @@ static struct config_group
>> *xe_config_make_device_group(struct config_group *gro
>>     unsigned int domain, bus, slot, function;
>>     struct xe_config_device *dev;
>>     struct pci_dev *pdev;
>> +    char canonical[16];
>>     int ret;
>>
>> -    ret = sscanf(name, "%04x:%02x:%02x.%x", &domain, &bus, &slot,
>> &function);
>> +    ret = sscanf(name, "%x:%x:%x.%d", &domain, &bus, &slot, &function);
> 
>                      ^
> 
> This should still be hex. It may work "better" because we have up to 8
> functions.
> 
> .9 and .a are equally bad, we don't need to fail one here and one below.

below is the next level of verification, that also includes check for
slot to be up to 1F (31) instead of FF that scanf would allow, so IMO
it's ok to fail one case here and other later

but since it looks that vsscanf expects signed int pointer for "%d" and
we have 'function' defined as unsigned int, so it must be "%x" - you win

> 
> Also grepping around, it's always %x that is used. Exxample:
> 
> drivers/pci/pci.c:      ret = sscanf(wpath, "%x:%x:%x.%x%c", &seg, &bus,
> &slot,
> drivers/pci/vgaarb.c:   n = sscanf(buf, "PCI:%x:%x:%x.%x", domain, bus,
> &slot, &func);
> 
> 
>>     if (ret != 4)
>>         return ERR_PTR(-EINVAL);
>>
>> +    ret = scnprintf(canonical, sizeof(canonical), "%04x:%02x:%02x.
>> %d", domain, bus,
>> +            PCI_SLOT(PCI_DEVFN(slot, function)),
>> +            PCI_FUNC(PCI_DEVFN(slot, function)));
>> +    if (ret != 12 || strcmp(name, canonical))
>> +        return ERR_PTR(-EINVAL);
> 
> do we really need this? Wouldn't it be sufficient to just fail with
> ENODEV in the line below?

with EINVAL we are saying that name format is wrong, like in this case:

	"0:0:2.0"

while ENODEV indicates that format is ok, but device is not present or
not supported, like:

	"0000:00:01.1"

> 
> Lucas De Marchi
> 
>> +
>>     pdev = pci_get_domain_bus_and_slot(domain, bus, PCI_DEVFN(slot,
>> function));
>>     if (!pdev)
>>         return ERR_PTR(-ENODEV);
>> -- 
>> 2.47.1
>>