[PATCH 1/3] acpi ec: Cleanup unused stuff

[PATCH 1/3] acpi ec: Cleanup unused stuff

[Thomas Renninger]

static void acpi_ec_gpe_query(void *ec_cxt);
-> The function is right above this declaration -> not needed.

poll_force is also not used, cleaned up in ec.c and its users:
compal-laptop and msi-laptop.

Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: lenb@kernel.org
CC: linux-acpi@vger.kernel.org
---
 drivers/acpi/ec.c                    |    6 +-----
 drivers/platform/x86/compal-laptop.c |   12 ++++++------
 drivers/platform/x86/msi-laptop.c    |   12 ++++++------
 include/linux/acpi.h                 |    3 +--
 4 files changed, 14 insertions(+), 19 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index fa848c4..b3f1d6f 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -69,7 +69,6 @@ enum ec_command {
 
 #define ACPI_EC_DELAY		500	/* Wait 500ms max. during EC ops */
 #define ACPI_EC_UDELAY_GLK	1000	/* Wait 1ms max. to get global lock */
-#define ACPI_EC_CDELAY		10	/* Wait 10us before polling EC */
 #define ACPI_EC_MSI_UDELAY	550	/* Wait 550us for MSI EC */
 
 #define ACPI_EC_STORM_THRESHOLD 8	/* number of false interrupts
@@ -433,8 +432,7 @@ EXPORT_SYMBOL(ec_write);
 
 int ec_transaction(u8 command,
 		   const u8 * wdata, unsigned wdata_len,
-		   u8 * rdata, unsigned rdata_len,
-		   int force_poll)
+		   u8 * rdata, unsigned rdata_len)
 {
 	struct transaction t = {.command = command,
 				.wdata = wdata, .rdata = rdata,
@@ -592,8 +590,6 @@ static void acpi_ec_gpe_query(void *ec_cxt)
 	mutex_unlock(&ec->lock);
 }
 
-static void acpi_ec_gpe_query(void *ec_cxt);
-
 static int ec_check_sci(struct acpi_ec *ec, u8 state)
 {
 	if (state & ACPI_EC_FLAG_SCI) {
diff --git a/drivers/platform/x86/compal-laptop.c b/drivers/platform/x86/compal-laptop.c
index 034572b..f4f43e6 100644
--- a/drivers/platform/x86/compal-laptop.c
+++ b/drivers/platform/x86/compal-laptop.c
@@ -200,7 +200,7 @@ static bool extra_features;
  * watching the output of address 0x4F (do an ec_transaction writing 0x33
  * into 0x4F and read a few bytes from the output, like so:
  *	u8 writeData = 0x33;
- *	ec_transaction(0x4F, &writeData, 1, buffer, 32, 0);
+ *	ec_transaction(0x4F, &writeData, 1, buffer, 32);
  * That address is labled "fan1 table information" in the service manual.
  * It should be clear which value in 'buffer' changes). This seems to be
  * related to fan speed. It isn't a proper 'realtime' fan speed value
@@ -286,7 +286,7 @@ static int get_backlight_level(void)
 static void set_backlight_state(bool on)
 {
 	u8 data = on ? BACKLIGHT_STATE_ON_DATA : BACKLIGHT_STATE_OFF_DATA;
-	ec_transaction(BACKLIGHT_STATE_ADDR, &data, 1, NULL, 0, 0);
+	ec_transaction(BACKLIGHT_STATE_ADDR, &data, 1, NULL, 0);
 }
 
 
@@ -294,24 +294,24 @@ static void set_backlight_state(bool on)
 static void pwm_enable_control(void)
 {
 	unsigned char writeData = PWM_ENABLE_DATA;
-	ec_transaction(PWM_ENABLE_ADDR, &writeData, 1, NULL, 0, 0);
+	ec_transaction(PWM_ENABLE_ADDR, &writeData, 1, NULL, 0);
 }
 
 static void pwm_disable_control(void)
 {
 	unsigned char writeData = PWM_DISABLE_DATA;
-	ec_transaction(PWM_DISABLE_ADDR, &writeData, 1, NULL, 0, 0);
+	ec_transaction(PWM_DISABLE_ADDR, &writeData, 1, NULL, 0);
 }
 
 static void set_pwm(int pwm)
 {
-	ec_transaction(PWM_ADDRESS, &pwm_lookup_table[pwm], 1, NULL, 0, 0);
+	ec_transaction(PWM_ADDRESS, &pwm_lookup_table[pwm], 1, NULL, 0);
 }
 
 static int get_fan_rpm(void)
 {
 	u8 value, data = FAN_DATA;
-	ec_transaction(FAN_ADDRESS, &data, 1, &value, 1, 0);
+	ec_transaction(FAN_ADDRESS, &data, 1, &value, 1);
 	return 100 * (int)value;
 }
 
diff --git a/drivers/platform/x86/msi-laptop.c b/drivers/platform/x86/msi-laptop.c
index 7e9bb6d..918a65d 100644
--- a/drivers/platform/x86/msi-laptop.c
+++ b/drivers/platform/x86/msi-laptop.c
@@ -120,7 +120,7 @@ static int set_lcd_level(int level)
 	buf[1] = (u8) (level*31);
 
 	return ec_transaction(MSI_EC_COMMAND_LCD_LEVEL, buf, sizeof(buf),
-			      NULL, 0, 1);
+			      NULL, 0);
 }
 
 static int get_lcd_level(void)
@@ -129,7 +129,7 @@ static int get_lcd_level(void)
 	int result;
 
 	result = ec_transaction(MSI_EC_COMMAND_LCD_LEVEL, &wdata, 1,
-				&rdata, 1, 1);
+				&rdata, 1);
 	if (result < 0)
 		return result;
 
@@ -142,7 +142,7 @@ static int get_auto_brightness(void)
 	int result;
 
 	result = ec_transaction(MSI_EC_COMMAND_LCD_LEVEL, &wdata, 1,
-				&rdata, 1, 1);
+				&rdata, 1);
 	if (result < 0)
 		return result;
 
@@ -157,7 +157,7 @@ static int set_auto_brightness(int enable)
 	wdata[0] = 4;
 
 	result = ec_transaction(MSI_EC_COMMAND_LCD_LEVEL, wdata, 1,
-				&rdata, 1, 1);
+				&rdata, 1);
 	if (result < 0)
 		return result;
 
@@ -165,7 +165,7 @@ static int set_auto_brightness(int enable)
 	wdata[1] = (rdata & 0xF7) | (enable ? 8 : 0);
 
 	return ec_transaction(MSI_EC_COMMAND_LCD_LEVEL, wdata, 2,
-			      NULL, 0, 1);
+			      NULL, 0);
 }
 
 static ssize_t set_device_state(const char *buf, size_t count, u8 mask)
@@ -202,7 +202,7 @@ static int get_wireless_state(int *wlan, int *bluetooth)
 	u8 wdata = 0, rdata;
 	int result;
 
-	result = ec_transaction(MSI_EC_COMMAND_WIRELESS, &wdata, 1, &rdata, 1, 1);
+	result = ec_transaction(MSI_EC_COMMAND_WIRELESS, &wdata, 1, &rdata, 1);
 	if (result < 0)
 		return -1;
 
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index a2e910e..1deb2a7 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -150,8 +150,7 @@ extern int ec_read(u8 addr, u8 *val);
 extern int ec_write(u8 addr, u8 val);
 extern int ec_transaction(u8 command,
                           const u8 *wdata, unsigned wdata_len,
-                          u8 *rdata, unsigned rdata_len,
-			  int force_poll);
+                          u8 *rdata, unsigned rdata_len);
 
 #if defined(CONFIG_ACPI_WMI) || defined(CONFIG_ACPI_WMI_MODULE)
 
-- 
1.7.3.4

[PATCH 2/3] acpi: Cleanup custom_method debug stuff

[Thomas Renninger]

- Revert param aml_debug_output, it's not used.
- Split acpi_debugfs_init to prepare custom_method to be
  an own .config option and driver.

Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: Rafael J. Wysocki <rjw@sisk.pl>
CC: lenb@kernel.org
CC: rui.zhang@intel.com
CC: linux-acpi@vger.kernel.org
---
 drivers/acpi/debugfs.c  |   36 +++++++++++++++---------------------
 drivers/acpi/internal.h |    3 ++-
 2 files changed, 17 insertions(+), 22 deletions(-)

diff --git a/drivers/acpi/debugfs.c b/drivers/acpi/debugfs.c
index 384f7ab..32945c7 100644
--- a/drivers/acpi/debugfs.c
+++ b/drivers/acpi/debugfs.c
@@ -12,13 +12,8 @@
 #define _COMPONENT		ACPI_SYSTEM_COMPONENT
 ACPI_MODULE_NAME("debugfs");
 
-
-/* /sys/modules/acpi/parameters/aml_debug_output */
-
-module_param_named(aml_debug_output, acpi_gbl_enable_aml_debug_object,
-		   bool, 0644);
-MODULE_PARM_DESC(aml_debug_output,
-		 "To enable/disable the ACPI Debug Object output.");
+struct dentry *acpi_debugfs_dir;
+static struct dentry *cm_dentry;
 
 /* /sys/kernel/debug/acpi/custom_method */
 
@@ -80,23 +75,22 @@ static const struct file_operations cm_fops = {
 	.llseek = default_llseek,
 };
 
-int __init acpi_debugfs_init(void)
+static int __init acpi_custom_method_init(void)
 {
-	struct dentry *acpi_dir, *cm_dentry;
+	if (acpi_debugfs_dir == NULL)
+		return -ENOENT;
 
-	acpi_dir = debugfs_create_dir("acpi", NULL);
-	if (!acpi_dir)
-		goto err;
+ 	cm_dentry = debugfs_create_file("custom_method", S_IWUSR,
+					acpi_debugfs_dir, NULL, &cm_fops);
+	if (cm_dentry == NULL)
+		return -ENODEV;
 
-	cm_dentry = debugfs_create_file("custom_method", S_IWUSR,
-					acpi_dir, NULL, &cm_fops);
-	if (!cm_dentry)
-		goto err;
+ 	return 0;
+}
 
-	return 0;
+void __init acpi_debugfs_init(void)
+{
+	acpi_debugfs_dir = debugfs_create_dir("acpi", NULL);
 
-err:
-	if (acpi_dir)
-		debugfs_remove(acpi_dir);
-	return -EINVAL;
+	acpi_custom_method_init();
 }
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index b1cc81a..afc1a1c 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -30,9 +30,10 @@ int acpi_scan_init(void);
 int acpi_sysfs_init(void);
 
 #ifdef CONFIG_DEBUG_FS
+extern struct dentry *acpi_debugfs_dir;
 int acpi_debugfs_init(void);
 #else
-static inline int acpi_debugfs_init(void) { return 0; }
+static inline void acpi_debugfs_init(void) { return; }
 #endif
 
 /* --------------------------------------------------------------------------
-- 
1.7.3.4

[PATCH 3/3] acpi: Split out custom_method functionality into an own driver

[Thomas Renninger]

With /sys/kernel/debug/acpi/custom_method root can write
to arbitrary memory and increase his priveleges, even if
these are restricted.

-> Make this an own debug .config option and warn about the
security issue in the config description.

-> Still keep acpi/debugfs.c which now only creates and empty
   /sys/kernel/debug/acpi directory. There might be other
   users of it later.

Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: Rafael J. Wysocki <rjw@sisk.pl>
CC: lenb@kernel.org
CC: rui.zhang@intel.com
CC: linux-acpi@vger.kernel.org
---
 Documentation/acpi/method-customizing.txt |    5 ++
 drivers/acpi/Kconfig                      |   12 ++++
 drivers/acpi/Makefile                     |    1 +
 drivers/acpi/custom_method.c              |  100 +++++++++++++++++++++++++++++
 drivers/acpi/debugfs.c                    |   80 +-----------------------
 5 files changed, 119 insertions(+), 79 deletions(-)
 create mode 100644 drivers/acpi/custom_method.c

diff --git a/Documentation/acpi/method-customizing.txt b/Documentation/acpi/method-customizing.txt
index 3e1d25a..5f55373 100644
--- a/Documentation/acpi/method-customizing.txt
+++ b/Documentation/acpi/method-customizing.txt
@@ -66,3 +66,8 @@ Note: We can use a kernel with multiple custom ACPI method running,
       But each individual write to debugfs can implement a SINGLE
       method override. i.e. if we want to insert/override multiple
       ACPI methods, we need to redo step c) ~ g) for multiple times.
+
+Note: Be aware that root can mis-use this driver to modify arbitrary
+      memory and gain additional rights, if root's privileges got
+      restricted (for example if root is not allowed to load additional
+      modules after boot).
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 2aa042a..48dcbaf 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -381,6 +381,18 @@ config ACPI_HED
 	  which is used to report some hardware errors notified via
 	  SCI, mainly the corrected errors.
 
+config ACPI_CUSTOM_METHOD
+	tristate "ACPI function runtime override debug utility (SECURITY ALERT)"
+	depends on DEBUG_FS
+	default n
+	help
+	  This is an ACPI debug facility:
+	  Documentation/acpi/method-customizing.txt.
+
+	  Be aware that it allows root to override arbitrary memory and to gain
+	  extended rights on systems where root privileges may be partly
+	  restricted.
+
 source "drivers/acpi/apei/Kconfig"
 
 endif	# ACPI
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index d113fa5..cba0b23 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -62,6 +62,7 @@ obj-$(CONFIG_ACPI_SBS)		+= sbs.o
 obj-$(CONFIG_ACPI_POWER_METER)	+= power_meter.o
 obj-$(CONFIG_ACPI_HED)		+= hed.o
 obj-$(CONFIG_ACPI_EC_DEBUGFS)	+= ec_sys.o
+obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o
 
 # processor has its own "processor." module_param namespace
 processor-y			:= processor_driver.o processor_throttling.o
diff --git a/drivers/acpi/custom_method.c b/drivers/acpi/custom_method.c
new file mode 100644
index 0000000..dc554c2
--- /dev/null
+++ b/drivers/acpi/custom_method.c
@@ -0,0 +1,100 @@
+/*
+ * debugfs.c - ACPI debugfs interface to userspace.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/uaccess.h>
+#include <linux/debugfs.h>
+#include <acpi/acpi_drivers.h>
+
+#include "internal.h"
+
+#define _COMPONENT		ACPI_SYSTEM_COMPONENT
+ACPI_MODULE_NAME("custom_method");
+MODULE_LICENSE("GPL");
+
+static struct dentry *cm_dentry;
+
+/* /sys/kernel/debug/acpi/custom_method */
+
+static ssize_t cm_write(struct file *file, const char __user * user_buf,
+			size_t count, loff_t *ppos)
+{
+	static char *buf;
+	static u32 max_size;
+	static u32 uncopied_bytes;
+
+	struct acpi_table_header table;
+	acpi_status status;
+
+	if (!(*ppos)) {
+		/* parse the table header to get the table length */
+		if (count <= sizeof(struct acpi_table_header))
+			return -EINVAL;
+		if (copy_from_user(&table, user_buf,
+				   sizeof(struct acpi_table_header)))
+			return -EFAULT;
+		uncopied_bytes = max_size = table.length;
+		buf = kzalloc(max_size, GFP_KERNEL);
+		if (!buf)
+			return -ENOMEM;
+	}
+
+	if (buf == NULL)
+		return -EINVAL;
+
+	if ((*ppos > max_size) ||
+	    (*ppos + count > max_size) ||
+	    (*ppos + count < count) ||
+	    (count > uncopied_bytes))
+		return -EINVAL;
+
+	if (copy_from_user(buf + (*ppos), user_buf, count)) {
+		kfree(buf);
+		buf = NULL;
+		return -EFAULT;
+	}
+
+	uncopied_bytes -= count;
+	*ppos += count;
+
+	if (!uncopied_bytes) {
+		status = acpi_install_method(buf);
+		kfree(buf);
+		buf = NULL;
+		if (ACPI_FAILURE(status))
+			return -EINVAL;
+		add_taint(TAINT_OVERRIDDEN_ACPI_TABLE);
+	}
+
+	return count;
+}
+
+static const struct file_operations cm_fops = {
+	.write = cm_write,
+	.llseek = default_llseek,
+};
+
+static int __init acpi_custom_method_init(void)
+{
+	if (acpi_debugfs_dir == NULL)
+		return -ENOENT;
+
+ 	cm_dentry = debugfs_create_file("custom_method", S_IWUSR,
+					acpi_debugfs_dir, NULL, &cm_fops);
+	if (cm_dentry == NULL)
+		return -ENODEV;
+
+ 	return 0;
+}
+
+static void __exit acpi_custom_method_exit(void)
+{
+	if (cm_dentry)
+		debugfs_remove(cm_dentry);
+ }
+
+module_init(acpi_custom_method_init);
+module_exit(acpi_custom_method_exit);
diff --git a/drivers/acpi/debugfs.c b/drivers/acpi/debugfs.c
index 32945c7..182a9fc 100644
--- a/drivers/acpi/debugfs.c
+++ b/drivers/acpi/debugfs.c
@@ -3,9 +3,6 @@
  */
 
 #include <linux/init.h>
-#include <linux/module.h>
-#include <linux/kernel.h>
-#include <linux/uaccess.h>
 #include <linux/debugfs.h>
 #include <acpi/acpi_drivers.h>
 
@@ -13,84 +10,9 @@
 ACPI_MODULE_NAME("debugfs");
 
 struct dentry *acpi_debugfs_dir;
-static struct dentry *cm_dentry;
-
-/* /sys/kernel/debug/acpi/custom_method */
-
-static ssize_t cm_write(struct file *file, const char __user * user_buf,
-			size_t count, loff_t *ppos)
-{
-	static char *buf;
-	static u32 max_size;
-	static u32 uncopied_bytes;
-
-	struct acpi_table_header table;
-	acpi_status status;
-
-	if (!(*ppos)) {
-		/* parse the table header to get the table length */
-		if (count <= sizeof(struct acpi_table_header))
-			return -EINVAL;
-		if (copy_from_user(&table, user_buf,
-				   sizeof(struct acpi_table_header)))
-			return -EFAULT;
-		uncopied_bytes = max_size = table.length;
-		buf = kzalloc(max_size, GFP_KERNEL);
-		if (!buf)
-			return -ENOMEM;
-	}
-
-	if (buf == NULL)
-		return -EINVAL;
-
-	if ((*ppos > max_size) ||
-	    (*ppos + count > max_size) ||
-	    (*ppos + count < count) ||
-	    (count > uncopied_bytes))
-		return -EINVAL;
-
-	if (copy_from_user(buf + (*ppos), user_buf, count)) {
-		kfree(buf);
-		buf = NULL;
-		return -EFAULT;
-	}
-
-	uncopied_bytes -= count;
-	*ppos += count;
-
-	if (!uncopied_bytes) {
-		status = acpi_install_method(buf);
-		kfree(buf);
-		buf = NULL;
-		if (ACPI_FAILURE(status))
-			return -EINVAL;
-		add_taint(TAINT_OVERRIDDEN_ACPI_TABLE);
-	}
-
-	return count;
-}
-
-static const struct file_operations cm_fops = {
-	.write = cm_write,
-	.llseek = default_llseek,
-};
-
-static int __init acpi_custom_method_init(void)
-{
-	if (acpi_debugfs_dir == NULL)
-		return -ENOENT;
-
- 	cm_dentry = debugfs_create_file("custom_method", S_IWUSR,
-					acpi_debugfs_dir, NULL, &cm_fops);
-	if (cm_dentry == NULL)
-		return -ENODEV;
-
- 	return 0;
-}
+EXPORT_SYMBOL_GPL(acpi_debugfs_dir);
 
 void __init acpi_debugfs_init(void)
 {
 	acpi_debugfs_dir = debugfs_create_dir("acpi", NULL);
-
-	acpi_custom_method_init();
 }
-- 
1.7.3.4

Re: [PATCH 2/3] acpi: Cleanup custom_method debug stuff

[Rafael J. Wysocki]

On Tuesday, March 29, 2011, Thomas Renninger wrote:
> - Revert param aml_debug_output, it's not used.
> - Split acpi_debugfs_init to prepare custom_method to be
>   an own .config option and driver.
> 
> Signed-off-by: Thomas Renninger <trenn@suse.de>
> CC: Rafael J. Wysocki <rjw@sisk.pl>
> CC: lenb@kernel.org
> CC: rui.zhang@intel.com
> CC: linux-acpi@vger.kernel.org
> ---
>  drivers/acpi/debugfs.c  |   36 +++++++++++++++---------------------
>  drivers/acpi/internal.h |    3 ++-
>  2 files changed, 17 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/acpi/debugfs.c b/drivers/acpi/debugfs.c
> index 384f7ab..32945c7 100644
> --- a/drivers/acpi/debugfs.c
> +++ b/drivers/acpi/debugfs.c
> @@ -12,13 +12,8 @@
>  #define _COMPONENT		ACPI_SYSTEM_COMPONENT
>  ACPI_MODULE_NAME("debugfs");
>  
> -
> -/* /sys/modules/acpi/parameters/aml_debug_output */
> -
> -module_param_named(aml_debug_output, acpi_gbl_enable_aml_debug_object,
> -		   bool, 0644);
> -MODULE_PARM_DESC(aml_debug_output,
> -		 "To enable/disable the ACPI Debug Object output.");
> +struct dentry *acpi_debugfs_dir;
> +static struct dentry *cm_dentry;
>  
>  /* /sys/kernel/debug/acpi/custom_method */
>  
> @@ -80,23 +75,22 @@ static const struct file_operations cm_fops = {
>  	.llseek = default_llseek,
>  };
>  
> -int __init acpi_debugfs_init(void)
> +static int __init acpi_custom_method_init(void)
>  {
> -	struct dentry *acpi_dir, *cm_dentry;
> +	if (acpi_debugfs_dir == NULL)
> +		return -ENOENT;

if (!acpi_debugfs_dir)
	return -ENOENT;

perhaps?

Apart from this it looks good.

> -	acpi_dir = debugfs_create_dir("acpi", NULL);
> -	if (!acpi_dir)
> -		goto err;
> + 	cm_dentry = debugfs_create_file("custom_method", S_IWUSR,
> +					acpi_debugfs_dir, NULL, &cm_fops);
> +	if (cm_dentry == NULL)
> +		return -ENODEV;
>  
> -	cm_dentry = debugfs_create_file("custom_method", S_IWUSR,
> -					acpi_dir, NULL, &cm_fops);
> -	if (!cm_dentry)
> -		goto err;
> + 	return 0;
> +}
>  
> -	return 0;
> +void __init acpi_debugfs_init(void)
> +{
> +	acpi_debugfs_dir = debugfs_create_dir("acpi", NULL);
>  
> -err:
> -	if (acpi_dir)
> -		debugfs_remove(acpi_dir);
> -	return -EINVAL;
> +	acpi_custom_method_init();
>  }
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index b1cc81a..afc1a1c 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -30,9 +30,10 @@ int acpi_scan_init(void);
>  int acpi_sysfs_init(void);
>  
>  #ifdef CONFIG_DEBUG_FS
> +extern struct dentry *acpi_debugfs_dir;
>  int acpi_debugfs_init(void);
>  #else
> -static inline int acpi_debugfs_init(void) { return 0; }
> +static inline void acpi_debugfs_init(void) { return; }
>  #endif
>  
>  /* --------------------------------------------------------------------------
> 

Re: [PATCH 3/3] acpi: Split out custom_method functionality into an own driver

[Rafael J. Wysocki]

On Tuesday, March 29, 2011, Thomas Renninger wrote:
> With /sys/kernel/debug/acpi/custom_method root can write
> to arbitrary memory and increase his priveleges, even if
> these are restricted.
> 
> -> Make this an own debug .config option and warn about the
> security issue in the config description.
> 
> -> Still keep acpi/debugfs.c which now only creates and empty
>    /sys/kernel/debug/acpi directory. There might be other
>    users of it later.
> 
> Signed-off-by: Thomas Renninger <trenn@suse.de>
> CC: Rafael J. Wysocki <rjw@sisk.pl>
> CC: lenb@kernel.org
> CC: rui.zhang@intel.com
> CC: linux-acpi@vger.kernel.org

OK, but you don't need to move custom_method to a separate file.  Why are
you doing that, exactly?

> ---
>  Documentation/acpi/method-customizing.txt |    5 ++
>  drivers/acpi/Kconfig                      |   12 ++++
>  drivers/acpi/Makefile                     |    1 +
>  drivers/acpi/custom_method.c              |  100 +++++++++++++++++++++++++++++
>  drivers/acpi/debugfs.c                    |   80 +-----------------------
>  5 files changed, 119 insertions(+), 79 deletions(-)
>  create mode 100644 drivers/acpi/custom_method.c
> 
> diff --git a/Documentation/acpi/method-customizing.txt b/Documentation/acpi/method-customizing.txt
> index 3e1d25a..5f55373 100644
> --- a/Documentation/acpi/method-customizing.txt
> +++ b/Documentation/acpi/method-customizing.txt
> @@ -66,3 +66,8 @@ Note: We can use a kernel with multiple custom ACPI method running,
>        But each individual write to debugfs can implement a SINGLE
>        method override. i.e. if we want to insert/override multiple
>        ACPI methods, we need to redo step c) ~ g) for multiple times.
> +
> +Note: Be aware that root can mis-use this driver to modify arbitrary
> +      memory and gain additional rights, if root's privileges got
> +      restricted (for example if root is not allowed to load additional
> +      modules after boot).
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 2aa042a..48dcbaf 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -381,6 +381,18 @@ config ACPI_HED
>  	  which is used to report some hardware errors notified via
>  	  SCI, mainly the corrected errors.
>  
> +config ACPI_CUSTOM_METHOD
> +	tristate "ACPI function runtime override debug utility (SECURITY ALERT)"

I wouldn't put the "SECURITY ALERT" in the option string.  I'd call it
"Allow ACPI methods to be inserted/replaced at run time"

> +	depends on DEBUG_FS
> +	default n
> +	help
> +	  This is an ACPI debug facility:

Here, I'd say:
"This debug facility allows ACPI AML methods to me inserted and/or replaced
without rebooting the system.  For details refer to "

> +	  Documentation/acpi/method-customizing.txt.
> +
> +	  Be aware that it allows root to override arbitrary memory and to gain
> +	  extended rights on systems where root privileges may be partly
> +	  restricted.

Here, I'd say.

"NOTE: This option is security sensitive, because it allows arbitrary kernel
memory to be written to by root (uid=0) users, allowing them to bypass certain
security measures (e.g. if root is not allowed to load additional kernel modules
after boot, this feature may be used to override that restriction)."

> +
>  source "drivers/acpi/apei/Kconfig"
>  
>  endif	# ACPI
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index d113fa5..cba0b23 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -62,6 +62,7 @@ obj-$(CONFIG_ACPI_SBS)		+= sbs.o
>  obj-$(CONFIG_ACPI_POWER_METER)	+= power_meter.o
>  obj-$(CONFIG_ACPI_HED)		+= hed.o
>  obj-$(CONFIG_ACPI_EC_DEBUGFS)	+= ec_sys.o
> +obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o
>  
>  # processor has its own "processor." module_param namespace
>  processor-y			:= processor_driver.o processor_throttling.o
> diff --git a/drivers/acpi/custom_method.c b/drivers/acpi/custom_method.c
> new file mode 100644
> index 0000000..dc554c2
> --- /dev/null
> +++ b/drivers/acpi/custom_method.c
> @@ -0,0 +1,100 @@
> +/*
> + * debugfs.c - ACPI debugfs interface to userspace.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/uaccess.h>
> +#include <linux/debugfs.h>
> +#include <acpi/acpi_drivers.h>
> +
> +#include "internal.h"
> +
> +#define _COMPONENT		ACPI_SYSTEM_COMPONENT
> +ACPI_MODULE_NAME("custom_method");
> +MODULE_LICENSE("GPL");
> +
> +static struct dentry *cm_dentry;
> +
> +/* /sys/kernel/debug/acpi/custom_method */
> +
> +static ssize_t cm_write(struct file *file, const char __user * user_buf,
> +			size_t count, loff_t *ppos)
> +{
> +	static char *buf;
> +	static u32 max_size;
> +	static u32 uncopied_bytes;
> +
> +	struct acpi_table_header table;
> +	acpi_status status;
> +
> +	if (!(*ppos)) {
> +		/* parse the table header to get the table length */
> +		if (count <= sizeof(struct acpi_table_header))
> +			return -EINVAL;
> +		if (copy_from_user(&table, user_buf,
> +				   sizeof(struct acpi_table_header)))
> +			return -EFAULT;
> +		uncopied_bytes = max_size = table.length;
> +		buf = kzalloc(max_size, GFP_KERNEL);
> +		if (!buf)
> +			return -ENOMEM;
> +	}
> +
> +	if (buf == NULL)
> +		return -EINVAL;
> +
> +	if ((*ppos > max_size) ||
> +	    (*ppos + count > max_size) ||
> +	    (*ppos + count < count) ||
> +	    (count > uncopied_bytes))
> +		return -EINVAL;
> +
> +	if (copy_from_user(buf + (*ppos), user_buf, count)) {
> +		kfree(buf);
> +		buf = NULL;
> +		return -EFAULT;
> +	}
> +
> +	uncopied_bytes -= count;
> +	*ppos += count;
> +
> +	if (!uncopied_bytes) {
> +		status = acpi_install_method(buf);
> +		kfree(buf);
> +		buf = NULL;
> +		if (ACPI_FAILURE(status))
> +			return -EINVAL;
> +		add_taint(TAINT_OVERRIDDEN_ACPI_TABLE);
> +	}
> +
> +	return count;
> +}
> +
> +static const struct file_operations cm_fops = {
> +	.write = cm_write,
> +	.llseek = default_llseek,
> +};
> +
> +static int __init acpi_custom_method_init(void)
> +{
> +	if (acpi_debugfs_dir == NULL)
> +		return -ENOENT;
> +
> + 	cm_dentry = debugfs_create_file("custom_method", S_IWUSR,
> +					acpi_debugfs_dir, NULL, &cm_fops);
> +	if (cm_dentry == NULL)
> +		return -ENODEV;
> +
> + 	return 0;
> +}
> +
> +static void __exit acpi_custom_method_exit(void)
> +{
> +	if (cm_dentry)
> +		debugfs_remove(cm_dentry);
> + }
> +
> +module_init(acpi_custom_method_init);
> +module_exit(acpi_custom_method_exit);
> diff --git a/drivers/acpi/debugfs.c b/drivers/acpi/debugfs.c
> index 32945c7..182a9fc 100644
> --- a/drivers/acpi/debugfs.c
> +++ b/drivers/acpi/debugfs.c
> @@ -3,9 +3,6 @@
>   */
>  
>  #include <linux/init.h>
> -#include <linux/module.h>
> -#include <linux/kernel.h>
> -#include <linux/uaccess.h>
>  #include <linux/debugfs.h>
>  #include <acpi/acpi_drivers.h>
>  
> @@ -13,84 +10,9 @@
>  ACPI_MODULE_NAME("debugfs");
>  
>  struct dentry *acpi_debugfs_dir;
> -static struct dentry *cm_dentry;
> -
> -/* /sys/kernel/debug/acpi/custom_method */
> -
> -static ssize_t cm_write(struct file *file, const char __user * user_buf,
> -			size_t count, loff_t *ppos)
> -{
> -	static char *buf;
> -	static u32 max_size;
> -	static u32 uncopied_bytes;
> -
> -	struct acpi_table_header table;
> -	acpi_status status;
> -
> -	if (!(*ppos)) {
> -		/* parse the table header to get the table length */
> -		if (count <= sizeof(struct acpi_table_header))
> -			return -EINVAL;
> -		if (copy_from_user(&table, user_buf,
> -				   sizeof(struct acpi_table_header)))
> -			return -EFAULT;
> -		uncopied_bytes = max_size = table.length;
> -		buf = kzalloc(max_size, GFP_KERNEL);
> -		if (!buf)
> -			return -ENOMEM;
> -	}
> -
> -	if (buf == NULL)
> -		return -EINVAL;
> -
> -	if ((*ppos > max_size) ||
> -	    (*ppos + count > max_size) ||
> -	    (*ppos + count < count) ||
> -	    (count > uncopied_bytes))
> -		return -EINVAL;
> -
> -	if (copy_from_user(buf + (*ppos), user_buf, count)) {
> -		kfree(buf);
> -		buf = NULL;
> -		return -EFAULT;
> -	}
> -
> -	uncopied_bytes -= count;
> -	*ppos += count;
> -
> -	if (!uncopied_bytes) {
> -		status = acpi_install_method(buf);
> -		kfree(buf);
> -		buf = NULL;
> -		if (ACPI_FAILURE(status))
> -			return -EINVAL;
> -		add_taint(TAINT_OVERRIDDEN_ACPI_TABLE);
> -	}
> -
> -	return count;
> -}
> -
> -static const struct file_operations cm_fops = {
> -	.write = cm_write,
> -	.llseek = default_llseek,
> -};
> -
> -static int __init acpi_custom_method_init(void)
> -{
> -	if (acpi_debugfs_dir == NULL)
> -		return -ENOENT;
> -
> - 	cm_dentry = debugfs_create_file("custom_method", S_IWUSR,
> -					acpi_debugfs_dir, NULL, &cm_fops);
> -	if (cm_dentry == NULL)
> -		return -ENODEV;
> -
> - 	return 0;
> -}
> +EXPORT_SYMBOL_GPL(acpi_debugfs_dir);
>  
>  void __init acpi_debugfs_init(void)
>  {
>  	acpi_debugfs_dir = debugfs_create_dir("acpi", NULL);
> -
> -	acpi_custom_method_init();
>  }
> 

Re: [PATCH 3/3] acpi: Split out custom_method functionality into an own driver

[Thomas Renninger]

On Tuesday 29 March 2011 21:36:50 Rafael J. Wysocki wrote:
> On Tuesday, March 29, 2011, Thomas Renninger wrote:
> > With /sys/kernel/debug/acpi/custom_method root can write
> > to arbitrary memory and increase his priveleges, even if
> > these are restricted.
> > 
> > -> Make this an own debug .config option and warn about the
> > security issue in the config description.
> > 
> > -> Still keep acpi/debugfs.c which now only creates and empty
> >    /sys/kernel/debug/acpi directory. There might be other
> >    users of it later.
> > 
> > Signed-off-by: Thomas Renninger <trenn@suse.de>
> > CC: Rafael J. Wysocki <rjw@sisk.pl>
> > CC: lenb@kernel.org
> > CC: rui.zhang@intel.com
> > CC: linux-acpi@vger.kernel.org
> 
> OK, but you don't need to move custom_method to a separate file.  Why
> are you doing that, exactly?
Because there may come other acpi debugfs stuff added there and then
it either needs ugly #ifdef logics inside the file or the split will be done then.
Separating the code belonging to this option into an own
file looks like the cleanest way to me.
 
> > +config ACPI_CUSTOM_METHOD
> > +	tristate "ACPI function runtime override debug utility (SECURITY ALERT)"
> 
> I wouldn't put the "SECURITY ALERT" in the option string.  I'd call it
> "Allow ACPI methods to be inserted/replaced at run time"
I agree.
> 
> > +	depends on DEBUG_FS
> > +	default n
> > +	help
> > +	  This is an ACPI debug facility:
> 
> Here, I'd say:
> "This debug facility allows ACPI AML methods to me inserted and/or replaced
> without rebooting the system.  For details refer to "
Yep, I'll fix the wording of this one and the rest and will resubmit tomorrow.

Thanks,

    Thomas

Re: [PATCH 3/3] acpi: Split out custom_method functionality into an own driver

[Rafael J. Wysocki]

On Tuesday, March 29, 2011, Thomas Renninger wrote:
> On Tuesday 29 March 2011 21:36:50 Rafael J. Wysocki wrote:
> > On Tuesday, March 29, 2011, Thomas Renninger wrote:
> > > With /sys/kernel/debug/acpi/custom_method root can write
> > > to arbitrary memory and increase his priveleges, even if
> > > these are restricted.
> > > 
> > > -> Make this an own debug .config option and warn about the
> > > security issue in the config description.
> > > 
> > > -> Still keep acpi/debugfs.c which now only creates and empty
> > >    /sys/kernel/debug/acpi directory. There might be other
> > >    users of it later.
> > > 
> > > Signed-off-by: Thomas Renninger <trenn@suse.de>
> > > CC: Rafael J. Wysocki <rjw@sisk.pl>
> > > CC: lenb@kernel.org
> > > CC: rui.zhang@intel.com
> > > CC: linux-acpi@vger.kernel.org
> > 
> > OK, but you don't need to move custom_method to a separate file.  Why
> > are you doing that, exactly?
> Because there may come other acpi debugfs stuff added there and then
> it either needs ugly #ifdef logics inside the file or the split will be done then.
> Separating the code belonging to this option into an own
> file looks like the cleanest way to me.

Well, I don't really expect anything else to depend on
CONFIG_ACPI_CUSTOM_METHOD, or if it does, the options should be called
differently, right?

Thanks,
Rafael

Re: [PATCH 2/3] acpi: Cleanup custom_method debug stuff

[Zhang Rui]

On Tue, 2011-03-29 at 20:33 +0800, Thomas Renninger wrote:
> - Revert param aml_debug_output, it's not used.

NAK.

void
acpi_ex_do_debug_object(union acpi_operand_object *source_desc,
                        u32 level, u32 index)
{
	...
        if (!acpi_gbl_enable_aml_debug_object &&
            !(acpi_dbg_level & ACPI_LV_DEBUG_OBJECT)) {
                return_VOID;
        }
	...
}

Users can set acpi_gbl_enable_aml_debug_object via module parameter
aml_bug_output, thus enable the ACPI Debug output at runtime.

thanks,
rui

> - Split acpi_debugfs_init to prepare custom_method to be
>   an own .config option and driver.
> 
> Signed-off-by: Thomas Renninger <trenn@suse.de>
> CC: Rafael J. Wysocki <rjw@sisk.pl>
> CC: lenb@kernel.org
> CC: rui.zhang@intel.com
> CC: linux-acpi@vger.kernel.org
> ---
>  drivers/acpi/debugfs.c  |   36 +++++++++++++++---------------------
>  drivers/acpi/internal.h |    3 ++-
>  2 files changed, 17 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/acpi/debugfs.c b/drivers/acpi/debugfs.c
> index 384f7ab..32945c7 100644
> --- a/drivers/acpi/debugfs.c
> +++ b/drivers/acpi/debugfs.c
> @@ -12,13 +12,8 @@
>  #define _COMPONENT		ACPI_SYSTEM_COMPONENT
>  ACPI_MODULE_NAME("debugfs");
>  
> -
> -/* /sys/modules/acpi/parameters/aml_debug_output */
> -
> -module_param_named(aml_debug_output, acpi_gbl_enable_aml_debug_object,
> -		   bool, 0644);
> -MODULE_PARM_DESC(aml_debug_output,
> -		 "To enable/disable the ACPI Debug Object output.");
> +struct dentry *acpi_debugfs_dir;
> +static struct dentry *cm_dentry;
>  
>  /* /sys/kernel/debug/acpi/custom_method */
>  
> @@ -80,23 +75,22 @@ static const struct file_operations cm_fops = {
>  	.llseek = default_llseek,
>  };
>  
> -int __init acpi_debugfs_init(void)
> +static int __init acpi_custom_method_init(void)
>  {
> -	struct dentry *acpi_dir, *cm_dentry;
> +	if (acpi_debugfs_dir == NULL)
> +		return -ENOENT;
>  
> -	acpi_dir = debugfs_create_dir("acpi", NULL);
> -	if (!acpi_dir)
> -		goto err;
> + 	cm_dentry = debugfs_create_file("custom_method", S_IWUSR,
> +					acpi_debugfs_dir, NULL, &cm_fops);
> +	if (cm_dentry == NULL)
> +		return -ENODEV;
>  
> -	cm_dentry = debugfs_create_file("custom_method", S_IWUSR,
> -					acpi_dir, NULL, &cm_fops);
> -	if (!cm_dentry)
> -		goto err;
> + 	return 0;
> +}
>  
> -	return 0;
> +void __init acpi_debugfs_init(void)
> +{
> +	acpi_debugfs_dir = debugfs_create_dir("acpi", NULL);
>  
> -err:
> -	if (acpi_dir)
> -		debugfs_remove(acpi_dir);
> -	return -EINVAL;
> +	acpi_custom_method_init();
>  }
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index b1cc81a..afc1a1c 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -30,9 +30,10 @@ int acpi_scan_init(void);
>  int acpi_sysfs_init(void);
>  
>  #ifdef CONFIG_DEBUG_FS
> +extern struct dentry *acpi_debugfs_dir;
>  int acpi_debugfs_init(void);
>  #else
> -static inline int acpi_debugfs_init(void) { return 0; }
> +static inline void acpi_debugfs_init(void) { return; }
>  #endif
>  
>  /* --------------------------------------------------------------------------

Re: [PATCH 3/3] acpi: Split out custom_method functionality into an own driver

[Zhang Rui]

On Tue, 2011-03-29 at 20:33 +0800, Thomas Renninger wrote:
> With /sys/kernel/debug/acpi/custom_method root can write
> to arbitrary memory and increase his priveleges, even if
> these are restricted.
> 
Sorry, I don't quite understand.

This interface just allocates a new piece of memory, copy the asl code
from user space and then attach it to ACPI namespace.

can you give more details about how it is misused to increase root's
privileges please?

thanks,
rui

> -> Make this an own debug .config option and warn about the
> security issue in the config description.
> 
> -> Still keep acpi/debugfs.c which now only creates and empty
>    /sys/kernel/debug/acpi directory. There might be other
>    users of it later.
> 
> Signed-off-by: Thomas Renninger <trenn@suse.de>
> CC: Rafael J. Wysocki <rjw@sisk.pl>
> CC: lenb@kernel.org
> CC: rui.zhang@intel.com
> CC: linux-acpi@vger.kernel.org
> ---
>  Documentation/acpi/method-customizing.txt |    5 ++
>  drivers/acpi/Kconfig                      |   12 ++++
>  drivers/acpi/Makefile                     |    1 +
>  drivers/acpi/custom_method.c              |  100 +++++++++++++++++++++++++++++
>  drivers/acpi/debugfs.c                    |   80 +-----------------------
>  5 files changed, 119 insertions(+), 79 deletions(-)
>  create mode 100644 drivers/acpi/custom_method.c
> 
> diff --git a/Documentation/acpi/method-customizing.txt b/Documentation/acpi/method-customizing.txt
> index 3e1d25a..5f55373 100644
> --- a/Documentation/acpi/method-customizing.txt
> +++ b/Documentation/acpi/method-customizing.txt
> @@ -66,3 +66,8 @@ Note: We can use a kernel with multiple custom ACPI method running,
>        But each individual write to debugfs can implement a SINGLE
>        method override. i.e. if we want to insert/override multiple
>        ACPI methods, we need to redo step c) ~ g) for multiple times.
> +
> +Note: Be aware that root can mis-use this driver to modify arbitrary
> +      memory and gain additional rights, if root's privileges got
> +      restricted (for example if root is not allowed to load additional
> +      modules after boot).
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 2aa042a..48dcbaf 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -381,6 +381,18 @@ config ACPI_HED
>  	  which is used to report some hardware errors notified via
>  	  SCI, mainly the corrected errors.
>  
> +config ACPI_CUSTOM_METHOD
> +	tristate "ACPI function runtime override debug utility (SECURITY ALERT)"
> +	depends on DEBUG_FS
> +	default n
> +	help
> +	  This is an ACPI debug facility:
> +	  Documentation/acpi/method-customizing.txt.
> +
> +	  Be aware that it allows root to override arbitrary memory and to gain
> +	  extended rights on systems where root privileges may be partly
> +	  restricted.
> +
>  source "drivers/acpi/apei/Kconfig"
>  
>  endif	# ACPI
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index d113fa5..cba0b23 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -62,6 +62,7 @@ obj-$(CONFIG_ACPI_SBS)		+= sbs.o
>  obj-$(CONFIG_ACPI_POWER_METER)	+= power_meter.o
>  obj-$(CONFIG_ACPI_HED)		+= hed.o
>  obj-$(CONFIG_ACPI_EC_DEBUGFS)	+= ec_sys.o
> +obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o
>  
>  # processor has its own "processor." module_param namespace
>  processor-y			:= processor_driver.o processor_throttling.o
> diff --git a/drivers/acpi/custom_method.c b/drivers/acpi/custom_method.c
> new file mode 100644
> index 0000000..dc554c2
> --- /dev/null
> +++ b/drivers/acpi/custom_method.c
> @@ -0,0 +1,100 @@
> +/*
> + * debugfs.c - ACPI debugfs interface to userspace.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/uaccess.h>
> +#include <linux/debugfs.h>
> +#include <acpi/acpi_drivers.h>
> +
> +#include "internal.h"
> +
> +#define _COMPONENT		ACPI_SYSTEM_COMPONENT
> +ACPI_MODULE_NAME("custom_method");
> +MODULE_LICENSE("GPL");
> +
> +static struct dentry *cm_dentry;
> +
> +/* /sys/kernel/debug/acpi/custom_method */
> +
> +static ssize_t cm_write(struct file *file, const char __user * user_buf,
> +			size_t count, loff_t *ppos)
> +{
> +	static char *buf;
> +	static u32 max_size;
> +	static u32 uncopied_bytes;
> +
> +	struct acpi_table_header table;
> +	acpi_status status;
> +
> +	if (!(*ppos)) {
> +		/* parse the table header to get the table length */
> +		if (count <= sizeof(struct acpi_table_header))
> +			return -EINVAL;
> +		if (copy_from_user(&table, user_buf,
> +				   sizeof(struct acpi_table_header)))
> +			return -EFAULT;
> +		uncopied_bytes = max_size = table.length;
> +		buf = kzalloc(max_size, GFP_KERNEL);
> +		if (!buf)
> +			return -ENOMEM;
> +	}
> +
> +	if (buf == NULL)
> +		return -EINVAL;
> +
> +	if ((*ppos > max_size) ||
> +	    (*ppos + count > max_size) ||
> +	    (*ppos + count < count) ||
> +	    (count > uncopied_bytes))
> +		return -EINVAL;
> +
> +	if (copy_from_user(buf + (*ppos), user_buf, count)) {
> +		kfree(buf);
> +		buf = NULL;
> +		return -EFAULT;
> +	}
> +
> +	uncopied_bytes -= count;
> +	*ppos += count;
> +
> +	if (!uncopied_bytes) {
> +		status = acpi_install_method(buf);
> +		kfree(buf);
> +		buf = NULL;
> +		if (ACPI_FAILURE(status))
> +			return -EINVAL;
> +		add_taint(TAINT_OVERRIDDEN_ACPI_TABLE);
> +	}
> +
> +	return count;
> +}
> +
> +static const struct file_operations cm_fops = {
> +	.write = cm_write,
> +	.llseek = default_llseek,
> +};
> +
> +static int __init acpi_custom_method_init(void)
> +{
> +	if (acpi_debugfs_dir == NULL)
> +		return -ENOENT;
> +
> + 	cm_dentry = debugfs_create_file("custom_method", S_IWUSR,
> +					acpi_debugfs_dir, NULL, &cm_fops);
> +	if (cm_dentry == NULL)
> +		return -ENODEV;
> +
> + 	return 0;
> +}
> +
> +static void __exit acpi_custom_method_exit(void)
> +{
> +	if (cm_dentry)
> +		debugfs_remove(cm_dentry);
> + }
> +
> +module_init(acpi_custom_method_init);
> +module_exit(acpi_custom_method_exit);
> diff --git a/drivers/acpi/debugfs.c b/drivers/acpi/debugfs.c
> index 32945c7..182a9fc 100644
> --- a/drivers/acpi/debugfs.c
> +++ b/drivers/acpi/debugfs.c
> @@ -3,9 +3,6 @@
>   */
>  
>  #include <linux/init.h>
> -#include <linux/module.h>
> -#include <linux/kernel.h>
> -#include <linux/uaccess.h>
>  #include <linux/debugfs.h>
>  #include <acpi/acpi_drivers.h>
>  
> @@ -13,84 +10,9 @@
>  ACPI_MODULE_NAME("debugfs");
>  
>  struct dentry *acpi_debugfs_dir;
> -static struct dentry *cm_dentry;
> -
> -/* /sys/kernel/debug/acpi/custom_method */
> -
> -static ssize_t cm_write(struct file *file, const char __user * user_buf,
> -			size_t count, loff_t *ppos)
> -{
> -	static char *buf;
> -	static u32 max_size;
> -	static u32 uncopied_bytes;
> -
> -	struct acpi_table_header table;
> -	acpi_status status;
> -
> -	if (!(*ppos)) {
> -		/* parse the table header to get the table length */
> -		if (count <= sizeof(struct acpi_table_header))
> -			return -EINVAL;
> -		if (copy_from_user(&table, user_buf,
> -				   sizeof(struct acpi_table_header)))
> -			return -EFAULT;
> -		uncopied_bytes = max_size = table.length;
> -		buf = kzalloc(max_size, GFP_KERNEL);
> -		if (!buf)
> -			return -ENOMEM;
> -	}
> -
> -	if (buf == NULL)
> -		return -EINVAL;
> -
> -	if ((*ppos > max_size) ||
> -	    (*ppos + count > max_size) ||
> -	    (*ppos + count < count) ||
> -	    (count > uncopied_bytes))
> -		return -EINVAL;
> -
> -	if (copy_from_user(buf + (*ppos), user_buf, count)) {
> -		kfree(buf);
> -		buf = NULL;
> -		return -EFAULT;
> -	}
> -
> -	uncopied_bytes -= count;
> -	*ppos += count;
> -
> -	if (!uncopied_bytes) {
> -		status = acpi_install_method(buf);
> -		kfree(buf);
> -		buf = NULL;
> -		if (ACPI_FAILURE(status))
> -			return -EINVAL;
> -		add_taint(TAINT_OVERRIDDEN_ACPI_TABLE);
> -	}
> -
> -	return count;
> -}
> -
> -static const struct file_operations cm_fops = {
> -	.write = cm_write,
> -	.llseek = default_llseek,
> -};
> -
> -static int __init acpi_custom_method_init(void)
> -{
> -	if (acpi_debugfs_dir == NULL)
> -		return -ENOENT;
> -
> - 	cm_dentry = debugfs_create_file("custom_method", S_IWUSR,
> -					acpi_debugfs_dir, NULL, &cm_fops);
> -	if (cm_dentry == NULL)
> -		return -ENODEV;
> -
> - 	return 0;
> -}
> +EXPORT_SYMBOL_GPL(acpi_debugfs_dir);
>  
>  void __init acpi_debugfs_init(void)
>  {
>  	acpi_debugfs_dir = debugfs_create_dir("acpi", NULL);
> -
> -	acpi_custom_method_init();
>  }

Re: [PATCH 3/3] acpi: Split out custom_method functionality into an own driver

[Matthew Garrett]

On Wed, Mar 30, 2011 at 10:03:48AM +0800, Zhang Rui wrote:
> On Tue, 2011-03-29 at 20:33 +0800, Thomas Renninger wrote:
> > With /sys/kernel/debug/acpi/custom_method root can write
> > to arbitrary memory and increase his priveleges, even if
> > these are restricted.
> > 
> Sorry, I don't quite understand.
> 
> This interface just allocates a new piece of memory, copy the asl code
> from user space and then attach it to ACPI namespace.
> 
> can you give more details about how it is misused to increase root's
> privileges please?

Identify the lid switch GPE. Start a shell, and identify the address of 
that processes's capabilities structure. Write some ASL that includes an 
opregion that covers that structure and a GPE handler that writes new 
values to it. Insert via custom_method. Close lid.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 2/3] acpi: Cleanup custom_method debug stuff

[Thomas Renninger]

On Wednesday, March 30, 2011 03:37:45 AM Zhang Rui wrote:
> On Tue, 2011-03-29 at 20:33 +0800, Thomas Renninger wrote:
> > - Revert param aml_debug_output, it's not used.
> 
> NAK.
> 
> void
> acpi_ex_do_debug_object(union acpi_operand_object *source_desc,
>                         u32 level, u32 index)
> {
> 	...
>         if (!acpi_gbl_enable_aml_debug_object &&
>             !(acpi_dbg_level & ACPI_LV_DEBUG_OBJECT)) {
>                 return_VOID;
>         }
> 	...
> }
> 
> Users can set acpi_gbl_enable_aml_debug_object via module parameter
> aml_bug_output, thus enable the ACPI Debug output at runtime.
Ah yes, I've overseen this one.
What is the advantage of:
/sys/module/acpi/parameters/aml_debug_output
over:
/sys/module/acpi/parameters/{debug_level,debug_layer}
?

The first is also available even if CONFIG_ACPI_DEBUG
is not defined at all?

Yep, this is rather useful. I still would move the param into:
drivers/acpi/sysfs.c
if I touch this now anyway, where all the other
/sys tunables are flying around:
grep \/sys drivers/acpi/sysfs.c
 * /sys/modules/acpi/parameters/debug_layer
 * /sys/modules/acpi/parameters/debug_level
 * /sys/modules/acpi/parameters/trace_method_name
 * /sys/modules/acpi/parameters/trace_state
 * /sys/modules/acpi/parameters/trace_debug_layer
 * /sys/modules/acpi/parameters/trace_debug_level
/* /sys/module/acpi/parameters/acpica_version */
 * /sys/firmware/acpi/tables/
 * /sys/firmware/acpi/tables/dynamic/
 * /sys/firmware/acpi/interrupts/

...if that's fine with you.

   Thomas

Re: [PATCH 2/3] acpi: Cleanup custom_method debug stuff

[Zhang Rui]

On Wed, 2011-03-30 at 17:06 +0800, Thomas Renninger wrote:
> On Wednesday, March 30, 2011 03:37:45 AM Zhang Rui wrote:
> > On Tue, 2011-03-29 at 20:33 +0800, Thomas Renninger wrote:
> > > - Revert param aml_debug_output, it's not used.
> > 
> > NAK.
> > 
> > void
> > acpi_ex_do_debug_object(union acpi_operand_object *source_desc,
> >                         u32 level, u32 index)
> > {
> > 	...
> >         if (!acpi_gbl_enable_aml_debug_object &&
> >             !(acpi_dbg_level & ACPI_LV_DEBUG_OBJECT)) {
> >                 return_VOID;
> >         }
> > 	...
> > }
> > 
> > Users can set acpi_gbl_enable_aml_debug_object via module parameter
> > aml_bug_output, thus enable the ACPI Debug output at runtime.
> Ah yes, I've overseen this one.
> What is the advantage of:
> /sys/module/acpi/parameters/aml_debug_output
> over:
> /sys/module/acpi/parameters/{debug_level,debug_layer}
> ?
> 
> The first is also available even if CONFIG_ACPI_DEBUG
> is not defined at all?

right. Users can enable the ACPI Debug output w/o rebuilding the kernel.

> 
> Yep, this is rather useful. I still would move the param into:
> drivers/acpi/sysfs.c
> if I touch this now anyway, where all the other
> /sys tunables are flying around:
> grep \/sys drivers/acpi/sysfs.c
>  * /sys/modules/acpi/parameters/debug_layer
>  * /sys/modules/acpi/parameters/debug_level
>  * /sys/modules/acpi/parameters/trace_method_name
>  * /sys/modules/acpi/parameters/trace_state
>  * /sys/modules/acpi/parameters/trace_debug_layer
>  * /sys/modules/acpi/parameters/trace_debug_level
> /* /sys/module/acpi/parameters/acpica_version */
>  * /sys/firmware/acpi/tables/
>  * /sys/firmware/acpi/tables/dynamic/
>  * /sys/firmware/acpi/interrupts/
> 
> ...if that's fine with you.
> 
aml_debug_output is designed to be used together with the runtime
control method override. And this is why I put it in debugfs.c rather
than sysfs.c.
I have no strong objection to your proposal because I'm really not sure
which one is better. :p

thanks,
rui

>    Thomas

[PATCH 3/3] acpi: Split out custom_method functionality into an own driver

[Thomas Renninger]

With /sys/kernel/debug/acpi/custom_method root can write
to arbitrary memory and increase his priveleges, even if
these are restricted.

-> Make this an own debug .config option and warn about the
security issue in the config description.

-> Still keep acpi/debugfs.c which now only creates and empty
   /sys/kernel/debug/acpi directory. There might be other
   users of it later.

Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: Rafael J. Wysocki <rjw@sisk.pl>
CC: lenb@kernel.org
CC: rui.zhang@intel.com
CC: linux-acpi@vger.kernel.org
---
 Documentation/acpi/method-customizing.txt |    5 ++
 drivers/acpi/Kconfig                      |   15 +++++
 drivers/acpi/Makefile                     |    1 +
 drivers/acpi/debugfs.c                    |   80 +----------------------------
 4 files changed, 22 insertions(+), 79 deletions(-)

diff --git a/Documentation/acpi/method-customizing.txt b/Documentation/acpi/method-customizing.txt
index 3e1d25a..5f55373 100644
--- a/Documentation/acpi/method-customizing.txt
+++ b/Documentation/acpi/method-customizing.txt
@@ -66,3 +66,8 @@ Note: We can use a kernel with multiple custom ACPI method running,
       But each individual write to debugfs can implement a SINGLE
       method override. i.e. if we want to insert/override multiple
       ACPI methods, we need to redo step c) ~ g) for multiple times.
+
+Note: Be aware that root can mis-use this driver to modify arbitrary
+      memory and gain additional rights, if root's privileges got
+      restricted (for example if root is not allowed to load additional
+      modules after boot).
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 2aa042a..3feeec8 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -381,6 +381,21 @@ config ACPI_HED
 	  which is used to report some hardware errors notified via
 	  SCI, mainly the corrected errors.
 
+config ACPI_CUSTOM_METHOD
+	tristate "Allow ACPI methods to be inserted/replaced at run time"
+	depends on DEBUG_FS
+	default n
+	help
+	  This debug facility allows ACPI AML methods to me inserted and/or
+	  replaced without rebooting the system. For details refer to:
+	  Documentation/acpi/method-customizing.txt.
+
+	  NOTE: This option is security sensitive, because it allows arbitrary
+	  kernel memory to be written to by root (uid=0) users, allowing them
+	  to bypass certain security measures (e.g. if root is not allowed to
+	  load additional kernel modules after boot, this feature may be used
+	  to override that restriction).
+
 source "drivers/acpi/apei/Kconfig"
 
 endif	# ACPI
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index d113fa5..cba0b23 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -62,6 +62,7 @@ obj-$(CONFIG_ACPI_SBS)		+= sbs.o
 obj-$(CONFIG_ACPI_POWER_METER)	+= power_meter.o
 obj-$(CONFIG_ACPI_HED)		+= hed.o
 obj-$(CONFIG_ACPI_EC_DEBUGFS)	+= ec_sys.o
+obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o
 
 # processor has its own "processor." module_param namespace
 processor-y			:= processor_driver.o processor_throttling.o
diff --git a/drivers/acpi/debugfs.c b/drivers/acpi/debugfs.c
index e7abc6e..182a9fc 100644
--- a/drivers/acpi/debugfs.c
+++ b/drivers/acpi/debugfs.c
@@ -3,9 +3,6 @@
  */
 
 #include <linux/init.h>
-#include <linux/module.h>
-#include <linux/kernel.h>
-#include <linux/uaccess.h>
 #include <linux/debugfs.h>
 #include <acpi/acpi_drivers.h>
 
@@ -13,84 +10,9 @@
 ACPI_MODULE_NAME("debugfs");
 
 struct dentry *acpi_debugfs_dir;
-static struct dentry *cm_dentry;
-
-/* /sys/kernel/debug/acpi/custom_method */
-
-static ssize_t cm_write(struct file *file, const char __user * user_buf,
-			size_t count, loff_t *ppos)
-{
-	static char *buf;
-	static u32 max_size;
-	static u32 uncopied_bytes;
-
-	struct acpi_table_header table;
-	acpi_status status;
-
-	if (!(*ppos)) {
-		/* parse the table header to get the table length */
-		if (count <= sizeof(struct acpi_table_header))
-			return -EINVAL;
-		if (copy_from_user(&table, user_buf,
-				   sizeof(struct acpi_table_header)))
-			return -EFAULT;
-		uncopied_bytes = max_size = table.length;
-		buf = kzalloc(max_size, GFP_KERNEL);
-		if (!buf)
-			return -ENOMEM;
-	}
-
-	if (buf == NULL)
-		return -EINVAL;
-
-	if ((*ppos > max_size) ||
-	    (*ppos + count > max_size) ||
-	    (*ppos + count < count) ||
-	    (count > uncopied_bytes))
-		return -EINVAL;
-
-	if (copy_from_user(buf + (*ppos), user_buf, count)) {
-		kfree(buf);
-		buf = NULL;
-		return -EFAULT;
-	}
-
-	uncopied_bytes -= count;
-	*ppos += count;
-
-	if (!uncopied_bytes) {
-		status = acpi_install_method(buf);
-		kfree(buf);
-		buf = NULL;
-		if (ACPI_FAILURE(status))
-			return -EINVAL;
-		add_taint(TAINT_OVERRIDDEN_ACPI_TABLE);
-	}
-
-	return count;
-}
-
-static const struct file_operations cm_fops = {
-	.write = cm_write,
-	.llseek = default_llseek,
-};
-
-static int __init acpi_custom_method_init(void)
-{
-	if (!acpi_debugfs_dir)
-		return -ENOENT;
-
-	cm_dentry = debugfs_create_file("custom_method", S_IWUSR,
-					acpi_debugfs_dir, NULL, &cm_fops);
-	if (!cm_dentry)
-		return -ENODEV;
-
-	return 0;
-}
+EXPORT_SYMBOL_GPL(acpi_debugfs_dir);
 
 void __init acpi_debugfs_init(void)
 {
 	acpi_debugfs_dir = debugfs_create_dir("acpi", NULL);
-
-	acpi_custom_method_init();
 }
-- 
1.7.3.4

Re: [PATCH 3/3] acpi: Split out custom_method functionality into an own driver

[Rafael J. Wysocki]

On Thursday, March 31, 2011, Thomas Renninger wrote:
> With /sys/kernel/debug/acpi/custom_method root can write
> to arbitrary memory and increase his priveleges, even if
> these are restricted.
> 
> -> Make this an own debug .config option and warn about the
> security issue in the config description.
> 
> -> Still keep acpi/debugfs.c which now only creates and empty
>    /sys/kernel/debug/acpi directory. There might be other
>    users of it later.
> 
> Signed-off-by: Thomas Renninger <trenn@suse.de>
> CC: Rafael J. Wysocki <rjw@sisk.pl>
> CC: lenb@kernel.org
> CC: rui.zhang@intel.com
> CC: linux-acpi@vger.kernel.org
> ---
>  Documentation/acpi/method-customizing.txt |    5 ++
>  drivers/acpi/Kconfig                      |   15 +++++
>  drivers/acpi/Makefile                     |    1 +
>  drivers/acpi/debugfs.c                    |   80 +----------------------------
>  4 files changed, 22 insertions(+), 79 deletions(-)

Did you forget to custom_method.c ?

Rafael

 
> diff --git a/Documentation/acpi/method-customizing.txt b/Documentation/acpi/method-customizing.txt
> index 3e1d25a..5f55373 100644
> --- a/Documentation/acpi/method-customizing.txt
> +++ b/Documentation/acpi/method-customizing.txt
> @@ -66,3 +66,8 @@ Note: We can use a kernel with multiple custom ACPI method running,
>        But each individual write to debugfs can implement a SINGLE
>        method override. i.e. if we want to insert/override multiple
>        ACPI methods, we need to redo step c) ~ g) for multiple times.
> +
> +Note: Be aware that root can mis-use this driver to modify arbitrary
> +      memory and gain additional rights, if root's privileges got
> +      restricted (for example if root is not allowed to load additional
> +      modules after boot).
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 2aa042a..3feeec8 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -381,6 +381,21 @@ config ACPI_HED
>  	  which is used to report some hardware errors notified via
>  	  SCI, mainly the corrected errors.
>  
> +config ACPI_CUSTOM_METHOD
> +	tristate "Allow ACPI methods to be inserted/replaced at run time"
> +	depends on DEBUG_FS
> +	default n
> +	help
> +	  This debug facility allows ACPI AML methods to me inserted and/or
> +	  replaced without rebooting the system. For details refer to:
> +	  Documentation/acpi/method-customizing.txt.
> +
> +	  NOTE: This option is security sensitive, because it allows arbitrary
> +	  kernel memory to be written to by root (uid=0) users, allowing them
> +	  to bypass certain security measures (e.g. if root is not allowed to
> +	  load additional kernel modules after boot, this feature may be used
> +	  to override that restriction).
> +
>  source "drivers/acpi/apei/Kconfig"
>  
>  endif	# ACPI
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index d113fa5..cba0b23 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -62,6 +62,7 @@ obj-$(CONFIG_ACPI_SBS)		+= sbs.o
>  obj-$(CONFIG_ACPI_POWER_METER)	+= power_meter.o
>  obj-$(CONFIG_ACPI_HED)		+= hed.o
>  obj-$(CONFIG_ACPI_EC_DEBUGFS)	+= ec_sys.o
> +obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o
>  
>  # processor has its own "processor." module_param namespace
>  processor-y			:= processor_driver.o processor_throttling.o
> diff --git a/drivers/acpi/debugfs.c b/drivers/acpi/debugfs.c
> index e7abc6e..182a9fc 100644
> --- a/drivers/acpi/debugfs.c
> +++ b/drivers/acpi/debugfs.c
> @@ -3,9 +3,6 @@
>   */
>  
>  #include <linux/init.h>
> -#include <linux/module.h>
> -#include <linux/kernel.h>
> -#include <linux/uaccess.h>
>  #include <linux/debugfs.h>
>  #include <acpi/acpi_drivers.h>
>  
> @@ -13,84 +10,9 @@
>  ACPI_MODULE_NAME("debugfs");
>  
>  struct dentry *acpi_debugfs_dir;
> -static struct dentry *cm_dentry;
> -
> -/* /sys/kernel/debug/acpi/custom_method */
> -
> -static ssize_t cm_write(struct file *file, const char __user * user_buf,
> -			size_t count, loff_t *ppos)
> -{
> -	static char *buf;
> -	static u32 max_size;
> -	static u32 uncopied_bytes;
> -
> -	struct acpi_table_header table;
> -	acpi_status status;
> -
> -	if (!(*ppos)) {
> -		/* parse the table header to get the table length */
> -		if (count <= sizeof(struct acpi_table_header))
> -			return -EINVAL;
> -		if (copy_from_user(&table, user_buf,
> -				   sizeof(struct acpi_table_header)))
> -			return -EFAULT;
> -		uncopied_bytes = max_size = table.length;
> -		buf = kzalloc(max_size, GFP_KERNEL);
> -		if (!buf)
> -			return -ENOMEM;
> -	}
> -
> -	if (buf == NULL)
> -		return -EINVAL;
> -
> -	if ((*ppos > max_size) ||
> -	    (*ppos + count > max_size) ||
> -	    (*ppos + count < count) ||
> -	    (count > uncopied_bytes))
> -		return -EINVAL;
> -
> -	if (copy_from_user(buf + (*ppos), user_buf, count)) {
> -		kfree(buf);
> -		buf = NULL;
> -		return -EFAULT;
> -	}
> -
> -	uncopied_bytes -= count;
> -	*ppos += count;
> -
> -	if (!uncopied_bytes) {
> -		status = acpi_install_method(buf);
> -		kfree(buf);
> -		buf = NULL;
> -		if (ACPI_FAILURE(status))
> -			return -EINVAL;
> -		add_taint(TAINT_OVERRIDDEN_ACPI_TABLE);
> -	}
> -
> -	return count;
> -}
> -
> -static const struct file_operations cm_fops = {
> -	.write = cm_write,
> -	.llseek = default_llseek,
> -};
> -
> -static int __init acpi_custom_method_init(void)
> -{
> -	if (!acpi_debugfs_dir)
> -		return -ENOENT;
> -
> -	cm_dentry = debugfs_create_file("custom_method", S_IWUSR,
> -					acpi_debugfs_dir, NULL, &cm_fops);
> -	if (!cm_dentry)
> -		return -ENODEV;
> -
> -	return 0;
> -}
> +EXPORT_SYMBOL_GPL(acpi_debugfs_dir);
>  
>  void __init acpi_debugfs_init(void)
>  {
>  	acpi_debugfs_dir = debugfs_create_dir("acpi", NULL);
> -
> -	acpi_custom_method_init();
>  }
> 

Re: [PATCH 3/3] acpi: Split out custom_method functionality into an own driver

[Thomas Renninger]

On Thursday, March 31, 2011 11:41:24 PM Rafael J. Wysocki wrote:
> On Thursday, March 31, 2011, Thomas Renninger wrote:
> > With /sys/kernel/debug/acpi/custom_method root can write
> > to arbitrary memory and increase his priveleges, even if
> > these are restricted.
> > 
> > -> Make this an own debug .config option and warn about the
> > security issue in the config description.
> > 
> > -> Still keep acpi/debugfs.c which now only creates and empty
> >    /sys/kernel/debug/acpi directory. There might be other
> >    users of it later.
> > 
> > Signed-off-by: Thomas Renninger <trenn@suse.de>
> > CC: Rafael J. Wysocki <rjw@sisk.pl>
> > CC: lenb@kernel.org
> > CC: rui.zhang@intel.com
> > CC: linux-acpi@vger.kernel.org
> > ---
> >  Documentation/acpi/method-customizing.txt |    5 ++
> >  drivers/acpi/Kconfig                      |   15 +++++
> >  drivers/acpi/Makefile                     |    1 +
> >  drivers/acpi/debugfs.c                    |   80 
+----------------------------
> >  4 files changed, 22 insertions(+), 79 deletions(-)
> 
> Did you forget to custom_method.c ?
Indeed. Happened already, looks like I need a guilt add
again after guilt push -f, no idea.

Resend coming in a second.

Thanks,

   Thomas

[PATCH 3/3] ACPI: Split out custom_method functionality into an own driver

[Thomas Renninger]

With /sys/kernel/debug/acpi/custom_method root can write
to arbitrary memory and increase his priveleges, even if
these are restricted.

-> Make this an own debug .config option and warn about the
security issue in the config description.

-> Still keep acpi/debugfs.c which now only creates an empty
   /sys/kernel/debug/acpi directory. There might be other
   users of it later.

Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: Rafael J. Wysocki <rjw@sisk.pl>
CC: lenb@kernel.org
CC: rui.zhang@intel.com
CC: linux-acpi@vger.kernel.org

---
 Documentation/acpi/method-customizing.txt |    5 ++
 drivers/acpi/Kconfig                      |   15 ++++
 drivers/acpi/Makefile                     |    1 +
 drivers/acpi/custom_method.c              |  100 +++++++++++++++++++++++++++++
 drivers/acpi/debugfs.c                    |   80 +-----------------------
 5 files changed, 122 insertions(+), 79 deletions(-)

diff --git a/Documentation/acpi/method-customizing.txt b/Documentation/acpi/method-customizing.txt
index 3e1d25a..5f55373 100644
--- a/Documentation/acpi/method-customizing.txt
+++ b/Documentation/acpi/method-customizing.txt
@@ -66,3 +66,8 @@ Note: We can use a kernel with multiple custom ACPI method running,
       But each individual write to debugfs can implement a SINGLE
       method override. i.e. if we want to insert/override multiple
       ACPI methods, we need to redo step c) ~ g) for multiple times.
+
+Note: Be aware that root can mis-use this driver to modify arbitrary
+      memory and gain additional rights, if root's privileges got
+      restricted (for example if root is not allowed to load additional
+      modules after boot).
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 2aa042a..3feeec8 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -381,6 +381,21 @@ config ACPI_HED
 	  which is used to report some hardware errors notified via
 	  SCI, mainly the corrected errors.
 
+config ACPI_CUSTOM_METHOD
+	tristate "Allow ACPI methods to be inserted/replaced at run time"
+	depends on DEBUG_FS
+	default n
+	help
+	  This debug facility allows ACPI AML methods to me inserted and/or
+	  replaced without rebooting the system. For details refer to:
+	  Documentation/acpi/method-customizing.txt.
+
+	  NOTE: This option is security sensitive, because it allows arbitrary
+	  kernel memory to be written to by root (uid=0) users, allowing them
+	  to bypass certain security measures (e.g. if root is not allowed to
+	  load additional kernel modules after boot, this feature may be used
+	  to override that restriction).
+
 source "drivers/acpi/apei/Kconfig"
 
 endif	# ACPI
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index d113fa5..cba0b23 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -62,6 +62,7 @@ obj-$(CONFIG_ACPI_SBS)		+= sbs.o
 obj-$(CONFIG_ACPI_POWER_METER)	+= power_meter.o
 obj-$(CONFIG_ACPI_HED)		+= hed.o
 obj-$(CONFIG_ACPI_EC_DEBUGFS)	+= ec_sys.o
+obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o
 
 # processor has its own "processor." module_param namespace
 processor-y			:= processor_driver.o processor_throttling.o
diff --git a/drivers/acpi/custom_method.c b/drivers/acpi/custom_method.c
new file mode 100644
index 0000000..dc554c2
--- /dev/null
+++ b/drivers/acpi/custom_method.c
@@ -0,0 +1,100 @@
+/*
+ * debugfs.c - ACPI debugfs interface to userspace.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/uaccess.h>
+#include <linux/debugfs.h>
+#include <acpi/acpi_drivers.h>
+
+#include "internal.h"
+
+#define _COMPONENT		ACPI_SYSTEM_COMPONENT
+ACPI_MODULE_NAME("custom_method");
+MODULE_LICENSE("GPL");
+
+static struct dentry *cm_dentry;
+
+/* /sys/kernel/debug/acpi/custom_method */
+
+static ssize_t cm_write(struct file *file, const char __user * user_buf,
+			size_t count, loff_t *ppos)
+{
+	static char *buf;
+	static u32 max_size;
+	static u32 uncopied_bytes;
+
+	struct acpi_table_header table;
+	acpi_status status;
+
+	if (!(*ppos)) {
+		/* parse the table header to get the table length */
+		if (count <= sizeof(struct acpi_table_header))
+			return -EINVAL;
+		if (copy_from_user(&table, user_buf,
+				   sizeof(struct acpi_table_header)))
+			return -EFAULT;
+		uncopied_bytes = max_size = table.length;
+		buf = kzalloc(max_size, GFP_KERNEL);
+		if (!buf)
+			return -ENOMEM;
+	}
+
+	if (buf == NULL)
+		return -EINVAL;
+
+	if ((*ppos > max_size) ||
+	    (*ppos + count > max_size) ||
+	    (*ppos + count < count) ||
+	    (count > uncopied_bytes))
+		return -EINVAL;
+
+	if (copy_from_user(buf + (*ppos), user_buf, count)) {
+		kfree(buf);
+		buf = NULL;
+		return -EFAULT;
+	}
+
+	uncopied_bytes -= count;
+	*ppos += count;
+
+	if (!uncopied_bytes) {
+		status = acpi_install_method(buf);
+		kfree(buf);
+		buf = NULL;
+		if (ACPI_FAILURE(status))
+			return -EINVAL;
+		add_taint(TAINT_OVERRIDDEN_ACPI_TABLE);
+	}
+
+	return count;
+}
+
+static const struct file_operations cm_fops = {
+	.write = cm_write,
+	.llseek = default_llseek,
+};
+
+static int __init acpi_custom_method_init(void)
+{
+	if (acpi_debugfs_dir == NULL)
+		return -ENOENT;
+
+ 	cm_dentry = debugfs_create_file("custom_method", S_IWUSR,
+					acpi_debugfs_dir, NULL, &cm_fops);
+	if (cm_dentry == NULL)
+		return -ENODEV;
+
+ 	return 0;
+}
+
+static void __exit acpi_custom_method_exit(void)
+{
+	if (cm_dentry)
+		debugfs_remove(cm_dentry);
+ }
+
+module_init(acpi_custom_method_init);
+module_exit(acpi_custom_method_exit);
diff --git a/drivers/acpi/debugfs.c b/drivers/acpi/debugfs.c
index e7abc6e..182a9fc 100644
--- a/drivers/acpi/debugfs.c
+++ b/drivers/acpi/debugfs.c
@@ -3,9 +3,6 @@
  */
 
 #include <linux/init.h>
-#include <linux/module.h>
-#include <linux/kernel.h>
-#include <linux/uaccess.h>
 #include <linux/debugfs.h>
 #include <acpi/acpi_drivers.h>
 
@@ -13,84 +10,9 @@
 ACPI_MODULE_NAME("debugfs");
 
 struct dentry *acpi_debugfs_dir;
-static struct dentry *cm_dentry;
-
-/* /sys/kernel/debug/acpi/custom_method */
-
-static ssize_t cm_write(struct file *file, const char __user * user_buf,
-			size_t count, loff_t *ppos)
-{
-	static char *buf;
-	static u32 max_size;
-	static u32 uncopied_bytes;
-
-	struct acpi_table_header table;
-	acpi_status status;
-
-	if (!(*ppos)) {
-		/* parse the table header to get the table length */
-		if (count <= sizeof(struct acpi_table_header))
-			return -EINVAL;
-		if (copy_from_user(&table, user_buf,
-				   sizeof(struct acpi_table_header)))
-			return -EFAULT;
-		uncopied_bytes = max_size = table.length;
-		buf = kzalloc(max_size, GFP_KERNEL);
-		if (!buf)
-			return -ENOMEM;
-	}
-
-	if (buf == NULL)
-		return -EINVAL;
-
-	if ((*ppos > max_size) ||
-	    (*ppos + count > max_size) ||
-	    (*ppos + count < count) ||
-	    (count > uncopied_bytes))
-		return -EINVAL;
-
-	if (copy_from_user(buf + (*ppos), user_buf, count)) {
-		kfree(buf);
-		buf = NULL;
-		return -EFAULT;
-	}
-
-	uncopied_bytes -= count;
-	*ppos += count;
-
-	if (!uncopied_bytes) {
-		status = acpi_install_method(buf);
-		kfree(buf);
-		buf = NULL;
-		if (ACPI_FAILURE(status))
-			return -EINVAL;
-		add_taint(TAINT_OVERRIDDEN_ACPI_TABLE);
-	}
-
-	return count;
-}
-
-static const struct file_operations cm_fops = {
-	.write = cm_write,
-	.llseek = default_llseek,
-};
-
-static int __init acpi_custom_method_init(void)
-{
-	if (!acpi_debugfs_dir)
-		return -ENOENT;
-
-	cm_dentry = debugfs_create_file("custom_method", S_IWUSR,
-					acpi_debugfs_dir, NULL, &cm_fops);
-	if (!cm_dentry)
-		return -ENODEV;
-
-	return 0;
-}
+EXPORT_SYMBOL_GPL(acpi_debugfs_dir);
 
 void __init acpi_debugfs_init(void)
 {
 	acpi_debugfs_dir = debugfs_create_dir("acpi", NULL);
-
-	acpi_custom_method_init();
 }

Re: [PATCH 3/3] ACPI: Split out custom_method functionality into an own driver

[Rafael J. Wysocki]

On Friday, April 01, 2011, Thomas Renninger wrote:
> With /sys/kernel/debug/acpi/custom_method root can write
> to arbitrary memory and increase his priveleges, even if
> these are restricted.
> 
> -> Make this an own debug .config option and warn about the
> security issue in the config description.
> 
> -> Still keep acpi/debugfs.c which now only creates an empty
>    /sys/kernel/debug/acpi directory. There might be other
>    users of it later.
> 
> Signed-off-by: Thomas Renninger <trenn@suse.de>

Acked-by: Rafael J. Wysocki <rjw@sisk.pl>

> CC: lenb@kernel.org
> CC: rui.zhang@intel.com
> CC: linux-acpi@vger.kernel.org
> 
> ---
>  Documentation/acpi/method-customizing.txt |    5 ++
>  drivers/acpi/Kconfig                      |   15 ++++
>  drivers/acpi/Makefile                     |    1 +
>  drivers/acpi/custom_method.c              |  100 +++++++++++++++++++++++++++++
>  drivers/acpi/debugfs.c                    |   80 +-----------------------
>  5 files changed, 122 insertions(+), 79 deletions(-)
> 
> diff --git a/Documentation/acpi/method-customizing.txt b/Documentation/acpi/method-customizing.txt
> index 3e1d25a..5f55373 100644
> --- a/Documentation/acpi/method-customizing.txt
> +++ b/Documentation/acpi/method-customizing.txt
> @@ -66,3 +66,8 @@ Note: We can use a kernel with multiple custom ACPI method running,
>        But each individual write to debugfs can implement a SINGLE
>        method override. i.e. if we want to insert/override multiple
>        ACPI methods, we need to redo step c) ~ g) for multiple times.
> +
> +Note: Be aware that root can mis-use this driver to modify arbitrary
> +      memory and gain additional rights, if root's privileges got
> +      restricted (for example if root is not allowed to load additional
> +      modules after boot).
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 2aa042a..3feeec8 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -381,6 +381,21 @@ config ACPI_HED
>  	  which is used to report some hardware errors notified via
>  	  SCI, mainly the corrected errors.
>  
> +config ACPI_CUSTOM_METHOD
> +	tristate "Allow ACPI methods to be inserted/replaced at run time"
> +	depends on DEBUG_FS
> +	default n
> +	help
> +	  This debug facility allows ACPI AML methods to me inserted and/or
> +	  replaced without rebooting the system. For details refer to:
> +	  Documentation/acpi/method-customizing.txt.
> +
> +	  NOTE: This option is security sensitive, because it allows arbitrary
> +	  kernel memory to be written to by root (uid=0) users, allowing them
> +	  to bypass certain security measures (e.g. if root is not allowed to
> +	  load additional kernel modules after boot, this feature may be used
> +	  to override that restriction).
> +
>  source "drivers/acpi/apei/Kconfig"
>  
>  endif	# ACPI
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index d113fa5..cba0b23 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -62,6 +62,7 @@ obj-$(CONFIG_ACPI_SBS)		+= sbs.o
>  obj-$(CONFIG_ACPI_POWER_METER)	+= power_meter.o
>  obj-$(CONFIG_ACPI_HED)		+= hed.o
>  obj-$(CONFIG_ACPI_EC_DEBUGFS)	+= ec_sys.o
> +obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o
>  
>  # processor has its own "processor." module_param namespace
>  processor-y			:= processor_driver.o processor_throttling.o
> diff --git a/drivers/acpi/custom_method.c b/drivers/acpi/custom_method.c
> new file mode 100644
> index 0000000..dc554c2
> --- /dev/null
> +++ b/drivers/acpi/custom_method.c
> @@ -0,0 +1,100 @@
> +/*
> + * debugfs.c - ACPI debugfs interface to userspace.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/uaccess.h>
> +#include <linux/debugfs.h>
> +#include <acpi/acpi_drivers.h>
> +
> +#include "internal.h"
> +
> +#define _COMPONENT		ACPI_SYSTEM_COMPONENT
> +ACPI_MODULE_NAME("custom_method");
> +MODULE_LICENSE("GPL");
> +
> +static struct dentry *cm_dentry;
> +
> +/* /sys/kernel/debug/acpi/custom_method */
> +
> +static ssize_t cm_write(struct file *file, const char __user * user_buf,
> +			size_t count, loff_t *ppos)
> +{
> +	static char *buf;
> +	static u32 max_size;
> +	static u32 uncopied_bytes;
> +
> +	struct acpi_table_header table;
> +	acpi_status status;
> +
> +	if (!(*ppos)) {
> +		/* parse the table header to get the table length */
> +		if (count <= sizeof(struct acpi_table_header))
> +			return -EINVAL;
> +		if (copy_from_user(&table, user_buf,
> +				   sizeof(struct acpi_table_header)))
> +			return -EFAULT;
> +		uncopied_bytes = max_size = table.length;
> +		buf = kzalloc(max_size, GFP_KERNEL);
> +		if (!buf)
> +			return -ENOMEM;
> +	}
> +
> +	if (buf == NULL)
> +		return -EINVAL;
> +
> +	if ((*ppos > max_size) ||
> +	    (*ppos + count > max_size) ||
> +	    (*ppos + count < count) ||
> +	    (count > uncopied_bytes))
> +		return -EINVAL;
> +
> +	if (copy_from_user(buf + (*ppos), user_buf, count)) {
> +		kfree(buf);
> +		buf = NULL;
> +		return -EFAULT;
> +	}
> +
> +	uncopied_bytes -= count;
> +	*ppos += count;
> +
> +	if (!uncopied_bytes) {
> +		status = acpi_install_method(buf);
> +		kfree(buf);
> +		buf = NULL;
> +		if (ACPI_FAILURE(status))
> +			return -EINVAL;
> +		add_taint(TAINT_OVERRIDDEN_ACPI_TABLE);
> +	}
> +
> +	return count;
> +}
> +
> +static const struct file_operations cm_fops = {
> +	.write = cm_write,
> +	.llseek = default_llseek,
> +};
> +
> +static int __init acpi_custom_method_init(void)
> +{
> +	if (acpi_debugfs_dir == NULL)
> +		return -ENOENT;
> +
> + 	cm_dentry = debugfs_create_file("custom_method", S_IWUSR,
> +					acpi_debugfs_dir, NULL, &cm_fops);
> +	if (cm_dentry == NULL)
> +		return -ENODEV;
> +
> + 	return 0;
> +}
> +
> +static void __exit acpi_custom_method_exit(void)
> +{
> +	if (cm_dentry)
> +		debugfs_remove(cm_dentry);
> + }
> +
> +module_init(acpi_custom_method_init);
> +module_exit(acpi_custom_method_exit);
> diff --git a/drivers/acpi/debugfs.c b/drivers/acpi/debugfs.c
> index e7abc6e..182a9fc 100644
> --- a/drivers/acpi/debugfs.c
> +++ b/drivers/acpi/debugfs.c
> @@ -3,9 +3,6 @@
>   */
>  
>  #include <linux/init.h>
> -#include <linux/module.h>
> -#include <linux/kernel.h>
> -#include <linux/uaccess.h>
>  #include <linux/debugfs.h>
>  #include <acpi/acpi_drivers.h>
>  
> @@ -13,84 +10,9 @@
>  ACPI_MODULE_NAME("debugfs");
>  
>  struct dentry *acpi_debugfs_dir;
> -static struct dentry *cm_dentry;
> -
> -/* /sys/kernel/debug/acpi/custom_method */
> -
> -static ssize_t cm_write(struct file *file, const char __user * user_buf,
> -			size_t count, loff_t *ppos)
> -{
> -	static char *buf;
> -	static u32 max_size;
> -	static u32 uncopied_bytes;
> -
> -	struct acpi_table_header table;
> -	acpi_status status;
> -
> -	if (!(*ppos)) {
> -		/* parse the table header to get the table length */
> -		if (count <= sizeof(struct acpi_table_header))
> -			return -EINVAL;
> -		if (copy_from_user(&table, user_buf,
> -				   sizeof(struct acpi_table_header)))
> -			return -EFAULT;
> -		uncopied_bytes = max_size = table.length;
> -		buf = kzalloc(max_size, GFP_KERNEL);
> -		if (!buf)
> -			return -ENOMEM;
> -	}
> -
> -	if (buf == NULL)
> -		return -EINVAL;
> -
> -	if ((*ppos > max_size) ||
> -	    (*ppos + count > max_size) ||
> -	    (*ppos + count < count) ||
> -	    (count > uncopied_bytes))
> -		return -EINVAL;
> -
> -	if (copy_from_user(buf + (*ppos), user_buf, count)) {
> -		kfree(buf);
> -		buf = NULL;
> -		return -EFAULT;
> -	}
> -
> -	uncopied_bytes -= count;
> -	*ppos += count;
> -
> -	if (!uncopied_bytes) {
> -		status = acpi_install_method(buf);
> -		kfree(buf);
> -		buf = NULL;
> -		if (ACPI_FAILURE(status))
> -			return -EINVAL;
> -		add_taint(TAINT_OVERRIDDEN_ACPI_TABLE);
> -	}
> -
> -	return count;
> -}
> -
> -static const struct file_operations cm_fops = {
> -	.write = cm_write,
> -	.llseek = default_llseek,
> -};
> -
> -static int __init acpi_custom_method_init(void)
> -{
> -	if (!acpi_debugfs_dir)
> -		return -ENOENT;
> -
> -	cm_dentry = debugfs_create_file("custom_method", S_IWUSR,
> -					acpi_debugfs_dir, NULL, &cm_fops);
> -	if (!cm_dentry)
> -		return -ENODEV;
> -
> -	return 0;
> -}
> +EXPORT_SYMBOL_GPL(acpi_debugfs_dir);
>  
>  void __init acpi_debugfs_init(void)
>  {
>  	acpi_debugfs_dir = debugfs_create_dir("acpi", NULL);
> -
> -	acpi_custom_method_init();
>  }
> 
>