Re: [PATCH/RFC] pci: dynids.use_driver_data considered harmful

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Sunday, August 17, 2008 12:06 pm Jean Delvare wrote:
> Hi all,
> From: Jean Delvare <khali@linux-fr.org>
> Subject: PCI: Check dynids driver_data value for validity
>
> Only accept dynids those driver_data value matches one of the driver's
> pci_driver_id entry. This prevents the user from accidentally passing
> values the drivers do not expect.
>
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: Milton Miller <miltonm@bga.com>
> Cc: Greg KH <greg@kroah.com>
> ---
>  Documentation/PCI/pci.txt       |    4 ++++
>  drivers/i2c/busses/i2c-amd756.c |    4 ----
>  drivers/i2c/busses/i2c-viapro.c |    4 ----
>  drivers/pci/pci-driver.c        |   18 ++++++++++++++++--
>  4 files changed, 20 insertions(+), 10 deletions(-)
>
> --- linux-2.6.27-rc3.orig/Documentation/PCI/pci.txt	2008-08-17
> 18:24:33.000000000 +0200 +++
> linux-2.6.27-rc3/Documentation/PCI/pci.txt	2008-08-17 18:24:38.000000000
> +0200 @@ -163,6 +163,10 @@ need pass only as many optional fields a
>  	o class and classmask fields default to 0
>  	o driver_data defaults to 0UL.
>
> +Note that driver_data must match the value used by any of the
> pci_device_id +entries defined in the driver. This makes the driver_data
> field mandatory +if all the pci_device_id entries have a non-zero
> driver_data value. +
>  Once added, the driver probe routine will be invoked for any unclaimed
>  PCI devices listed in its (newly updated) pci_ids list.
>
> --- linux-2.6.27-rc3.orig/drivers/i2c/busses/i2c-amd756.c	2008-08-17
> 17:15:57.000000000 +0200 +++
> linux-2.6.27-rc3/drivers/i2c/busses/i2c-amd756.c	2008-08-17
> 19:42:14.000000000 +0200 @@ -332,10 +332,6 @@ static int __devinit
> amd756_probe(struct
>  	int error;
>  	u8 temp;
>
> -	/* driver_data might come from user-space, so check it */
> -	if (id->driver_data >= ARRAY_SIZE(chipname))
> -		return -EINVAL;
> -
>  	if (amd756_ioport) {
>  		dev_err(&pdev->dev, "Only one device supported "
>  		       "(you have a strange motherboard, btw)\n");
> --- linux-2.6.27-rc3.orig/drivers/i2c/busses/i2c-viapro.c	2008-08-17
> 17:15:57.000000000 +0200 +++
> linux-2.6.27-rc3/drivers/i2c/busses/i2c-viapro.c	2008-08-17
> 19:42:24.000000000 +0200 @@ -320,10 +320,6 @@ static int __devinit
> vt596_probe(struct
>  	unsigned char temp;
>  	int error = -ENODEV;
>
> -	/* driver_data might come from user-space, so check it */
> -	if (id->driver_data & 1 || id->driver_data > 0xff)
> -		return -EINVAL;
> -
>  	/* Determine the address of the SMBus areas */
>  	if (force_addr) {
>  		vt596_smba = force_addr & 0xfff0;
> --- linux-2.6.27-rc3.orig/drivers/pci/pci-driver.c	2008-08-17
> 17:15:57.000000000 +0200 +++
> linux-2.6.27-rc3/drivers/pci/pci-driver.c	2008-08-17 19:17:55.000000000
> +0200 @@ -43,18 +43,32 @@ store_new_id(struct device_driver *drive
>  {
>  	struct pci_dynid *dynid;
>  	struct pci_driver *pdrv = to_pci_driver(driver);
> +	const struct pci_device_id *ids = pdrv->id_table;
>  	__u32 vendor, device, subvendor=PCI_ANY_ID,
>  		subdevice=PCI_ANY_ID, class=0, class_mask=0;
>  	unsigned long driver_data=0;
>  	int fields=0;
> -	int retval = 0;
> +	int retval;
>
> -	fields = sscanf(buf, "%x %x %x %x %x %x %lux",
> +	fields = sscanf(buf, "%x %x %x %x %x %x %lx",
>  			&vendor, &device, &subvendor, &subdevice,
>  			&class, &class_mask, &driver_data);
>  	if (fields < 2)
>  		return -EINVAL;
>
> +	/* Only accept driver_data values that match an existing id_table
> +	   entry */
> +	retval = -EINVAL;
> +	while (ids->vendor || ids->subvendor || ids->class_mask) {
> +		if (driver_data == ids->driver_data) {
> +			retval = 0;
> +			break;
> +		}
> +		ids++;
> +	}
> +	if (retval)	/* No match */
> +		return retval;
> +
>  	dynid = kzalloc(sizeof(*dynid), GFP_KERNEL);
>  	if (!dynid)
>  		return -ENOMEM;
>
>
> * * * * *
>
> The patch above applies on top of Milton's patch removing
> dynids.use_driver_data.

Looks good; I think we'll want to put this into linux-next along with Milton's 
change.  I'll push them out after a quick smoke test.

Thanks,
Jesse