[PATCH v2] PCI: rework new_id interface for known vendor/device values

[PATCH v2] PCI: rework new_id interface for known vendor/device values

[Bandan Das]

While using the new_id interface, the user can unintentionally feed
incorrect values if the driver static table has a matching entry.
This is possible since only the device and vendor fields are
mandatory and the rest are optional. As a result, store_new_id
will fill in default values that are then passed on to the driver
and can have unintended consequences.

As an example, consider the ixgbe driver and the 82599EB network card :
echo "8086 10fb" > /sys/bus/pci/drivers/ixgbe/new_id

This will pass a driver_data value of 0 to the driver whereas
the index 0 in ixgbe actually points to a different set of card
operations.

This change returns an error if the user attempts to add a dynid for
a vendor/device combination for which a static entry already exists.
However, if the user intentionally wants a different set of values,
she must provide all the 7 fields and that will be accepted.

In KVM/device assignment scenario, the user might want 
to bind a device back to the host driver by writing to new_id
and trip on a possible null pointer dereference.

Signed-off-by: Bandan Das <bsd@redhat.com>
---
v2:
1. Return error if there is a matching static entry
and change commit message to reflect this behavior
3. Fill in a pdev and call pci_match_id instead of creating
a new matching function
4. Change commit message to reflect that libvirt does not
depend on this behavior

 drivers/pci/pci-driver.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 25f0bc6..493514e 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -103,11 +103,12 @@ store_new_id(struct device_driver *driver, const char *buf, size_t count)
 {
 	struct pci_driver *pdrv = to_pci_driver(driver);
 	const struct pci_device_id *ids = pdrv->id_table;
+	struct pci_dev *pdev;
 	__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;
+	int retval = 0;
 
 	fields = sscanf(buf, "%x %x %x %x %x %x %lx",
 			&vendor, &device, &subvendor, &subdevice,
@@ -115,6 +116,26 @@ store_new_id(struct device_driver *driver, const char *buf, size_t count)
 	if (fields < 2)
 		return -EINVAL;
 
+	if (fields != 7) {
+		pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
+		if (!pdev)
+			return -ENOMEM;
+
+		pdev->vendor = vendor;
+		pdev->device = device;
+		pdev->subsystem_vendor = subvendor;
+		pdev->subsystem_device = subdevice;
+		pdev->class = class;
+
+		if (pci_match_id(pdrv->id_table, pdev))
+			retval = -EEXIST;
+
+		kfree(pdev);
+
+		if (retval)
+			return retval;
+	}
+
 	/* Only accept driver_data values that match an existing id_table
 	   entry */
 	if (ids) {
-- 
1.8.3.1

Re: [PATCH v2] PCI: rework new_id interface for known vendor/device values

[Alex Williamson]

On Tue, 2014-04-01 at 17:22 -0400, Bandan Das wrote:
> While using the new_id interface, the user can unintentionally feed
> incorrect values if the driver static table has a matching entry.
> This is possible since only the device and vendor fields are
> mandatory and the rest are optional. As a result, store_new_id
> will fill in default values that are then passed on to the driver
> and can have unintended consequences.
> 
> As an example, consider the ixgbe driver and the 82599EB network card :
> echo "8086 10fb" > /sys/bus/pci/drivers/ixgbe/new_id
> 
> This will pass a driver_data value of 0 to the driver whereas
> the index 0 in ixgbe actually points to a different set of card
> operations.
> 
> This change returns an error if the user attempts to add a dynid for
> a vendor/device combination for which a static entry already exists.
> However, if the user intentionally wants a different set of values,
> she must provide all the 7 fields and that will be accepted.
> 
> In KVM/device assignment scenario, the user might want 
> to bind a device back to the host driver by writing to new_id
> and trip on a possible null pointer dereference.
> 
> Signed-off-by: Bandan Das <bsd@redhat.com>
> ---
> v2:
> 1. Return error if there is a matching static entry
> and change commit message to reflect this behavior
> 3. Fill in a pdev and call pci_match_id instead of creating
> a new matching function
> 4. Change commit message to reflect that libvirt does not
> depend on this behavior
> 
>  drivers/pci/pci-driver.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 25f0bc6..493514e 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -103,11 +103,12 @@ store_new_id(struct device_driver *driver, const char *buf, size_t count)
>  {
>  	struct pci_driver *pdrv = to_pci_driver(driver);
>  	const struct pci_device_id *ids = pdrv->id_table;
> +	struct pci_dev *pdev;
>  	__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;
> +	int retval = 0;
>  
>  	fields = sscanf(buf, "%x %x %x %x %x %x %lx",
>  			&vendor, &device, &subvendor, &subdevice,
> @@ -115,6 +116,26 @@ store_new_id(struct device_driver *driver, const char *buf, size_t count)
>  	if (fields < 2)
>  		return -EINVAL;
>  
> +	if (fields != 7) {

nit, pdev could be declared within this scope.

> +		pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
> +		if (!pdev)
> +			return -ENOMEM;
> +
> +		pdev->vendor = vendor;
> +		pdev->device = device;
> +		pdev->subsystem_vendor = subvendor;
> +		pdev->subsystem_device = subdevice;
> +		pdev->class = class;

Why not class_mask?

> +
> +		if (pci_match_id(pdrv->id_table, pdev))
> +			retval = -EEXIST;
> +
> +		kfree(pdev);
> +
> +		if (retval)
> +			return retval;
> +	}
> +
>  	/* Only accept driver_data values that match an existing id_table
>  	   entry */
>  	if (ids) {

Re: [PATCH v2] PCI: rework new_id interface for known vendor/device values

[Bandan Das]

Alex Williamson <alex.williamson@redhat.com> writes:

> On Tue, 2014-04-01 at 17:22 -0400, Bandan Das wrote:
>> While using the new_id interface, the user can unintentionally feed
>> incorrect values if the driver static table has a matching entry.
>> This is possible since only the device and vendor fields are
>> mandatory and the rest are optional. As a result, store_new_id
>> will fill in default values that are then passed on to the driver
>> and can have unintended consequences.
>> 
>> As an example, consider the ixgbe driver and the 82599EB network card :
>> echo "8086 10fb" > /sys/bus/pci/drivers/ixgbe/new_id
>> 
>> This will pass a driver_data value of 0 to the driver whereas
>> the index 0 in ixgbe actually points to a different set of card
>> operations.
>> 
>> This change returns an error if the user attempts to add a dynid for
>> a vendor/device combination for which a static entry already exists.
>> However, if the user intentionally wants a different set of values,
>> she must provide all the 7 fields and that will be accepted.
>> 
>> In KVM/device assignment scenario, the user might want 
>> to bind a device back to the host driver by writing to new_id
>> and trip on a possible null pointer dereference.
>> 
>> Signed-off-by: Bandan Das <bsd@redhat.com>
>> ---
>> v2:
>> 1. Return error if there is a matching static entry
>> and change commit message to reflect this behavior
>> 3. Fill in a pdev and call pci_match_id instead of creating
>> a new matching function
>> 4. Change commit message to reflect that libvirt does not
>> depend on this behavior
>> 
>>  drivers/pci/pci-driver.c | 23 ++++++++++++++++++++++-
>>  1 file changed, 22 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
>> index 25f0bc6..493514e 100644
>> --- a/drivers/pci/pci-driver.c
>> +++ b/drivers/pci/pci-driver.c
>> @@ -103,11 +103,12 @@ store_new_id(struct device_driver *driver, const char *buf, size_t count)
>>  {
>>  	struct pci_driver *pdrv = to_pci_driver(driver);
>>  	const struct pci_device_id *ids = pdrv->id_table;
>> +	struct pci_dev *pdev;
>>  	__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;
>> +	int retval = 0;
>>  
>>  	fields = sscanf(buf, "%x %x %x %x %x %x %lx",
>>  			&vendor, &device, &subvendor, &subdevice,
>> @@ -115,6 +116,26 @@ store_new_id(struct device_driver *driver, const char *buf, size_t count)
>>  	if (fields < 2)
>>  		return -EINVAL;
>>  
>> +	if (fields != 7) {
>
> nit, pdev could be declared within this scope.

Ok, will spin a new one.

>> +		pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
>> +		if (!pdev)
>> +			return -ENOMEM;
>> +
>> +		pdev->vendor = vendor;
>> +		pdev->device = device;
>> +		pdev->subsystem_vendor = subvendor;
>> +		pdev->subsystem_device = subdevice;
>> +		pdev->class = class;
>
> Why not class_mask?
pdev->class_mask isn't used anywhere.

>> +
>> +		if (pci_match_id(pdrv->id_table, pdev))
>> +			retval = -EEXIST;
>> +
>> +		kfree(pdev);
>> +
>> +		if (retval)
>> +			return retval;
>> +	}
>> +
>>  	/* Only accept driver_data values that match an existing id_table
>>  	   entry */
>>  	if (ids) {

Re: [PATCH v2] PCI: rework new_id interface for known vendor/device values

[Alex Williamson]

On Tue, 2014-04-01 at 18:00 -0400, Bandan Das wrote:
> Alex Williamson <alex.williamson@redhat.com> writes:
> 
> > On Tue, 2014-04-01 at 17:22 -0400, Bandan Das wrote:
> >> While using the new_id interface, the user can unintentionally feed
> >> incorrect values if the driver static table has a matching entry.
> >> This is possible since only the device and vendor fields are
> >> mandatory and the rest are optional. As a result, store_new_id
> >> will fill in default values that are then passed on to the driver
> >> and can have unintended consequences.
> >> 
> >> As an example, consider the ixgbe driver and the 82599EB network card :
> >> echo "8086 10fb" > /sys/bus/pci/drivers/ixgbe/new_id
> >> 
> >> This will pass a driver_data value of 0 to the driver whereas
> >> the index 0 in ixgbe actually points to a different set of card
> >> operations.
> >> 
> >> This change returns an error if the user attempts to add a dynid for
> >> a vendor/device combination for which a static entry already exists.
> >> However, if the user intentionally wants a different set of values,
> >> she must provide all the 7 fields and that will be accepted.
> >> 
> >> In KVM/device assignment scenario, the user might want 
> >> to bind a device back to the host driver by writing to new_id
> >> and trip on a possible null pointer dereference.
> >> 
> >> Signed-off-by: Bandan Das <bsd@redhat.com>
> >> ---
> >> v2:
> >> 1. Return error if there is a matching static entry
> >> and change commit message to reflect this behavior
> >> 3. Fill in a pdev and call pci_match_id instead of creating
> >> a new matching function
> >> 4. Change commit message to reflect that libvirt does not
> >> depend on this behavior
> >> 
> >>  drivers/pci/pci-driver.c | 23 ++++++++++++++++++++++-
> >>  1 file changed, 22 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> >> index 25f0bc6..493514e 100644
> >> --- a/drivers/pci/pci-driver.c
> >> +++ b/drivers/pci/pci-driver.c
> >> @@ -103,11 +103,12 @@ store_new_id(struct device_driver *driver, const char *buf, size_t count)
> >>  {
> >>  	struct pci_driver *pdrv = to_pci_driver(driver);
> >>  	const struct pci_device_id *ids = pdrv->id_table;
> >> +	struct pci_dev *pdev;
> >>  	__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;
> >> +	int retval = 0;
> >>  
> >>  	fields = sscanf(buf, "%x %x %x %x %x %x %lx",
> >>  			&vendor, &device, &subvendor, &subdevice,
> >> @@ -115,6 +116,26 @@ store_new_id(struct device_driver *driver, const char *buf, size_t count)
> >>  	if (fields < 2)
> >>  		return -EINVAL;
> >>  
> >> +	if (fields != 7) {
> >
> > nit, pdev could be declared within this scope.
> 
> Ok, will spin a new one.
> 
> >> +		pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
> >> +		if (!pdev)
> >> +			return -ENOMEM;
> >> +
> >> +		pdev->vendor = vendor;
> >> +		pdev->device = device;
> >> +		pdev->subsystem_vendor = subvendor;
> >> +		pdev->subsystem_device = subdevice;
> >> +		pdev->class = class;
> >
> > Why not class_mask?
> pdev->class_mask isn't used anywhere.

Ah, you're right.  Ok.  Thanks,

Alex

> >> +
> >> +		if (pci_match_id(pdrv->id_table, pdev))
> >> +			retval = -EEXIST;
> >> +
> >> +		kfree(pdev);
> >> +
> >> +		if (retval)
> >> +			return retval;
> >> +	}
> >> +
> >>  	/* Only accept driver_data values that match an existing id_table
> >>  	   entry */
> >>  	if (ids) {