On Tue, 2014-04-01 at 18:00 -0400, Bandan Das wrote:
> Alex Williamson <alex.williamson@xxxxxxxxxx> 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@xxxxxxxxxx>
> >> ---
> >> 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) {
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
