Regression - Xorg start failed

PATCH In review Dave Young 6 participants 14 messages Feb 13, 2011 – Feb 16, 2011 PCI

Share & tools

Dave Young

author

2011-02-13 06:22 UTC #0 lore raw

With kernel built from current linus's tree, I can not start xorg, it failed with:

Backtrace:
0: X(xorg_backtrace+0x26) [0x4e8bb6]
1: X(xf86SigHandler+0x39) [0x489989]

2: /lib64/libc.so.6 [0x7f077a6b9f30]
3: /usr/lib64/libpciaccess.so.0(pci_device_get_bridge_buses+0xf1) [0x7f077c101fd1] 4: X(initPciBusState+0x8d) [0x470abd]
5: X(xf86AccessInit+0xe) [0x48e26e]
6: X(InitOutput+0x108b) [0x466bfb]
7: X(main+0x20e) [0x42ceee]
8: /lib64/libc.so.6(__libc_start_main+0xe6) [0x7f077a6a5526]
9: X [0x42c529]

Fatal server error:
Caught signal 11. Server aborting

The graphic driver I used is intel (i915)

Finally I bisected it, results:
47970b1b2aa64464bc0a9543e86361a622ae7c03 is first bad commit commit 47970b1b2aa64464bc0a9543e86361a622ae7c03

Author: Chris Wright <[email protected]>
Date:   Thu Feb 10 15:58:56 2011 -0800

pci: use security_capable() when checking capablities during config space read

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 <[email protected]>

Signed-off-by Chris Wright <[email protected]>

Signed-off-by James Morris <[email protected]>

:040000 040000 e14ad9470ca5f84f13cd31eaf5def16d40bc54f1 cbf987647b8052214dd928c87c6becfb639e4ca2 M drivers

Any idea?

--
Thanks
Dave

Dave Airlie

2011-02-13 07:53 UTC #1 lore raw

Quoted 4 lines · Dave Young

Hi
With kernel built from current linus's tree, I can not start xorg, it failed with:

Me too!,

Thanks for bisceting this, I just tried -rc4 on my Fedora 13 laptop and wasn't looking forward to bisecting it.

If this isn't hitting later distros its probably an updated libpciaccess that fixed something.

libpciaccess-0.10.9-2.20091209.fc13.i686 is what is on this box.

Probably should revert first, then work out what is crapping out libpciaccess.

Dave.

Linus Torvalds

2011-02-13 15:50 UTC #2 lore raw

Quoted 2 lines · Dave Airlie

Probably should revert first, then work out what is crapping out libpciaccess.

Yeah, I'll revert. The patch is one of those "obviously a good idea, but in practice it's not something we can change now".

Linus

Chris Wright

2011-02-14 00:35 UTC #3 lore raw

Quoted 5 lines · * Linus Torvalds ([email protected]) wrote:

Quoted 1 line · Dave Airlie
Probably should revert first, then work out what is crapping out libpciaccess.
Yeah, I'll revert. The patch is one of those "obviously a good idea, but in practice it's not something we can change now".

Turns out I'm just a bona fide idiot.

I was not testing the right kernel _and_ didn't get the logic right.

sorry for the screw up,
-chris
---

Subject: [PATCH] pci: use security_capable correctly during config space read

Commit 47970b1 ("pci: use security_capable() when checking capablities during config space read") is just plain broken. The normal capable() interface returns true on success, but the LSM interface returns 0 on success.

Signed-off-by Chris Wright <[email protected]>

---

I've tested this quickly (lspci behaviour is as expected).

drivers/pci/pci-sysfs.c +1 -1

index f7771f3..ea25e5b 100644
@@ -369,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 (security_capable(filp->f_cred, 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;

James Morris

Acked-by

2011-02-14 01:04 UTC #4 lore raw

Quoted 8 lines · Chris Wright

Subject: [PATCH] pci: use security_capable correctly during config space read
Commit 47970b1 ("pci: use security_capable() when checking capablities during config space read") is just plain broken. The normal capable() interface returns true on success, but the LSM interface returns 0 on success.
Signed-off-by Chris Wright <[email protected]>

Sorry, I should have caught this.

Acked-by James Morris <[email protected]>

Quoted 19 lines

---

I've tested this quickly (lspci behaviour is as expected).

drivers/pci/pci-sysfs.c +1 -1

index f7771f3..ea25e5b 100644
@@ -369,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 (security_capable(filp->f_cred, 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;

Signature · 2 lines

James Morris
<[email protected]>

Chris Wright

2011-02-15 01:21 UTC #5 lore raw

PATCH v3 1/1 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 <[email protected]>

Tested-by Dave Young <[email protected]>

Acked-by James Morris <[email protected]>

Cc Dave Airlie <[email protected]>

Cc Alex Riesen <[email protected]>

Cc Sedat Dilek <[email protected]>

Cc Linus Torvalds <[email protected]>

Signed-off-by Chris Wright <[email protected]>

---
v2: added Reported-by Eric
v3: fix logic screw up

 drivers/pci/pci-sysfs.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

drivers/pci/pci-sysfs.c +2 -1

index 8ecaac9..ea25e5b 100644
@@ -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;

Signature · 1 line

1.7.3.4

James Morris

2011-02-15 09:39 UTC #6 lore raw

Re: [PATCH v3] pci: use security_capable() when checking capablities during config space read

Also pullable from here:

The following changes since commit 795abaf1e4e188c4171e3cd3dbb11a9fcacaf505:
David Miller (1):
klist: Fix object alignment on 64-bit.

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6 for-linus

Chris Wright (1):
pci: use security_capable() when checking capablities during config space read

 drivers/pci/pci-sysfs.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

Signature · 2 lines

James Morris
<[email protected]>

Alex Riesen

2011-02-16 06:24 UTC #7 lore raw

Re: [PATCH v3] pci: use security_capable() when checking capablities during config space read

In reply to Chris Wright

Quoted 13 lines · Chris Wright

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.

Quoted 8 lines

Reported-by Eric Paris <[email protected]>
Tested-by Dave Young <[email protected]>
Acked-by James Morris <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Alex Riesen <[email protected]>
Cc Sedat Dilek <[email protected]>
Cc Linus Torvalds <[email protected]>
Signed-off-by Chris Wright <[email protected]>

FWIW, I confirm the fix.
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Dave Young

author

2011-02-14 05:38 UTC #8 lore raw

In reply to Chris Wright

Quoted 15 lines · Chris Wright

Quoted 5 lines · * Linus Torvalds ([email protected]) wrote:
Quoted 1 line · Dave Airlie
Probably should revert first, then work out what is crapping out libpciaccess.
Yeah, I'll revert. The patch is one of those "obviously a good idea, but in practice it's not something we can change now".
Turns out I'm just a bona fide idiot.
I was not testing the right kernel _and_ didn't get the logic right.
sorry for the screw up,
-chris
---

Quoted 6 lines

Subject: [PATCH] pci: use security_capable correctly during config space read
Commit 47970b1 ("pci: use security_capable() when checking capablities during config space read") is just plain broken. The normal capable() interface returns true on success, but the LSM interface returns 0 on success.

Chris, linus has reverted the original commit, so this does not apply.

Anyway I have tested this one, it works well. Feel free add my Tested-by line.

Quoted 21 lines

Signed-off-by Chris Wright <[email protected]>

---

I've tested this quickly (lspci behaviour is as expected).

drivers/pci/pci-sysfs.c +1 -1

index f7771f3..ea25e5b 100644
@@ -369,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 (security_capable(filp->f_cred, 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;

Chris Wright

2011-02-14 16:56 UTC #9 lore raw

Quoted 7 lines · * Dave Young ([email protected]) wrote:

Quoted 4 lines · Chris Wright
Commit 47970b1 ("pci: use security_capable() when checking capablities during config space read") is just plain broken. The normal capable() interface returns true on success, but the LSM interface returns 0 on success.
Chris, linus has reverted the original commit, so this does not apply.

Right, I'll send a new patch to James.

Quoted 1 line

Anyway I have tested this one, it works well. Feel free add my Tested-by line.

Thanks for confirming the fix Dave.

thanks,
-chris

Chris Wright

2011-02-13 23:31 UTC #10 lore raw

In reply to Dave Airlie

Quoted 9 lines · * Dave Airlie ([email protected]) wrote:

Quoted 2 lines · Dave Young
With kernel built from current linus's tree, I can not start xorg, it failed with:
Me too!,
Thanks for bisceting this, I just tried -rc4 on my Fedora 13 laptop and wasn't looking forward to bisecting it.

Quoted 6 lines

If this isn't hitting later distros its probably an updated libpciaccess that fixed something.
libpciaccess-0.10.9-2.20091209.fc13.i686 is what is on this box.
Probably should revert first, then work out what is crapping out libpciaccess.

Are you running with SELinux enabled, if so can you paste the AVC error? It's working for me here.

thanks,
-chris

Dave Airlie

2011-02-13 23:34 UTC #11 lore raw

Quoted 13 lines · Chris Wright

Quoted 12 lines · * Dave Airlie ([email protected]) wrote:
Quoted 2 lines · Dave Young
With kernel built from current linus's tree, I can not start xorg, it failed with:
Me too!,
Thanks for bisceting this, I just tried -rc4 on my Fedora 13 laptop and wasn't looking forward to bisecting it.
If this isn't hitting later distros its probably an updated libpciaccess that fixed something.

Quoted 6 lines

Quoted 3 lines
libpciaccess-0.10.9-2.20091209.fc13.i686 is what is on this box.
Probably should revert first, then work out what is crapping out libpciaccess.
Are you running with SELinux enabled, if so can you paste the AVC error? It's working for me here.

Yes most likely, I'm not at the machine now but I'll see if I can the AVC off it later.

On Fedora 13?

F14/rawhide seem fine.

Dave.

Chris Wright

2011-02-13 23:35 UTC #12 lore raw

Quoted 2 lines · * Dave Airlie ([email protected]) wrote:

Yes most likely, I'm not at the machine now but I'll see if I can the AVC off it later.

Great, thanks.

Quoted 3 lines

On Fedora 13?
F14/rawhide seem fine.

I'm on F14.

Alex Riesen

2011-02-13 08:15 UTC #13 lore raw

In reply to Dave Young

Quoted 14 lines · Dave Young

Finally I bisected it, results:
47970b1b2aa64464bc0a9543e86361a622ae7c03 is first bad commit commit 47970b1b2aa64464bc0a9543e86361a622ae7c03
Author: Chris Wright <[email protected]>
Date: Thu Feb 10 15:58:56 2011 -0800
pci: use security_capable() when checking capablities during config space read
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.

Quoted 4 lines

Reported-by Eric Paris <[email protected]>
Signed-off-by Chris Wright <[email protected]>
Signed-off-by James Morris <[email protected]>

Actually, even reading the PCI capabilities fails with lspci reporting "Capabilities: <access denied>" if run as root. "libpciaccess" should have handled this situation, but still it looks like a regression and it breaks existing systems.
_______________________________________________
dri-devel mailing list [email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel