Re: KVM Test report, kernel a685b38... qemu 671d89d...

[Chris Wright <chrisw@sous-sol.org>]

* Alex Williamson (alex.williamson@redhat.com) wrote:
> On Wed, 2011-02-16 at 11:10 +0200, Avi Kivity wrote:
> > On 02/16/2011 11:05 AM, Hao, Xudong wrote:
> > > Hi, all,
> > > This is KVM test result against kvm.git a685b38e272587e644fedd37269ddb82df21c052, and qemu-kvm.git 671d89d6411655bb4f8058ce6eb86bb0bb8ec978.
> > >
> > > Currently qemu-kvm can build successfully on RHEL5, and Qcow image create failure issue also got fixed, our nightly testing resumed. One VT-d device assignment issue opened on latest KVM.
> > >
> > > New issue:
> > > 1. [VT-d] VT-d device passthrough fail to guest
> > > https://bugzilla.kernel.org/show_bug.cgi?id=29232
> > >
> 
> Extremely reproducible.  Looks like it's a result of this kernel change:
> 
> commit 47970b1b2aa64464bc0a9543e86361a622ae7c03
> Author: Chris Wright <chrisw@sous-sol.org>
> Date:   Thu Feb 10 15:58:56 2011 -0800
> 
>     pci: use security_capable() when checking capablities during config space re
>     
>     Eric Paris noted that commit de139a3 ("pci: check caps from sysfs file
>     open to read device dependent config space") caused the capability check
>     to bypass security modules and potentially auditing.  Rectify this by
>     calling security_capable() when checking the open file's capabilities
>     for config space reads.
>     
>     Reported-by: Eric Paris <eparis@redhat.com>
>     Signed-off-by: Chris Wright <chrisw@sous-sol.org>
>     Signed-off-by: James Morris <jmorris@namei.org>
> 
> Chris, why isn't this working for us?  Thanks,

It's a broken patch, the fix is floating about.  Linus reverted it and I
supplied this patch after the revert:


>From 683034fca7b8c6666322f87b8b4f664f1ae0b5fc Mon Sep 17 00:00:00 2001
From: Chris Wright <chrisw@sous-sol.org>
Date: Mon, 14 Feb 2011 19:12:00 -0500
Subject: [PATCH] pci: use security_capable() when checking capablities during config space read

This reintroduces commit 47970b1b which was subsequently reverted
as f00eaeea.  The original change was broken and caused X startup
failures and generally made privileged processes incapable of reading
device dependent config space.  The normal capable() interface returns
true on success, but the LSM interface returns 0 on success.  This thinko
is now fixed in this patch, and has been confirmed to work properly.

So, once again...Eric Paris noted that commit de139a3 ("pci: check caps
from sysfs file open to read device dependent config space") caused the
capability check to bypass security modules and potentially auditing.
Rectify this by calling security_capable() when checking the open file's
capabilities for config space reads.

Reported-by: Eric Paris <eparis@redhat.com>
Tested-by: Dave Young <hidave.darkstar@gmail.com>
Acked-by: James Morris <jmorris@namei.org>
Cc: Dave Airlie <airlied@gmail.com>
Cc: Alex Riesen <raa.lkml@gmail.com>
Cc: Sedat Dilek <sedat.dilek@googlemail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Chris Wright <chrisw@sous-sol.org>
---
 drivers/pci/pci-sysfs.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 8ecaac9..ea25e5b 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -23,6 +23,7 @@
 #include <linux/mm.h>
 #include <linux/fs.h>
 #include <linux/capability.h>
+#include <linux/security.h>
 #include <linux/pci-aspm.h>
 #include <linux/slab.h>
 #include "pci.h"
@@ -368,7 +369,7 @@ pci_read_config(struct file *filp, struct kobject *kobj,
 	u8 *data = (u8*) buf;
 
 	/* Several chips lock up trying to read undefined config space */
-	if (cap_raised(filp->f_cred->cap_effective, CAP_SYS_ADMIN)) {
+	if (security_capable(filp->f_cred, CAP_SYS_ADMIN) == 0) {
 		size = dev->cfg_size;
 	} else if (dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) {
 		size = 128;
-- 
1.7.3.4