Skip to contentSkip to conversation

KVM: Fix PCI header check on device assignment

PATCH Applied Jan Kiszka 4 participants 8 messages Jun 5, 2012 – Jun 16, 2012 KVM

lore
Share & tools
lore

Jan Kiszka

author
#0 lore raw

PATCH 1/1 KVM: Fix PCI header check on device assignment

The masking was wrong (must have been 0x7f), and there is no need to re-read the value as pci_setup_device already does this for us.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=43339

Signed-off-by Jan Kiszka <[email protected]>
---
 virt/kvm/assigned-dev.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)
virt/kvm/assigned-dev.c +1 -3
index 01f572c..b1e091a 100644
@@ -635,7 +635,6 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
 	int r = 0, idx;
 	struct kvm_assigned_dev_kernel *match;
 	struct pci_dev *dev;
-	u8 header_type;
 
 	if (!(assigned_dev->flags & KVM_DEV_ASSIGN_ENABLE_IOMMU))
 		return -EINVAL;

@@ -668,8 +667,7 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
 	}
 
 	/* 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) {
+	if (dev->hdr_type != PCI_HEADER_TYPE_NORMAL) {
 		r = -EPERM;
 		goto out_put;
 	}
Signature · 1 line
1.7.3.4
Acked-by on v1 1/1
#1 lore raw
Quoted 2 lines · Jan Kiszka

The masking was wrong (must have been 0x7f), and there is no need to re-read the value as pci_setup_device already does this for us.

The intent was to mask off the multifunction bit from header_type, but the implementation is clearly wrong. hdr_type does both. Thanks

Quoted 2 lines

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=43339

Signed-off-by Jan Kiszka <[email protected]>
Acked-by Alex Williamson <[email protected]>
Quoted 26 lines
---
 virt/kvm/assigned-dev.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)
virt/kvm/assigned-dev.c +1 -3
index 01f572c..b1e091a 100644
@@ -635,7 +635,6 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
 	int r = 0, idx;
 	struct kvm_assigned_dev_kernel *match;
 	struct pci_dev *dev;
-	u8 header_type;
 
 	if (!(assigned_dev->flags & KVM_DEV_ASSIGN_ENABLE_IOMMU))
 		return -EINVAL;

@@ -668,8 +667,7 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
 	}
 
 	/* 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) {
+	if (dev->hdr_type != PCI_HEADER_TYPE_NORMAL) {
 		r = -EPERM;
 		goto out_put;
 	}
#2 lore raw
Quoted 7 lines · Alex Williamson
Quoted 2 lines · Jan Kiszka

The masking was wrong (must have been 0x7f), and there is no need to re-read the value as pci_setup_device already does this for us.

The intent was to mask off the multifunction bit from header_type, but the implementation is clearly wrong. hdr_type does both. Thanks

Quoted 6 lines
Quoted 2 lines

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=43339

Signed-off-by Jan Kiszka <[email protected]>
Acked-by Alex Williamson <[email protected]>

From your comment in the bugzilla entry I conclude that there is no need

to get this into 3.5. is this correct?

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#3 lore raw
Quoted 8 lines · Avi Kivity
Quoted 7 lines · Alex Williamson
Quoted 2 lines · Jan Kiszka

The masking was wrong (must have been 0x7f), and there is no need to re-read the value as pci_setup_device already does this for us.

The intent was to mask off the multifunction bit from header_type, but the implementation is clearly wrong. hdr_type does both. Thanks

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=43339

Signed-off-by Jan Kiszka <[email protected]>
Acked-by Alex Williamson <[email protected]>

From your comment in the bugzilla entry I conclude that there is no need to get this into 3.5. is this correct?

As I asses this (and I think Alex meant the same), this is not a critical fix or even a security issue, just a (so far broken) safety belt for users that are privileged anyway. Also, there were no valid devices accidentally excluded due to the bug.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
#4 lore raw
Quoted 14 lines · Jan Kiszka
Quoted 13 lines · Avi Kivity
Quoted 11 lines · Alex Williamson
Quoted 2 lines · Jan Kiszka

The masking was wrong (must have been 0x7f), and there is no need to re-read the value as pci_setup_device already does this for us.

The intent was to mask off the multifunction bit from header_type, but the implementation is clearly wrong. hdr_type does both. Thanks

Quoted 2 lines

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=43339

Signed-off-by Jan Kiszka <[email protected]>
Acked-by Alex Williamson <[email protected]>
Quoted 7 lines
Quoted 2 lines

From your comment in the bugzilla entry I conclude that there is no need to get this into 3.5. is this correct?

As I asses this (and I think Alex meant the same), this is not a critical fix or even a security issue, just a (so far broken) safety belt for users that are privileged anyway. Also, there were no valid devices accidentally excluded due to the bug.

Right. If a bridge does not have BARs (apparently what I tested on), it will get rejected from assignment because we assume devices without resources are special. If it does have BARs, some privileged entity needs to grant permissions to the resources, just like any other device. So while the existing code doesn't close the misconfiguration window we were trying for, it doesn't expose any kind of security issue. Thanks,

Alex

#5 lore raw
In reply to Jan Kiszka

Avi, Marcelo,

Can we get this in for 3.5 please? There's already an ack from me on the list. Thanks,

Alex

Quoted 31 lines · Jan Kiszka

The masking was wrong (must have been 0x7f), and there is no need to re-read the value as pci_setup_device already does this for us.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=43339

Signed-off-by Jan Kiszka <[email protected]>
---
 virt/kvm/assigned-dev.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)
virt/kvm/assigned-dev.c +1 -3
index 01f572c..b1e091a 100644
@@ -635,7 +635,6 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
 	int r = 0, idx;
 	struct kvm_assigned_dev_kernel *match;
 	struct pci_dev *dev;
-	u8 header_type;
 
 	if (!(assigned_dev->flags & KVM_DEV_ASSIGN_ENABLE_IOMMU))
 		return -EINVAL;

@@ -668,8 +667,7 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
 	}
 
 	/* 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) {
+	if (dev->hdr_type != PCI_HEADER_TYPE_NORMAL) {
 		r = -EPERM;
 		goto out_put;
 	}
#6 lore raw
Quoted 6 lines · Alex Williamson

Avi, Marcelo,

Can we get this in for 3.5 please? There's already an ack from me on the list. Thanks,

Alex

Can you bounce it to me please ?

Quoted 39 lines · Jan Kiszka
Quoted 22 lines

The masking was wrong (must have been 0x7f), and there is no need to re-read the value as pci_setup_device already does this for us.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=43339

Signed-off-by Jan Kiszka <[email protected]>
---
 virt/kvm/assigned-dev.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)
virt/kvm/assigned-dev.c +0 -1
index 01f572c..b1e091a 100644
@@ -635,7 +635,6 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
 	int r = 0, idx;
 	struct kvm_assigned_dev_kernel *match;
 	struct pci_dev *dev;
-	u8 header_type;
 
 	if (!(assigned_dev->flags & KVM_DEV_ASSIGN_ENABLE_IOMMU))
 		return -EINVAL;

virt/kvm/assigned-dev.c static int kvm_vm_ioctl_assign_device(struct kvm *kvm, · line 673

Quoted 9 lines
virt/kvm/assigned-dev.c +1 -2
@@ -668,8 +667,7 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
 	}
 
 	/* 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) {
+	if (dev->hdr_type != PCI_HEADER_TYPE_NORMAL) {
 		r = -EPERM;
 		goto out_put;
 	}

--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html