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

[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

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 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 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 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]

- Move param aml_debug_output to other params into sysfs.c
- 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  |   32 +++++++++++++-------------------
 drivers/acpi/internal.h |    3 ++-
 drivers/acpi/sysfs.c    |    8 ++++++++
 3 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/drivers/acpi/debugfs.c b/drivers/acpi/debugfs.c
index 384f7ab..e7abc6e 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;
-
-	acpi_dir = debugfs_create_dir("acpi", NULL);
-	if (!acpi_dir)
-		goto err;
+	if (!acpi_debugfs_dir)
+		return -ENOENT;
 
 	cm_dentry = debugfs_create_file("custom_method", S_IWUSR,
-					acpi_dir, NULL, &cm_fops);
+					acpi_debugfs_dir, NULL, &cm_fops);
 	if (!cm_dentry)
-		goto err;
+		return -ENODEV;
 
 	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
 
 /* --------------------------------------------------------------------------
diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
index 61891e7..77255f2 100644
--- a/drivers/acpi/sysfs.c
+++ b/drivers/acpi/sysfs.c
@@ -220,6 +220,14 @@ module_param_call(trace_state, param_set_trace_state, param_get_trace_state,
 		  NULL, 0644);
 #endif /* CONFIG_ACPI_DEBUG */
 
+
+/* /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.");
+
 /* /sys/module/acpi/parameters/acpica_version */
 static int param_get_acpica_version(char *buffer, struct kernel_param *kp)
 {
-- 
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                      |   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 2/3] acpi: Cleanup custom_method debug stuff

[Rafael J. Wysocki]

On Thursday, March 31, 2011, Thomas Renninger wrote:
> - Move param aml_debug_output to other params into sysfs.c
> - Split acpi_debugfs_init to prepare custom_method to be
>   an own .config option and driver.
> 
> 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
> ---
>  drivers/acpi/debugfs.c  |   32 +++++++++++++-------------------
>  drivers/acpi/internal.h |    3 ++-
>  drivers/acpi/sysfs.c    |    8 ++++++++
>  3 files changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/acpi/debugfs.c b/drivers/acpi/debugfs.c
> index 384f7ab..e7abc6e 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;
> -
> -	acpi_dir = debugfs_create_dir("acpi", NULL);
> -	if (!acpi_dir)
> -		goto err;
> +	if (!acpi_debugfs_dir)
> +		return -ENOENT;
>  
>  	cm_dentry = debugfs_create_file("custom_method", S_IWUSR,
> -					acpi_dir, NULL, &cm_fops);
> +					acpi_debugfs_dir, NULL, &cm_fops);
>  	if (!cm_dentry)
> -		goto err;
> +		return -ENODEV;
>  
>  	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
>  
>  /* --------------------------------------------------------------------------
> diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
> index 61891e7..77255f2 100644
> --- a/drivers/acpi/sysfs.c
> +++ b/drivers/acpi/sysfs.c
> @@ -220,6 +220,14 @@ module_param_call(trace_state, param_set_trace_state, param_get_trace_state,
>  		  NULL, 0644);
>  #endif /* CONFIG_ACPI_DEBUG */
>  
> +
> +/* /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.");
> +
>  /* /sys/module/acpi/parameters/acpica_version */
>  static int param_get_acpica_version(char *buffer, struct kernel_param *kp)
>  {
> 

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 1/3] acpi ec: Cleanup unused stuff

[Len Brown]

applied

thanks,
Len Brown, Intel Open Source Technology Center

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

[Zhang Rui]

On Fri, 2011-04-01 at 05:39 +0800, Rafael J. Wysocki wrote:
> On Thursday, March 31, 2011, Thomas Renninger wrote:
> > - Move param aml_debug_output to other params into sysfs.c
> > - Split acpi_debugfs_init to prepare custom_method to be
> >   an own .config option and driver.
> > 
> > Signed-off-by: Thomas Renninger <trenn@suse.de>
> 
> Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
> 
Acked-by: Zhang Rui <rui.zhang@intel.com>
> > CC: lenb@kernel.org
> > CC: rui.zhang@intel.com
> > CC: linux-acpi@vger.kernel.org
> > ---
> >  drivers/acpi/debugfs.c  |   32 +++++++++++++-------------------
> >  drivers/acpi/internal.h |    3 ++-
> >  drivers/acpi/sysfs.c    |    8 ++++++++
> >  3 files changed, 23 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/acpi/debugfs.c b/drivers/acpi/debugfs.c
> > index 384f7ab..e7abc6e 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;
> > -
> > -	acpi_dir = debugfs_create_dir("acpi", NULL);
> > -	if (!acpi_dir)
> > -		goto err;
> > +	if (!acpi_debugfs_dir)
> > +		return -ENOENT;
> >  
> >  	cm_dentry = debugfs_create_file("custom_method", S_IWUSR,
> > -					acpi_dir, NULL, &cm_fops);
> > +					acpi_debugfs_dir, NULL, &cm_fops);
> >  	if (!cm_dentry)
> > -		goto err;
> > +		return -ENODEV;
> >  
> >  	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
> >  
> >  /* --------------------------------------------------------------------------
> > diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
> > index 61891e7..77255f2 100644
> > --- a/drivers/acpi/sysfs.c
> > +++ b/drivers/acpi/sysfs.c
> > @@ -220,6 +220,14 @@ module_param_call(trace_state, param_set_trace_state, param_get_trace_state,
> >  		  NULL, 0644);
> >  #endif /* CONFIG_ACPI_DEBUG */
> >  
> > +
> > +/* /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.");
> > +
> >  /* /sys/module/acpi/parameters/acpica_version */
> >  static int param_get_acpica_version(char *buffer, struct kernel_param *kp)
> >  {
> > 
> 

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

[Len Brown]

applied

thanks,
Len Brown, Intel Open Source Technology Center

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();
>  }
> 
> 

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

[Thomas Renninger]

On Friday, April 01, 2011 08:38:10 AM Len Brown wrote:
> applied
> 
Eh, Len, can you double check, the patch(es) were not applied?
I checked against your ACPI test branch.

Best probably is I repost these 3 patches, will do so.

Thanks,

   Thomas