diff mbox

[v1,1/1] PCI / PM: check all fields in pci_set_platform_pm()

Message ID 1465223133-73532-1-git-send-email-andriy.shevchenko@linux.intel.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Andy Shevchenko June 6, 2016, 2:25 p.m. UTC
When assign new PCI platform PM operations check for all mandatory fields to
prevent NULL pointer dereference.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pci/pci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Rafael J. Wysocki June 8, 2016, 12:17 a.m. UTC | #1
On Monday, June 06, 2016 05:25:33 PM Andy Shevchenko wrote:
> When assign new PCI platform PM operations check for all mandatory fields to
> prevent NULL pointer dereference.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

OK in principle, but what's the motivation?

> ---
>  drivers/pci/pci.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index c8b4dbd..badbddc 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -530,8 +530,8 @@ static const struct pci_platform_pm_ops *pci_platform_pm;
>  
>  int pci_set_platform_pm(const struct pci_platform_pm_ops *ops)
>  {
> -	if (!ops->is_manageable || !ops->set_state || !ops->choose_state
> -	    || !ops->sleep_wake)
> +	if (!ops->is_manageable || !ops->set_state || !ops->choose_state ||
> +	    !ops->sleep_wake || !ops->run_wake || !ops->need_resume)
>  		return -EINVAL;
>  	pci_platform_pm = ops;
>  	return 0;
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Shevchenko June 8, 2016, 9:04 a.m. UTC | #2
On Wed, 2016-06-08 at 02:17 +0200, Rafael J. Wysocki wrote:
> On Monday, June 06, 2016 05:25:33 PM Andy Shevchenko wrote:
> > When assign new PCI platform PM operations check for all mandatory
> > fields to
> > prevent NULL pointer dereference.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> OK in principle, but what's the motivation?

I didn't investigate the guts of the core code, but I'm about to add new
module which will use these facilities. Since the module is slightly
based on existing code for older kernels I noticed those new callbacks.
To prevent potential NULL pointer dereference.


> > ---
> >  drivers/pci/pci.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index c8b4dbd..badbddc 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -530,8 +530,8 @@ static const struct pci_platform_pm_ops
> > *pci_platform_pm;
> >  
> >  int pci_set_platform_pm(const struct pci_platform_pm_ops *ops)
> >  {
> > -	if (!ops->is_manageable || !ops->set_state || !ops-
> > >choose_state
> > -	    || !ops->sleep_wake)
> > +	if (!ops->is_manageable || !ops->set_state || !ops-
> > >choose_state ||
> > +	    !ops->sleep_wake || !ops->run_wake || !ops-
> > >need_resume)
> >  		return -EINVAL;
> >  	pci_platform_pm = ops;
> >  	return 0;
> > 
>
Bjorn Helgaas July 19, 2016, 9:37 p.m. UTC | #3
On Mon, Jun 06, 2016 at 05:25:33PM +0300, Andy Shevchenko wrote:
> When assign new PCI platform PM operations check for all mandatory fields to
> prevent NULL pointer dereference.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pci/pci.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index c8b4dbd..badbddc 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -530,8 +530,8 @@ static const struct pci_platform_pm_ops *pci_platform_pm;
>  
>  int pci_set_platform_pm(const struct pci_platform_pm_ops *ops)
>  {
> -	if (!ops->is_manageable || !ops->set_state || !ops->choose_state
> -	    || !ops->sleep_wake)
> +	if (!ops->is_manageable || !ops->set_state || !ops->choose_state ||
> +	    !ops->sleep_wake || !ops->run_wake || !ops->need_resume)
>  		return -EINVAL;

This looks OK to me.  platform_pci_run_wake() and
platform_pci_need_resume() assume that if pci_platform_pm is set,
pci_platform_pm->run_wake and pci_platform_pm->need_resume are valid
function pointers.  This is analogous to what we already do for
ops->is_manageable, etc.

Rafael, do you want to take this or should I?

>  	pci_platform_pm = ops;
>  	return 0;
> -- 
> 2.8.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki July 19, 2016, 10:04 p.m. UTC | #4
On Tue, Jul 19, 2016 at 11:37 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Mon, Jun 06, 2016 at 05:25:33PM +0300, Andy Shevchenko wrote:
>> When assign new PCI platform PM operations check for all mandatory fields to
>> prevent NULL pointer dereference.
>>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> ---
>>  drivers/pci/pci.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index c8b4dbd..badbddc 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -530,8 +530,8 @@ static const struct pci_platform_pm_ops *pci_platform_pm;
>>
>>  int pci_set_platform_pm(const struct pci_platform_pm_ops *ops)
>>  {
>> -     if (!ops->is_manageable || !ops->set_state || !ops->choose_state
>> -         || !ops->sleep_wake)
>> +     if (!ops->is_manageable || !ops->set_state || !ops->choose_state ||
>> +         !ops->sleep_wake || !ops->run_wake || !ops->need_resume)
>>               return -EINVAL;
>
> This looks OK to me.  platform_pci_run_wake() and
> platform_pci_need_resume() assume that if pci_platform_pm is set,
> pci_platform_pm->run_wake and pci_platform_pm->need_resume are valid
> function pointers.  This is analogous to what we already do for
> ops->is_manageable, etc.
>
> Rafael, do you want to take this or should I?
>
>>       pci_platform_pm = ops;
>>       return 0;
>> --

I can take it.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index c8b4dbd..badbddc 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -530,8 +530,8 @@  static const struct pci_platform_pm_ops *pci_platform_pm;
 
 int pci_set_platform_pm(const struct pci_platform_pm_ops *ops)
 {
-	if (!ops->is_manageable || !ops->set_state || !ops->choose_state
-	    || !ops->sleep_wake)
+	if (!ops->is_manageable || !ops->set_state || !ops->choose_state ||
+	    !ops->sleep_wake || !ops->run_wake || !ops->need_resume)
 		return -EINVAL;
 	pci_platform_pm = ops;
 	return 0;