[ltp] Re: Bug#565790: cat /proc/acpi/ibm/video and must press power button

[Henrique de Moraes Holschuh]

--EVF5PPMfhYS0aIcm
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Wed, 27 Jan 2010, jidanni@jidanni.org wrote:
> go reading /proc/acpi/ibm/video every day. But the curious user should
> have to resort to the power button if he steps on this landmine.

Sure.  I limited it to CAP_SYS_ADMIN now, which usually means root.

The patch will be sent for merging, but I can't promise it will be merged
before the next merge window for 2.6.34, and only after that will it be
a cantidade for a 2.6.32.y stable.

Patch attached for reference.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

--EVF5PPMfhYS0aIcm
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="lock-down-video-ctrl.patch"

commit 72772159f8105d2bfa8306ba079e0b2878b32e93
Author: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Date:   Sun Feb 7 20:03:24 2010 -0200

    thinkpad-acpi: lock down video output state access
    
    Given the right combination of ThinkPad and X.org, just reading the
    video output control state is enough to hard-crash X.org.
    
    Until the day I somehow find out a model or BIOS cut date to not
    provide this feature to ThinkPads that can do video switching through
    X RandR, change permissions so that only processes with CAP_SYS_ADMIN
    can access any sort of video output control state.
    
    Reported-by: Jidanni <jidanni@jidanni.org>
    Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>

diff --git a/Documentation/laptops/thinkpad-acpi.txt b/Documentation/laptops/thinkpad-acpi.txt
index 75afa12..39c0a09 100644
--- a/Documentation/laptops/thinkpad-acpi.txt
+++ b/Documentation/laptops/thinkpad-acpi.txt
@@ -650,6 +650,10 @@ LCD, CRT or DVI (if available). The following commands are available:
 	echo expand_toggle > /proc/acpi/ibm/video
 	echo video_switch > /proc/acpi/ibm/video
 
+NOTE: Access to this feature is restricted to processes owning the
+CAP_SYS_ADMIN capability for safety reasons, as it can interact badly
+enough with some versions of X.org to crash it.
+
 Each video output device can be enabled or disabled individually.
 Reading /proc/acpi/ibm/video shows the status of each device.
 
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 7e9cb67..dd7d286 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -319,9 +319,15 @@ config THINKPAD_ACPI_VIDEO
 	  server running, phase of the moon, and the current mood of
 	  Schroedinger's cat.  If you can use X.org's RandR to control
 	  your ThinkPad's video output ports instead of this feature,
-	  don't think twice: do it and say N here to save some memory.
+	  don't think twice: do it and say N here to save memory and avoid
+	  bad interactions with X.org.
 
-	  If you are not sure, say Y here.
+	  NOTE: access to this feature is limited to processes with the
+	  CAP_SYS_ADMIN capability, to avoid local DoS issues in platforms
+	  where it interacts badly with X.org.
+
+	  If you are not sure, say Y here but do try to check if you could
+	  be using X.org RandR instead.
 
 config THINKPAD_ACPI_HOTKEY_POLL
 	bool "Support NVRAM polling for hot keys"
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 0c40713..d12b61b 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -286,6 +286,7 @@ struct ibm_init_struct {
 	char param[32];
 
 	int (*init) (struct ibm_init_struct *);
+	mode_t base_procfs_mode;
 	struct ibm_struct *data;
 };
 
@@ -4625,6 +4626,10 @@ static int video_read(struct seq_file *m)
 		return 0;
 	}
 
+	/* Even reads can crash X.org, so... */
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
 	status = video_outputsw_get();
 	if (status < 0)
 		return status;
@@ -4658,6 +4663,10 @@ static int video_write(char *buf)
 	if (video_supported == TPACPI_VIDEO_NONE)
 		return -ENODEV;
 
+	/* Even reads can crash X.org, let alone writes... */
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
 	enable = 0;
 	disable = 0;
 
@@ -8483,9 +8492,10 @@ static int __init ibm_init(struct ibm_init_struct *iibm)
 		"%s installed\n", ibm->name);
 
 	if (ibm->read) {
-		mode_t mode;
+		mode_t mode = iibm->base_procfs_mode;
 
-		mode = S_IRUGO;
+		if (!mode)
+			mode = S_IRUGO;
 		if (ibm->write)
 			mode |= S_IWUSR;
 		entry = proc_create_data(ibm->name, mode, proc_dir,
@@ -8676,6 +8686,7 @@ static struct ibm_init_struct ibms_init[] __initdata = {
 #ifdef CONFIG_THINKPAD_ACPI_VIDEO
 	{
 		.init = video_init,
+		.base_procfs_mode = S_IRUSR,
 		.data = &video_driver_data,
 	},
 #endif

--EVF5PPMfhYS0aIcm--