[PATCH 0/2] kvm: Lock down device assignment

[PATCH 0/2] kvm: Lock down device assignment

[Alex Williamson]

Two patches to try to better secure the device assignment ioctl.
This firt patch makes KVM_DEV_ASSIGN_ENABLE_IOMMU a mandatory
option when assigning a device.  I don't believe we have any
users of this option, so I think we can skip any deprecation
period, especially since it's existence is rather dangerous.

The second patch introduces some file permission checking that Avi
suggested.  If a user has been granted read/write permission to
the PCI sysfs BAR resource files, this is a good indication that
they have access to the device.  We can't call sys_faccessat
directly (not exported), but the important bits are self contained
enough to include directly.  This still works with sudo and libvirt
usage, the latter already grants qemu permission to these files.
Thanks,

Alex

---

Alex Williamson (2):
      kvm: Device assignment permission checks
      kvm: Remove ability to assign a device without iommu support


 virt/kvm/assigned-dev.c |   73 +++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 63 insertions(+), 10 deletions(-)

[PATCH 1/2] kvm: Remove ability to assign a device without iommu support

[Alex Williamson]

This option has no users and it exposes a security hole that we
can allow devices to be assigned without iommu protection.  Make
KVM_DEV_ASSIGN_ENABLE_IOMMU a mandatory option.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 virt/kvm/assigned-dev.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
index 3ad0925..a251a28 100644
--- a/virt/kvm/assigned-dev.c
+++ b/virt/kvm/assigned-dev.c
@@ -487,6 +487,9 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
 	struct kvm_assigned_dev_kernel *match;
 	struct pci_dev *dev;
 
+	if (!(assigned_dev->flags & KVM_DEV_ASSIGN_ENABLE_IOMMU))
+		return -EINVAL;
+
 	mutex_lock(&kvm->lock);
 	idx = srcu_read_lock(&kvm->srcu);
 
@@ -544,16 +547,14 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
 
 	list_add(&match->list, &kvm->arch.assigned_dev_head);
 
-	if (assigned_dev->flags & KVM_DEV_ASSIGN_ENABLE_IOMMU) {
-		if (!kvm->arch.iommu_domain) {
-			r = kvm_iommu_map_guest(kvm);
-			if (r)
-				goto out_list_del;
-		}
-		r = kvm_assign_device(kvm, match);
+	if (!kvm->arch.iommu_domain) {
+		r = kvm_iommu_map_guest(kvm);
 		if (r)
 			goto out_list_del;
 	}
+	r = kvm_assign_device(kvm, match);
+	if (r)
+		goto out_list_del;
 
 out:
 	srcu_read_unlock(&kvm->srcu, idx);
@@ -593,8 +594,7 @@ static int kvm_vm_ioctl_deassign_device(struct kvm *kvm,
 		goto out;
 	}
 
-	if (match->flags & KVM_DEV_ASSIGN_ENABLE_IOMMU)
-		kvm_deassign_device(kvm, match);
+	kvm_deassign_device(kvm, match);
 
 	kvm_free_assigned_device(kvm, match);
 

Re: [PATCH 1/2] kvm: Remove ability to assign a device without iommu support

[Sasha Levin]

On Mon, 2011-12-19 at 20:19 -0700, Alex Williamson wrote:
> This option has no users and it exposes a security hole that we
> can allow devices to be assigned without iommu protection.  Make
> KVM_DEV_ASSIGN_ENABLE_IOMMU a mandatory option.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
>  virt/kvm/assigned-dev.c |   18 +++++++++---------
>  1 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> index 3ad0925..a251a28 100644
> --- a/virt/kvm/assigned-dev.c
> +++ b/virt/kvm/assigned-dev.c
> @@ -487,6 +487,9 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
>  	struct kvm_assigned_dev_kernel *match;
>  	struct pci_dev *dev;
>  
> +	if (!(assigned_dev->flags & KVM_DEV_ASSIGN_ENABLE_IOMMU))
> +		return -EINVAL;

Could we just drop KVM_DEV_ASSIGN_ENABLE_IOMMU and do it by default?
calling KVM_ASSIGN_PCI_DEVICE without that flag set it pretty
meaningless.

-- 

Sasha.

Re: [PATCH 1/2] kvm: Remove ability to assign a device without iommu support

[Jan Kiszka]

On 2011-12-20 09:49, Sasha Levin wrote:
> On Mon, 2011-12-19 at 20:19 -0700, Alex Williamson wrote:
>> This option has no users and it exposes a security hole that we
>> can allow devices to be assigned without iommu protection.  Make
>> KVM_DEV_ASSIGN_ENABLE_IOMMU a mandatory option.
>>
>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>> ---
>>
>>  virt/kvm/assigned-dev.c |   18 +++++++++---------
>>  1 files changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
>> index 3ad0925..a251a28 100644
>> --- a/virt/kvm/assigned-dev.c
>> +++ b/virt/kvm/assigned-dev.c
>> @@ -487,6 +487,9 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
>>  	struct kvm_assigned_dev_kernel *match;
>>  	struct pci_dev *dev;
>>  
>> +	if (!(assigned_dev->flags & KVM_DEV_ASSIGN_ENABLE_IOMMU))
>> +		return -EINVAL;
> 
> Could we just drop KVM_DEV_ASSIGN_ENABLE_IOMMU and do it by default?
> calling KVM_ASSIGN_PCI_DEVICE without that flag set it pretty
> meaningless.

There is that thing called "backward compatibility". :)

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH 1/2] kvm: Remove ability to assign a device without iommu support

[Sasha Levin]

On Tue, 2011-12-20 at 10:03 +0100, Jan Kiszka wrote:
> On 2011-12-20 09:49, Sasha Levin wrote:
> > On Mon, 2011-12-19 at 20:19 -0700, Alex Williamson wrote:
> >> This option has no users and it exposes a security hole that we
> >> can allow devices to be assigned without iommu protection.  Make
> >> KVM_DEV_ASSIGN_ENABLE_IOMMU a mandatory option.
> >>
> >> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> >> ---
> >>
> >>  virt/kvm/assigned-dev.c |   18 +++++++++---------
> >>  1 files changed, 9 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> >> index 3ad0925..a251a28 100644
> >> --- a/virt/kvm/assigned-dev.c
> >> +++ b/virt/kvm/assigned-dev.c
> >> @@ -487,6 +487,9 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
> >>  	struct kvm_assigned_dev_kernel *match;
> >>  	struct pci_dev *dev;
> >>  
> >> +	if (!(assigned_dev->flags & KVM_DEV_ASSIGN_ENABLE_IOMMU))
> >> +		return -EINVAL;
> > 
> > Could we just drop KVM_DEV_ASSIGN_ENABLE_IOMMU and do it by default?
> > calling KVM_ASSIGN_PCI_DEVICE without that flag set it pretty
> > meaningless.
> 
> There is that thing called "backward compatibility". :)

Well, Alex suggested skipping deprecation period because there are
currently no users of KVM_ASSIGN_PCI_DEVICE without
KVM_DEV_ASSIGN_ENABLE_IOMMU, so it should be fine to just make it the
default behavior, no?

We can leave KVM_DEV_ASSIGN_ENABLE_IOMMU itself so userspace won't
break, but theres no reason to enforce it being set in the kernel code.

-- 

Sasha.

Re: [PATCH 1/2] kvm: Remove ability to assign a device without iommu support

[Jan Kiszka]

On 2011-12-20 10:08, Sasha Levin wrote:
> On Tue, 2011-12-20 at 10:03 +0100, Jan Kiszka wrote:
>> On 2011-12-20 09:49, Sasha Levin wrote:
>>> On Mon, 2011-12-19 at 20:19 -0700, Alex Williamson wrote:
>>>> This option has no users and it exposes a security hole that we
>>>> can allow devices to be assigned without iommu protection.  Make
>>>> KVM_DEV_ASSIGN_ENABLE_IOMMU a mandatory option.
>>>>
>>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>>>> ---
>>>>
>>>>  virt/kvm/assigned-dev.c |   18 +++++++++---------
>>>>  1 files changed, 9 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
>>>> index 3ad0925..a251a28 100644
>>>> --- a/virt/kvm/assigned-dev.c
>>>> +++ b/virt/kvm/assigned-dev.c
>>>> @@ -487,6 +487,9 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
>>>>  	struct kvm_assigned_dev_kernel *match;
>>>>  	struct pci_dev *dev;
>>>>  
>>>> +	if (!(assigned_dev->flags & KVM_DEV_ASSIGN_ENABLE_IOMMU))
>>>> +		return -EINVAL;
>>>
>>> Could we just drop KVM_DEV_ASSIGN_ENABLE_IOMMU and do it by default?
>>> calling KVM_ASSIGN_PCI_DEVICE without that flag set it pretty
>>> meaningless.
>>
>> There is that thing called "backward compatibility". :)
> 
> Well, Alex suggested skipping deprecation period because there are
> currently no users of KVM_ASSIGN_PCI_DEVICE without
> KVM_DEV_ASSIGN_ENABLE_IOMMU, so it should be fine to just make it the
> default behavior, no?

This iommu-less mode used to "work" for older qemu-kvm version, and I
think it should still do. Though it makes no sense, I fully agree.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH 1/2] kvm: Remove ability to assign a device without iommu support

[Avi Kivity]

On 12/20/2011 11:12 AM, Jan Kiszka wrote:
> > 
> > Well, Alex suggested skipping deprecation period because there are
> > currently no users of KVM_ASSIGN_PCI_DEVICE without
> > KVM_DEV_ASSIGN_ENABLE_IOMMU, so it should be fine to just make it the
> > default behavior, no?
>
> This iommu-less mode used to "work" for older qemu-kvm version, and I
> think it should still do. Though it makes no sense, I fully agree.
>

It only worked for special kernels that allowed 1:1 gpa/hpa mappings, IIRC.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 1/2] kvm: Remove ability to assign a device without iommu support

[Alex Williamson]

On Tue, 2011-12-20 at 11:08 +0200, Sasha Levin wrote:
> On Tue, 2011-12-20 at 10:03 +0100, Jan Kiszka wrote:
> > On 2011-12-20 09:49, Sasha Levin wrote:
> > > On Mon, 2011-12-19 at 20:19 -0700, Alex Williamson wrote:
> > >> This option has no users and it exposes a security hole that we
> > >> can allow devices to be assigned without iommu protection.  Make
> > >> KVM_DEV_ASSIGN_ENABLE_IOMMU a mandatory option.
> > >>
> > >> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > >> ---
> > >>
> > >>  virt/kvm/assigned-dev.c |   18 +++++++++---------
> > >>  1 files changed, 9 insertions(+), 9 deletions(-)
> > >>
> > >> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> > >> index 3ad0925..a251a28 100644
> > >> --- a/virt/kvm/assigned-dev.c
> > >> +++ b/virt/kvm/assigned-dev.c
> > >> @@ -487,6 +487,9 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
> > >>  	struct kvm_assigned_dev_kernel *match;
> > >>  	struct pci_dev *dev;
> > >>  
> > >> +	if (!(assigned_dev->flags & KVM_DEV_ASSIGN_ENABLE_IOMMU))
> > >> +		return -EINVAL;
> > > 
> > > Could we just drop KVM_DEV_ASSIGN_ENABLE_IOMMU and do it by default?
> > > calling KVM_ASSIGN_PCI_DEVICE without that flag set it pretty
> > > meaningless.
> > 
> > There is that thing called "backward compatibility". :)
> 
> Well, Alex suggested skipping deprecation period because there are
> currently no users of KVM_ASSIGN_PCI_DEVICE without
> KVM_DEV_ASSIGN_ENABLE_IOMMU, so it should be fine to just make it the
> default behavior, no?
> 
> We can leave KVM_DEV_ASSIGN_ENABLE_IOMMU itself so userspace won't
> break, but theres no reason to enforce it being set in the kernel code.

As Jan said, the option does have historical meaning.  Ignoring the flag
adds ambiguity to the API, so I think it's best to make it clear which
path is no longer supported.  Either way, we don't get to take the flag
back, so it might as well serve as a sanity check.  Thanks,

Alex 

Re: [PATCH 1/2] kvm: Remove ability to assign a device without iommu support

[Avi Kivity]

On 12/20/2011 05:19 AM, Alex Williamson wrote:
> This option has no users and it exposes a security hole that we
> can allow devices to be assigned without iommu protection.  Make
> KVM_DEV_ASSIGN_ENABLE_IOMMU a mandatory option.
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>
>  virt/kvm/assigned-dev.c |   18 +++++++++---------
>
Documentation/virtual/kvm/api.txt +++

-- 
error compiling committee.c: too many arguments to function

[PATCH 2/2] kvm: Device assignment permission checks

[Alex Williamson]

Only allow KVM device assignment to attach to devices which:

 - Are not bridges
 - Have BAR resources (assume others are special devices)
 - The user has permissions to use

Assigning a bridge is a configuration error, it's not supported, and
typically doesn't result in the behavior the user is expecting anyway.
Devices without BAR resources are typically chipset components that
also don't have host drivers.  We don't want users to hold such devices
captive or cause system problems by fencing them off into an iommu
domain.  We determine "permission to use" by testing whether the user
has access to the PCI sysfs resource files.  By default a normal user
will not have access to these files, so it provides a good indication
that an administration agent has granted the user access to the device.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 virt/kvm/assigned-dev.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 54 insertions(+), 1 deletions(-)

diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
index a251a28..faec641 100644
--- a/virt/kvm/assigned-dev.c
+++ b/virt/kvm/assigned-dev.c
@@ -17,6 +17,7 @@
 #include <linux/pci.h>
 #include <linux/interrupt.h>
 #include <linux/slab.h>
+#include <linux/namei.h>
 #include "irq.h"
 
 static struct kvm_assigned_dev_kernel *kvm_find_assigned_dev(struct list_head *head,
@@ -483,9 +484,11 @@ out:
 static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
 				      struct kvm_assigned_pci_dev *assigned_dev)
 {
-	int r = 0, idx;
+	int r = 0, idx, i;
 	struct kvm_assigned_dev_kernel *match;
 	struct pci_dev *dev;
+	u8 header_type;
+	bool bar_found = false;
 
 	if (!(assigned_dev->flags & KVM_DEV_ASSIGN_ENABLE_IOMMU))
 		return -EINVAL;
@@ -516,6 +519,56 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
 		r = -EINVAL;
 		goto out_free;
 	}
+
+	/* Don't allow bridges to be assigned */
+	pci_read_config_byte(dev, PCI_HEADER_TYPE, &header_type);
+	if ((header_type & PCI_HEADER_TYPE) != PCI_HEADER_TYPE_NORMAL) {
+		r = -EPERM;
+		goto out_put;
+	}
+
+	/* We want to test whether the caller has been granted permissions to
+	 * use this device.  To be able to configure and control the device,
+	 * the user needs access to PCI configuration space and BAR resources.
+	 * These are accessed through PCI sysfs.  PCI config space is often
+	 * passed to the process calling this ioctl via file descriptor, so we
+	 * can't rely on access to that file.  We can check for permissions
+	 * on each of the BAR resource files, which is a pretty clear
+	 * indicator that the user has been granted access to the device. */
+	for (i = PCI_STD_RESOURCES; i <= PCI_STD_RESOURCE_END; i++) {
+		char buf[64];
+		struct path path;
+		struct inode *inode;
+
+		if (!pci_resource_len(dev, i))
+			continue;
+
+		/* Per sysfs-rules, sysfs is always at /sys */
+		snprintf(buf, sizeof(buf), "/sys/bus/pci/devices/%04x:%02x:"
+			 "%02x.%d/resource%d", pci_domain_nr(dev->bus),
+			 dev->bus->number, PCI_SLOT(dev->devfn),
+			 PCI_FUNC(dev->devfn), i);
+
+		r = kern_path(buf, LOOKUP_FOLLOW, &path);
+		if (r)
+			goto out_put;
+
+		inode = path.dentry->d_inode;
+
+		r = inode_permission(inode, MAY_READ | MAY_WRITE | MAY_ACCESS);
+		path_put(&path);
+		if (r)
+			goto out_put;
+
+		bar_found = true;
+	}
+
+	/* If no resources, probably something special */
+	if (!bar_found) {
+		r = -EPERM;
+		goto out_put;
+	}
+
 	if (pci_enable_device(dev)) {
 		printk(KERN_INFO "%s: Could not enable PCI device\n", __func__);
 		r = -EBUSY;

Re: [PATCH 0/2] kvm: Lock down device assignment

[Avi Kivity]

On 12/20/2011 05:19 AM, Alex Williamson wrote:
> Two patches to try to better secure the device assignment ioctl.
> This firt patch makes KVM_DEV_ASSIGN_ENABLE_IOMMU a mandatory
> option when assigning a device.  I don't believe we have any
> users of this option, so I think we can skip any deprecation
> period, especially since it's existence is rather dangerous.
>
> The second patch introduces some file permission checking that Avi
> suggested.  If a user has been granted read/write permission to
> the PCI sysfs BAR resource files, this is a good indication that
> they have access to the device.  We can't call sys_faccessat
> directly (not exported), but the important bits are self contained
> enough to include directly.  This still works with sudo and libvirt
> usage, the latter already grants qemu permission to these files.
> Thanks,
>
>

Looks good, but please update the API documentation.

-- 
error compiling committee.c: too many arguments to function