Tags Tags

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

December 21st, 2011 - 12:00 am ET by Alex Williamson | Report spam
Help Create a new topic
v2: Update API documentation for each patch
v3: Incorporate Sasha's comments: kobject path, separate func, and CONFIG_SYSFS

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


Documentation/virtual/kvm/api.txt | 7 +++
virt/kvm/assigned-dev.c | 90 +++++++++++++++++++++++++++++++++-
2 files changed, 88 insertions(+), 9 deletions(-)
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
email Follow the discussionReplies 3 repliesReplies Make a reply

Replies

#1 Alex Williamson
December 21st, 2011 - 12:00 am ET | Report spam
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


Documentation/virtual/kvm/api.txt | 3 +++
virt/kvm/assigned-dev.c | 18 +++++++++
2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 7945b0b..ee2c96b 100644
a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -1151,6 +1151,9 @@ following flags are specified:
/* Depends on KVM_CAP_IOMMU */
#define KVM_DEV_ASSIGN_ENABLE_IOMMU (1 << 0)

+The KVM_DEV_ASSIGN_ENABLE_IOMMU flag is a mandatory option to ensure
+isolation of the device. Usages not specifying this flag are deprecated.
+
4.49 KVM_DEASSIGN_PCI_DEVICE

Capability: KVM_CAP_DEVICE_DEASSIGNMENT
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);


To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Similar topics

Make your own search :