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

[Bjorn Helgaas <bhelgaas@google.com>]

On Fri, Apr 25, 2014 at 01:51:50PM -0400, Bandan Das wrote:
> Bjorn Helgaas <bhelgaas@google.com> writes:
> 
> > On Tue, Apr 01, 2014 at 09:32:59PM -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.
> >
> > I don't understand this last KVM comment.  If this patch fixes a null
> > pointer dereference, it must be because we return -EEXIST instead of
> > calling the driver's probe method.
> 
> A null pointer dereference in the ixgbe driver's struct ixgbe_info
> that points to operations for a card model. In this case, when the user 
> uses the new_id interface (without specifying driver_data), it defaults
> to 0. So, ixgbe_info points to ixgbe_82598_info with mac_ops set to 
> mac_ops_82598 while the card in question is a 82599.
> 
> > Can you outline the sequence of events and the drivers involved?  Did we
> 
> Something like this is enough to trigger this -
> echo "b:f:d" > /sys/bus/.../driver/unbind
> echo "b:f:d" > /sys/bus/pci/drives/ixgbe/new_id
> echo 16 > /sys/bus/pci/devices/b:f:d/sriov_numvfs
> 
> > start with a device that was claimed by vfio, and now we're trying to get
> > ixgbe to claim it by writing to /sys/bus/pci/drivers/ixgbe/new_id?  If so,
> > does that mean the user has to know what driver_data value to supply?
> 
> Yes, but isn't it better than defaulting to 0 ?
> 
> > I know you didn't add the new_id mechanism, and this patch makes it safer
> > than it was before, but I'm uneasy about it in general.  Most drivers do
> > not validate the driver_data value.  They assume it came out of the
> > id_table supplied by the driver and is therefore trustworthy.  But new_id
> > is a loophole that allows a user (hopefully only root) to pass arbitrary
> > junk to the driver.
> 
> I think this is what this patch does. If the user intends to, let her
> pass arbitrary junk, let's not assume values on behalf of the user.

Yep, I agree, I was just trying to figure out if there was something
specific to KVM here.  But I don't think there is.

Bjorn