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

[Cavitt, Jonathan]

-----Original Message-----
From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of Michal Wajdeczko
Sent: Thursday, July 17, 2025 11:48 AM
To: intel-xe at lists.freedesktop.org
Cc: Wajdeczko, Michal <Michal.Wajdeczko at intel.com>; De Marchi, Lucas <lucas.demarchi at intel.com>
Subject: [PATCH 2/5] drm/xe/configfs: Enforce canonical device names
> 
> 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);
>  	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)));

Does this function have an external-facing interface?  If so, I'm a bit worried this might
break some customer tooling.  It's probably not a big deal, as the tools should be passing
the canonical addresses anyways, but I'm unfortunately somewhat familiar with even
debug formatting changes breaking external tools, and they've causes endless
headaches in the past that would be good to avoid.

We could probably fix that by using "canonical" instead of "name" later in this function,
such that even the 'slightly broken' names can be parsed without introducing any new
rejection cases.

I'll leave that up to you to decide, though.

Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
-Jonathan Cavitt

> +	if (ret != 12 || strcmp(name, canonical))
> +		return ERR_PTR(-EINVAL);
> +
>  	pdev = pci_get_domain_bus_and_slot(domain, bus, PCI_DEVFN(slot, function));
>  	if (!pdev)
>  		return ERR_PTR(-ENODEV);
> -- 
> 2.47.1
> 
>