DPDK patches and discussions
 help / color / mirror / Atom feed
From: Cornelia Huck <cohuck@redhat.com>
To: Alex Williamson <alex.williamson@redhat.com>
Cc: kvm@vger.kernel.org, linux-pci@vger.kernel.org,
	linux-kernel@vger.kernel.org, dev@dpdk.org, mtosatti@redhat.com,
	thomas@monjalon.net, bluca@debian.org, jerinjacobk@gmail.com,
	bruce.richardson@intel.com
Subject: Re: [dpdk-dev] [PATCH 3/7] vfio/pci: Introduce VF token
Date: Thu, 13 Feb 2020 12:46:54 +0100	[thread overview]
Message-ID: <20200213124654.76128d29.cohuck@redhat.com> (raw)
In-Reply-To: <158146234273.16827.10591457733122265139.stgit@gimli.home>

On Tue, 11 Feb 2020 16:05:42 -0700
Alex Williamson <alex.williamson@redhat.com> wrote:

> If we enable SR-IOV on a vfio-pci owned PF, the resulting VFs are not
> fully isolated from the PF.  The PF can always cause a denial of
> service to the VF, if not access data passed through the VF directly.
> This is why vfio-pci currently does not bind to PFs with SR-IOV enabled
> and does not provide access itself to enabling SR-IOV on a PF.  The
> IOMMU grouping mechanism might allow us a solution to this lack of
> isolation, however the deficiency isn't actually in the DMA path, so
> much as the potential cooperation between PF and VF devices.  Also,
> if we were to force VFs into the same IOMMU group as the PF, we severely
> limit the utility of having independent drivers managing PFs and VFs
> with vfio.
> 
> Therefore we introduce the concept of a VF token.  The token is
> implemented as a UUID and represents a shared secret which must be set
> by the PF driver and used by the VF drivers in order to access a vfio
> device file descriptor for the VF.  The ioctl to set the VF token will
> be provided in a later commit, this commit implements the underlying
> infrastructure.  The concept here is to augment the string the user
> passes to match a device within a group in order to retrieve access to
> the device descriptor.  For example, rather than passing only the PCI
> device name (ex. "0000:03:00.0") the user would also pass a vf_token
> UUID (ex. "2ab74924-c335-45f4-9b16-8569e5b08258").  The device match
> string therefore becomes:
> 
> "0000:03:00.0 vf_token=2ab74924-c335-45f4-9b16-8569e5b08258"
> 
> This syntax is expected to be extensible to future options as well, with
> the standard being:
> 
> "$DEVICE_NAME $OPTION1=$VALUE1 $OPTION2=$VALUE2"

Is this designed to be an AND condition? (I read it as such.)

> 
> The device name must be first and option=value pairs are separated by
> spaces.
> 
> The vf_token option is only required for VFs where the PF device is
> bound to vfio-pci.  There is no change for PFs using existing host
> drivers.
> 
> Note that in order to protect existing VF users, not only is it required
> to set a vf_token on the PF before VFs devices can be accessed, but also
> if there are existing VF users, (re)opening the PF device must also
> provide the current vf_token as authentication.  This is intended to
> prevent a VF driver starting with a trusted PF driver and later being
> replaced by an unknown driver.  A vf_token is not required to open the
> PF device when none of the VF devices are in use by vfio-pci drivers.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>  drivers/vfio/pci/vfio_pci.c         |  127 +++++++++++++++++++++++++++++++++++
>  drivers/vfio/pci/vfio_pci_private.h |    8 ++
>  2 files changed, 134 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 2ec6c31d0ab0..26aea9ac4863 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -466,6 +466,35 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev)
>  		vfio_pci_set_power_state(vdev, PCI_D3hot);
>  }
>  
> +static struct pci_driver vfio_pci_driver;
> +
> +static void vfio_pci_vf_token_user_add(struct vfio_pci_device *vdev, int val)

Suggestion: call this _user_modify(), and have _user_add() and
_user_remove() as wrappers. That said, ...

> +{
> +	struct pci_dev *physfn = pci_physfn(vdev->pdev);
> +	struct vfio_device *pf_dev;
> +	struct vfio_pci_device *pf_vdev;
> +
> +	if (!vdev->pdev->is_virtfn)
> +		return;
> +
> +	pf_dev = vfio_device_get_from_dev(&physfn->dev);
> +	if (!pf_dev)
> +		return;
> +
> +	if (pci_dev_driver(physfn) != &vfio_pci_driver) {
> +		vfio_device_put(pf_dev);
> +		return;
> +	}
> +
> +	pf_vdev = vfio_device_data(pf_dev);
> +
> +	mutex_lock(&pf_vdev->vf_token->lock);
> +	pf_vdev->vf_token->users += val;

...is this expected to always be >= 0? If yes, it looks like a bug if
this is called with val==-n for n > users.

> +	mutex_unlock(&pf_vdev->vf_token->lock);
> +
> +	vfio_device_put(pf_dev);
> +}
> +
>  static void vfio_pci_release(void *device_data)
>  {
>  	struct vfio_pci_device *vdev = device_data;
> @@ -475,6 +504,7 @@ static void vfio_pci_release(void *device_data)
>  	if (!(--vdev->refcnt)) {
>  		vfio_spapr_pci_eeh_release(vdev->pdev);
>  		vfio_pci_disable(vdev);
> +		vfio_pci_vf_token_user_add(vdev, -1);
>  	}
>  
>  	mutex_unlock(&vdev->reflck->lock);
> @@ -493,6 +523,7 @@ static int vfio_pci_open(void *device_data)
>  	mutex_lock(&vdev->reflck->lock);
>  
>  	if (!vdev->refcnt) {
> +		vfio_pci_vf_token_user_add(vdev, 1);
>  		ret = vfio_pci_enable(vdev);
>  		if (ret)
>  			goto error;
> @@ -1278,11 +1309,86 @@ static void vfio_pci_request(void *device_data, unsigned int count)
>  	mutex_unlock(&vdev->igate);
>  }
>  
> +#define VF_TOKEN_ARG "vf_token="
> +
> +/* Called with vdev->vf_token->lock */
> +static int vfio_pci_vf_token_match(struct vfio_pci_device *vdev, char *opts)
> +{
> +	char *token;
> +	uuid_t uuid;
> +	int ret;
> +
> +	if (!opts)
> +		return -EINVAL;
> +
> +	token = strstr(opts, VF_TOKEN_ARG);
> +	if (!token)
> +		return -EINVAL;
> +
> +	token += strlen(VF_TOKEN_ARG);
> +
> +	ret = uuid_parse(token, &uuid);
> +	if (ret)
> +		return ret;
> +
> +	if (!uuid_equal(&uuid, &vdev->vf_token->uuid))
> +		return -EACCES;
> +
> +	return 0;
> +}
> +
>  static int vfio_pci_match(void *device_data, char *buf)
>  {
>  	struct vfio_pci_device *vdev = device_data;
> +	char *opts;
> +
> +	opts = strchr(buf, ' ');
> +	if (opts) {
> +		*opts = 0;
> +		opts++;
> +	}
> +
> +	if (strcmp(pci_name(vdev->pdev), buf))
> +		return 0; /* No match */

Up to here, this function is fine; but below, it gets a bit hard to
follow...

> +
> +	if (vdev->pdev->is_virtfn) {
> +		struct pci_dev *physfn = pci_physfn(vdev->pdev);
> +		struct vfio_device *pf_dev;
> +		int ret = 0;
> +
> +		pf_dev = vfio_device_get_from_dev(&physfn->dev);
> +		if (pf_dev) {
> +			if (pci_dev_driver(physfn) == &vfio_pci_driver) {
> +				struct vfio_pci_device *pf_vdev =
> +						vfio_device_data(pf_dev);
> +
> +				mutex_lock(&pf_vdev->vf_token->lock);
> +				ret = vfio_pci_vf_token_match(pf_vdev, opts);
> +				mutex_unlock(&pf_vdev->vf_token->lock);
> +			}
> +
> +			vfio_device_put(pf_dev);
> +
> +			if (ret)
> +				return -EACCES;
> +		}
> +	}

If we are a VF, and the PF is bound to vfio, pass whatever stuff other
than the device name that was passed in to an opaque match function.

>  
> -	return !strcmp(pci_name(vdev->pdev), buf);
> +	if (vdev->vf_token) {
> +		int ret = 0;
> +
> +		mutex_lock(&vdev->vf_token->lock);
> +
> +		if (vdev->vf_token->users)
> +			ret = vfio_pci_vf_token_match(vdev, opts);
> +
> +		mutex_unlock(&vdev->vf_token->lock);
> +
> +		if (ret)
> +			return -EACCES;
> +	}

If we have a VF token with users, pass whatever stuff other than the
device name that was passed in to an opaque match function.

What about the following instead:

- parse the passed-in string into device name and key/value pairs
- maybe reject anything with an unknown key
- check the device name
- if we're a VF with the PF bound to vfio, require a VF token to be
  specified and pass it to a token match function
- if we have a VF token with users, require a VF token to be specified
  and pass it to a token match function

My main gripes with the current code are:
- key=value parsing is done in the match function for vf_token
- it looks hard to extend the list of supported key/value pairs
- I don't see a good way to find out (as the user) _why_ the VF isn't
  matching


> +
> +	return 1; /* Match */
>  }

(...)


  reply	other threads:[~2020-02-13 11:47 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-11 23:05 [dpdk-dev] [PATCH 0/7] vfio/pci: SR-IOV support Alex Williamson
2020-02-11 23:05 ` [dpdk-dev] [PATCH 1/7] vfio: Include optional device match in vfio_device_ops callbacks Alex Williamson
2020-02-13 10:31   ` Cornelia Huck
2020-02-11 23:05 ` [dpdk-dev] [PATCH 2/7] vfio/pci: Implement match ops Alex Williamson
2020-02-13 11:04   ` Cornelia Huck
2020-02-11 23:05 ` [dpdk-dev] [PATCH 3/7] vfio/pci: Introduce VF token Alex Williamson
2020-02-13 11:46   ` Cornelia Huck [this message]
2020-02-13 17:23     ` Alex Williamson
2020-02-13 18:35       ` Cornelia Huck
2020-02-14 23:40         ` Alex Williamson
2020-02-11 23:05 ` [dpdk-dev] [PATCH 4/7] vfio: Introduce VFIO_DEVICE_FEATURE ioctl and first user Alex Williamson
2020-02-13 12:41   ` Cornelia Huck
2020-02-13 17:39     ` Alex Williamson
2020-02-13 18:08       ` Cornelia Huck
2020-02-11 23:06 ` [dpdk-dev] [PATCH 5/7] vfio/pci: Add sriov_configure support Alex Williamson
2020-02-11 23:06 ` [dpdk-dev] [PATCH 6/7] vfio/pci: Remove dev_fmt definition Alex Williamson
2020-02-11 23:06 ` [dpdk-dev] [PATCH 7/7] vfio/pci: Cleanup .probe() exit paths Alex Williamson
2020-02-14  4:57 ` [dpdk-dev] [PATCH 0/7] vfio/pci: SR-IOV support Alexey Kardashevskiy
2020-02-14 15:27   ` Alex Williamson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200213124654.76128d29.cohuck@redhat.com \
    --to=cohuck@redhat.com \
    --cc=alex.williamson@redhat.com \
    --cc=bluca@debian.org \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=jerinjacobk@gmail.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=mtosatti@redhat.com \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).