[PATCH] KVM: Fix PCI header check on device assignment

[PATCH] KVM: Fix PCI header check on device assignment

[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 <jan.kiszka@siemens.com>
---
 virt/kvm/assigned-dev.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
index 01f572c..b1e091a 100644
--- a/virt/kvm/assigned-dev.c
+++ b/virt/kvm/assigned-dev.c
@@ -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;
 	}
-- 
1.7.3.4

Re: [PATCH] KVM: Fix PCI header check on device assignment

[Alex Williamson]

On Tue, 2012-06-05 at 10:37 +0200, Jan Kiszka wrote:
> 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

> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=43339
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

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

> ---
>  virt/kvm/assigned-dev.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
> 
> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> index 01f572c..b1e091a 100644
> --- a/virt/kvm/assigned-dev.c
> +++ b/virt/kvm/assigned-dev.c
> @@ -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;
>  	}

Re: [PATCH] KVM: Fix PCI header check on device assignment

[Avi Kivity]

On 06/05/2012 08:13 PM, Alex Williamson wrote:
> On Tue, 2012-06-05 at 10:37 +0200, Jan Kiszka wrote:
>> 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
> 
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=43339
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Acked-by: Alex Williamson <alex.williamson@redhat.com>

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

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

Re: [PATCH] KVM: Fix PCI header check on device assignment

[Jan Kiszka]

On 2012-06-06 13:12, Avi Kivity wrote:
> On 06/05/2012 08:13 PM, Alex Williamson wrote:
>> On Tue, 2012-06-05 at 10:37 +0200, Jan Kiszka wrote:
>>> 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
>>
>>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=43339
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Acked-by: Alex Williamson <alex.williamson@redhat.com>
> 
> 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

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

Re: [PATCH] KVM: Fix PCI header check on device assignment

[Alex Williamson]

On Wed, 2012-06-06 at 13:18 +0200, Jan Kiszka wrote:
> On 2012-06-06 13:12, Avi Kivity wrote:
> > On 06/05/2012 08:13 PM, Alex Williamson wrote:
> >> On Tue, 2012-06-05 at 10:37 +0200, Jan Kiszka wrote:
> >>> 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
> >>
> >>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=43339
> >>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> Acked-by: Alex Williamson <alex.williamson@redhat.com>
> > 
> > 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

Re: [PATCH] KVM: Fix PCI header check on device assignment

[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

On Tue, 2012-06-05 at 10:37 +0200, Jan Kiszka wrote:
> 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 <jan.kiszka@siemens.com>
> ---
>  virt/kvm/assigned-dev.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
> 
> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> index 01f572c..b1e091a 100644
> --- a/virt/kvm/assigned-dev.c
> +++ b/virt/kvm/assigned-dev.c
> @@ -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;
>  	}

Re: [PATCH] KVM: Fix PCI header check on device assignment

[Marcelo Tosatti]

On Thu, Jun 14, 2012 at 01:48:28PM -0600, Alex Williamson wrote:
> 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 ?

> On Tue, 2012-06-05 at 10:37 +0200, Jan Kiszka wrote:
> > 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 <jan.kiszka@siemens.com>
> > ---
> >  virt/kvm/assigned-dev.c |    4 +---
> >  1 files changed, 1 insertions(+), 3 deletions(-)
> > 
> > diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> > index 01f572c..b1e091a 100644
> > --- a/virt/kvm/assigned-dev.c
> > +++ b/virt/kvm/assigned-dev.c
> > @@ -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;
> >  	}
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] KVM: Fix PCI header check on device assignment

[Marcelo Tosatti]

On Fri, Jun 15, 2012 at 07:56:20AM -0600, Jan Kiszka wrote:
> 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 <jan.kiszka@siemens.com>

Applied, thanks.