Re: [PATCH v4 6/9] agp/amd-k7: Don't rely on address of pci_device_id

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Storing kernel string pointers in driver_data prevents adding dynamic IDs via sysfs (new_id).
- [Low] Binding the driver via driver_override passes a NULL pointer to a %s format specifier.
--

--- Patch [6]: [PATCH v4 6/9] agp/amd-k7: Don't rely on address of pci_device_id ---
commit 6e944a8c8b19b7d71cd09452e813daa32931ee55
Author: Gary Guo <[email protected]>

agp/amd-k7: Don't rely on address of pci_device_id
    
Address of pci_device_id cannot be relied on due to presence of dynamic ID
and driver_override. Use driver_data instead.

> diff --git a/drivers/char/agp/amd-k7-agp.c b/drivers/char/agp/amd-k7-agp.c
> index 898ff30ffd46b..4d201e71c5177 100644
> --- a/drivers/char/agp/amd-k7-agp.c
> +++ b/drivers/char/agp/amd-k7-agp.c

[ ... ]

> @@ -387,37 +387,17 @@ static const struct agp_bridge_driver amd_irongate_driver = {
>  	.agp_type_to_mask_type  = agp_generic_type_to_mask_type,
>  };
>  
> -static struct agp_device_ids amd_agp_device_ids[] =
> -{
> -	{
> -		.device_id	= PCI_DEVICE_ID_AMD_FE_GATE_7006,
> -		.chipset_name	= "Irongate",
> -	},
> -	{
> -		.device_id	= PCI_DEVICE_ID_AMD_FE_GATE_700E,
> -		.chipset_name	= "761",
> -	},
> -	{
> -		.device_id	= PCI_DEVICE_ID_AMD_FE_GATE_700C,
> -		.chipset_name	= "760MP",
> -	},
> -	{ }, /* dummy final entry, always present */
> -};
> -
>  static int agp_amdk7_probe(struct pci_dev *pdev,
>  			   const struct pci_device_id *ent)
>  {
>  	struct agp_bridge_data *bridge;
>  	u8 cap_ptr;
> -	int j;
>  
>  	cap_ptr = pci_find_capability(pdev, PCI_CAP_ID_AGP);
>  	if (!cap_ptr)
>  		return -ENODEV;
>  
> -	j = ent - agp_amdk7_pci_table;
> -	dev_info(&pdev->dev, "AMD %s chipset\n",
> -		 amd_agp_device_ids[j].chipset_name);
> +	dev_info(&pdev->dev, "AMD %s chipset\n", (const char *)ent->driver_data);

[Severity: Low]
Does this pass a NULL pointer to a %s format specifier when the driver is bound
via driver_override?

When a device is bound using the sysfs driver_override interface, the PCI core
passes a dummy pci_device_id_any structure where driver_data is 0 (NULL). This
would result in an ungraceful log message like "AMD (null) chipset".

Should we add a fallback check for NULL and print a string like "Unknown"
instead?

>  
>  	bridge = agp_alloc_bridge();
>  	if (!bridge)

[ ... ]

> @@ -492,7 +472,6 @@ static int agp_amdk7_resume(struct device *dev)
>  	return amd_irongate_driver.configure();
>  }
>  
> -/* must be the same order as name table above */
>  static const struct pci_device_id agp_amdk7_pci_table[] = {
>  	{
>  	.class		= (PCI_CLASS_BRIDGE_HOST << 8),
> @@ -501,6 +480,7 @@ static const struct pci_device_id agp_amdk7_pci_table[] = {
>  	.device		= PCI_DEVICE_ID_AMD_FE_GATE_7006,
>  	.subvendor	= PCI_ANY_ID,
>  	.subdevice	= PCI_ANY_ID,
> +	.driver_data	= (kernel_ulong_t)"Irongate",

[Severity: Medium]
Will storing kernel memory addresses (string literals) in driver_data prevent
users from adding dynamic IDs via sysfs?

The PCI core's new_id_store validates the provided driver_data (which defaults
to 0) against the driver_data in the driver's static id_table. Since this patch
assigns string pointers to driver_data, users cannot supply a matching value
via the new_id sysfs interface, causing new_id_store to always reject the input
with -EINVAL.

Could the driver store an enum or integer index in driver_data and use it to
look up the string array instead?

>  	},
>  	{
>  	.class		= (PCI_CLASS_BRIDGE_HOST << 8),

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6