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

[Lucas De Marchi]

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.

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?

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
>