[PATCH v2 1/3] power: validate wakeup source before activating it.

[PATCH v2 1/3] power: validate wakeup source before activating it.

[Jin Qian]

A rogue wakeup source not registered in wakeup_sources list is not visible
from wakeup_sources_stats_show. Check if the wakeup source is registered
properly by looking at the timer function.

Signed-off-by: Jin Qian <jinqian@android.com>
---
 drivers/base/power/wakeup.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index 7726200..5d866b9 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -14,6 +14,7 @@
 #include <linux/suspend.h>
 #include <linux/seq_file.h>
 #include <linux/debugfs.h>
+#include <linux/types.h>
 #include <trace/events/power.h>
 
 #include "power.h"
@@ -351,6 +352,19 @@ int device_set_wakeup_enable(struct device *dev, bool enable)
 }
 EXPORT_SYMBOL_GPL(device_set_wakeup_enable);
 
+/**
+ * validate_wakeup_source - validate the given wakeup source.
+ * @ws: Wakeup source to be validated.
+ */
+static bool validate_wakeup_source(struct wakeup_source *ws)
+{
+	/*
+	 * Use timer function to check if the given source is initialized
+	 * by wakeup_source_add.
+	 */
+	return ws->timer.function == pm_wakeup_timer_fn;
+}
+
 /*
  * The functions below use the observation that each wakeup event starts a
  * period in which the system should not be suspended.  The moment this period
@@ -391,6 +405,11 @@ static void wakeup_source_activate(struct wakeup_source *ws)
 {
 	unsigned int cec;
 
+	if (WARN_ON(!validate_wakeup_source(ws))) {
+		pr_err("unregistered wakeup source: %s\n", ws->name);
+		return;
+	}
+
 	/*
 	 * active wakeup source should bring the system
 	 * out of PM_SUSPEND_FREEZE state
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH v2 1/3] power: validate wakeup source before activating it.

[Rafael J. Wysocki]

On Tuesday, May 05, 2015 03:24:13 PM Jin Qian wrote:
> A rogue wakeup source not registered in wakeup_sources list is not visible
> from wakeup_sources_stats_show. Check if the wakeup source is registered
> properly by looking at the timer function.
> 
> Signed-off-by: Jin Qian <jinqian@android.com>
> ---
>  drivers/base/power/wakeup.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index 7726200..5d866b9 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -14,6 +14,7 @@
>  #include <linux/suspend.h>
>  #include <linux/seq_file.h>
>  #include <linux/debugfs.h>
> +#include <linux/types.h>

What's that for?

>  #include <trace/events/power.h>
>  
>  #include "power.h"
> @@ -351,6 +352,19 @@ int device_set_wakeup_enable(struct device *dev, bool enable)
>  }
>  EXPORT_SYMBOL_GPL(device_set_wakeup_enable);
>  
> +/**
> + * validate_wakeup_source - validate the given wakeup source.
> + * @ws: Wakeup source to be validated.
> + */
> +static bool validate_wakeup_source(struct wakeup_source *ws)

I would call that wakeup_source_not_registered() and do the reverse check.
The extra logical negation below wouldn't be necessary then.

> +{
> +	/*
> +	 * Use timer function to check if the given source is initialized
> +	 * by wakeup_source_add.
> +	 */
> +	return ws->timer.function == pm_wakeup_timer_fn;

You can also verify if timer.data is equal to ws.

> +}
> +
>  /*
>   * The functions below use the observation that each wakeup event starts a
>   * period in which the system should not be suspended.  The moment this period
> @@ -391,6 +405,11 @@ static void wakeup_source_activate(struct wakeup_source *ws)
>  {
>  	unsigned int cec;
>  
> +	if (WARN_ON(!validate_wakeup_source(ws))) {

WARN_ONCE() would be better, so you can make it print the message and you
won't need the extra statement.

> +		pr_err("unregistered wakeup source: %s\n", ws->name);
> +		return;
> +	}
> +
>  	/*
>  	 * active wakeup source should bring the system
>  	 * out of PM_SUSPEND_FREEZE state
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH v2 1/3] power: validate wakeup source before activating it.

[Jin Qian]

A rogue wakeup source not registered in wakeup_sources list is not visible
from wakeup_sources_stats_show. Check if the wakeup source is registered
properly by looking at the timer struct.

Signed-off-by: Jin Qian <jinqian@android.com>
---
 drivers/base/power/wakeup.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index 7726200..aead738 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -351,6 +351,20 @@ int device_set_wakeup_enable(struct device *dev, bool enable)
 }
 EXPORT_SYMBOL_GPL(device_set_wakeup_enable);
 
+/**
+ * wakeup_source_not_registered - validate the given wakeup source.
+ * @ws: Wakeup source to be validated.
+ */
+static bool wakeup_source_not_registered(struct wakeup_source *ws)
+{
+	/*
+	 * Use timer struct to check if the given source is initialized
+	 * by wakeup_source_add.
+	 */
+	return ws->timer.function != pm_wakeup_timer_fn ||
+		   ws->timer.data != (unsigned long)ws;
+}
+
 /*
  * The functions below use the observation that each wakeup event starts a
  * period in which the system should not be suspended.  The moment this period
@@ -391,6 +405,11 @@ static void wakeup_source_activate(struct wakeup_source *ws)
 {
 	unsigned int cec;
 
+	if (WARN_ONCE(wakeup_source_not_registered(ws),
+			"unregistered wakeup source\n")) {
+		return;
+	}
+
 	/*
 	 * active wakeup source should bring the system
 	 * out of PM_SUSPEND_FREEZE state
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH v2 1/3] power: validate wakeup source before activating it.

[Rafael J. Wysocki]

On Wednesday, May 06, 2015 11:51:07 AM Jin Qian wrote:
> A rogue wakeup source not registered in wakeup_sources list is not visible
> from wakeup_sources_stats_show. Check if the wakeup source is registered
> properly by looking at the timer struct.
> 
> Signed-off-by: Jin Qian <jinqian@android.com>
> ---
>  drivers/base/power/wakeup.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index 7726200..aead738 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -351,6 +351,20 @@ int device_set_wakeup_enable(struct device *dev, bool enable)
>  }
>  EXPORT_SYMBOL_GPL(device_set_wakeup_enable);
>  
> +/**
> + * wakeup_source_not_registered - validate the given wakeup source.
> + * @ws: Wakeup source to be validated.
> + */
> +static bool wakeup_source_not_registered(struct wakeup_source *ws)
> +{
> +	/*
> +	 * Use timer struct to check if the given source is initialized
> +	 * by wakeup_source_add.
> +	 */
> +	return ws->timer.function != pm_wakeup_timer_fn ||
> +		   ws->timer.data != (unsigned long)ws;
> +}
> +
>  /*
>   * The functions below use the observation that each wakeup event starts a
>   * period in which the system should not be suspended.  The moment this period
> @@ -391,6 +405,11 @@ static void wakeup_source_activate(struct wakeup_source *ws)
>  {
>  	unsigned int cec;
>  
> +	if (WARN_ONCE(wakeup_source_not_registered(ws),
> +			"unregistered wakeup source\n")) {
> +		return;
> +	}

The braces are not necessary and you can use format strings with WARN_ONCE()
too IIRC.

> +
>  	/*
>  	 * active wakeup source should bring the system
>  	 * out of PM_SUSPEND_FREEZE state
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v2 1/3] power: validate wakeup source before activating it.

[Jin Qian]

Hi Rafael,

I kept braces since checkpatch.pl didn't report error or warning. The
if condition has two lines, a brace make the statement more clear I
think.

I removed the format string since ws->name can be random strings if
the wakeup_source is not registered (i.e. using prepare functions).
It's not very useful to print name if we have stack dump from
WARN_ONCE.

Thanks,
jin

On Wed, May 6, 2015 at 3:00 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, May 06, 2015 11:51:07 AM Jin Qian wrote:
>> A rogue wakeup source not registered in wakeup_sources list is not visible
>> from wakeup_sources_stats_show. Check if the wakeup source is registered
>> properly by looking at the timer struct.
>>
>> Signed-off-by: Jin Qian <jinqian@android.com>
>> ---
>>  drivers/base/power/wakeup.c | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
>> index 7726200..aead738 100644
>> --- a/drivers/base/power/wakeup.c
>> +++ b/drivers/base/power/wakeup.c
>> @@ -351,6 +351,20 @@ int device_set_wakeup_enable(struct device *dev, bool enable)
>>  }
>>  EXPORT_SYMBOL_GPL(device_set_wakeup_enable);
>>
>> +/**
>> + * wakeup_source_not_registered - validate the given wakeup source.
>> + * @ws: Wakeup source to be validated.
>> + */
>> +static bool wakeup_source_not_registered(struct wakeup_source *ws)
>> +{
>> +     /*
>> +      * Use timer struct to check if the given source is initialized
>> +      * by wakeup_source_add.
>> +      */
>> +     return ws->timer.function != pm_wakeup_timer_fn ||
>> +                ws->timer.data != (unsigned long)ws;
>> +}
>> +
>>  /*
>>   * The functions below use the observation that each wakeup event starts a
>>   * period in which the system should not be suspended.  The moment this period
>> @@ -391,6 +405,11 @@ static void wakeup_source_activate(struct wakeup_source *ws)
>>  {
>>       unsigned int cec;
>>
>> +     if (WARN_ONCE(wakeup_source_not_registered(ws),
>> +                     "unregistered wakeup source\n")) {
>> +             return;
>> +     }
>
> The braces are not necessary and you can use format strings with WARN_ONCE()
> too IIRC.
>
>> +
>>       /*
>>        * active wakeup source should bring the system
>>        * out of PM_SUSPEND_FREEZE state
>>
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v2 1/3] power: validate wakeup source before activating it.

[Rafael J. Wysocki]

On Wednesday, May 06, 2015 03:11:48 PM Jin Qian wrote:
> Hi Rafael,
> 
> I kept braces since checkpatch.pl didn't report error or warning. The
> if condition has two lines, a brace make the statement more clear I
> think.

Well, you don't have to break the "if" line. :-)

As per CodingStyle, the braces should not be there.


> I removed the format string since ws->name can be random strings if
> the wakeup_source is not registered (i.e. using prepare functions).
> It's not very useful to print name if we have stack dump from
> WARN_ONCE.

OK


> On Wed, May 6, 2015 at 3:00 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Wednesday, May 06, 2015 11:51:07 AM Jin Qian wrote:
> >> A rogue wakeup source not registered in wakeup_sources list is not visible
> >> from wakeup_sources_stats_show. Check if the wakeup source is registered
> >> properly by looking at the timer struct.
> >>
> >> Signed-off-by: Jin Qian <jinqian@android.com>
> >> ---
> >>  drivers/base/power/wakeup.c | 19 +++++++++++++++++++
> >>  1 file changed, 19 insertions(+)
> >>
> >> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> >> index 7726200..aead738 100644
> >> --- a/drivers/base/power/wakeup.c
> >> +++ b/drivers/base/power/wakeup.c
> >> @@ -351,6 +351,20 @@ int device_set_wakeup_enable(struct device *dev, bool enable)
> >>  }
> >>  EXPORT_SYMBOL_GPL(device_set_wakeup_enable);
> >>
> >> +/**
> >> + * wakeup_source_not_registered - validate the given wakeup source.
> >> + * @ws: Wakeup source to be validated.
> >> + */
> >> +static bool wakeup_source_not_registered(struct wakeup_source *ws)
> >> +{
> >> +     /*
> >> +      * Use timer struct to check if the given source is initialized
> >> +      * by wakeup_source_add.
> >> +      */
> >> +     return ws->timer.function != pm_wakeup_timer_fn ||
> >> +                ws->timer.data != (unsigned long)ws;
> >> +}
> >> +
> >>  /*
> >>   * The functions below use the observation that each wakeup event starts a
> >>   * period in which the system should not be suspended.  The moment this period
> >> @@ -391,6 +405,11 @@ static void wakeup_source_activate(struct wakeup_source *ws)
> >>  {
> >>       unsigned int cec;
> >>
> >> +     if (WARN_ONCE(wakeup_source_not_registered(ws),
> >> +                     "unregistered wakeup source\n")) {
> >> +             return;
> >> +     }
> >
> > The braces are not necessary and you can use format strings with WARN_ONCE()
> > too IIRC.
> >
> >> +
> >>       /*
> >>        * active wakeup source should bring the system
> >>        * out of PM_SUSPEND_FREEZE state
> >>
> >
> > --
> > I speak only for myself.
> > Rafael J. Wysocki, Intel Open Source Technology Center.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v2 1/3] power: validate wakeup source before activating it.

[Jin Qian]

I have to break the line since it's over 80 characters : ) I'll follow
CodingStyle and send a new patch.

Thanks,
jin

On Wed, May 6, 2015 at 3:39 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, May 06, 2015 03:11:48 PM Jin Qian wrote:
>> Hi Rafael,
>>
>> I kept braces since checkpatch.pl didn't report error or warning. The
>> if condition has two lines, a brace make the statement more clear I
>> think.
>
> Well, you don't have to break the "if" line. :-)
>
> As per CodingStyle, the braces should not be there.
>
>
>> I removed the format string since ws->name can be random strings if
>> the wakeup_source is not registered (i.e. using prepare functions).
>> It's not very useful to print name if we have stack dump from
>> WARN_ONCE.
>
> OK
>
>
>> On Wed, May 6, 2015 at 3:00 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > On Wednesday, May 06, 2015 11:51:07 AM Jin Qian wrote:
>> >> A rogue wakeup source not registered in wakeup_sources list is not visible
>> >> from wakeup_sources_stats_show. Check if the wakeup source is registered
>> >> properly by looking at the timer struct.
>> >>
>> >> Signed-off-by: Jin Qian <jinqian@android.com>
>> >> ---
>> >>  drivers/base/power/wakeup.c | 19 +++++++++++++++++++
>> >>  1 file changed, 19 insertions(+)
>> >>
>> >> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
>> >> index 7726200..aead738 100644
>> >> --- a/drivers/base/power/wakeup.c
>> >> +++ b/drivers/base/power/wakeup.c
>> >> @@ -351,6 +351,20 @@ int device_set_wakeup_enable(struct device *dev, bool enable)
>> >>  }
>> >>  EXPORT_SYMBOL_GPL(device_set_wakeup_enable);
>> >>
>> >> +/**
>> >> + * wakeup_source_not_registered - validate the given wakeup source.
>> >> + * @ws: Wakeup source to be validated.
>> >> + */
>> >> +static bool wakeup_source_not_registered(struct wakeup_source *ws)
>> >> +{
>> >> +     /*
>> >> +      * Use timer struct to check if the given source is initialized
>> >> +      * by wakeup_source_add.
>> >> +      */
>> >> +     return ws->timer.function != pm_wakeup_timer_fn ||
>> >> +                ws->timer.data != (unsigned long)ws;
>> >> +}
>> >> +
>> >>  /*
>> >>   * The functions below use the observation that each wakeup event starts a
>> >>   * period in which the system should not be suspended.  The moment this period
>> >> @@ -391,6 +405,11 @@ static void wakeup_source_activate(struct wakeup_source *ws)
>> >>  {
>> >>       unsigned int cec;
>> >>
>> >> +     if (WARN_ONCE(wakeup_source_not_registered(ws),
>> >> +                     "unregistered wakeup source\n")) {
>> >> +             return;
>> >> +     }
>> >
>> > The braces are not necessary and you can use format strings with WARN_ONCE()
>> > too IIRC.
>> >
>> >> +
>> >>       /*
>> >>        * active wakeup source should bring the system
>> >>        * out of PM_SUSPEND_FREEZE state
>> >>
>> >
>> > --
>> > I speak only for myself.
>> > Rafael J. Wysocki, Intel Open Source Technology Center.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.