PCI: endpoint: handle probable NULL pointer access

Message ID 1507780677-7983-1-git-send-email-pankaj.dubey@samsung.com
State Changes Requested
Headers show
Series
  • PCI: endpoint: handle probable NULL pointer access
Related show

Commit Message

Pankaj Dubey Oct. 12, 2017, 3:57 a.m.
controller_group allocation in pci_ep_cfs_init function can fail
so we should have a check while using it in pci_ep_cfs_add_epc_group
for registering group, else we will hit NULL pointer access.

This patch adds required check for the same and returns -EPROBE_DEFER,
so that endpoint controller driver probe can be reattempted later
in case controller_group is not allocated because pci_ep_cfs_init is
not yet called.

Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
---
 drivers/pci/endpoint/pci-ep-cfs.c   | 7 ++++++-
 drivers/pci/endpoint/pci-epc-core.c | 4 ++++
 2 files changed, 10 insertions(+), 1 deletion(-)

Comments

Bjorn Helgaas Oct. 24, 2017, 8:02 p.m. | #1
On Thu, Oct 12, 2017 at 09:27:57AM +0530, Pankaj Dubey wrote:
> controller_group allocation in pci_ep_cfs_init function can fail
> so we should have a check while using it in pci_ep_cfs_add_epc_group
> for registering group, else we will hit NULL pointer access.
> 
> This patch adds required check for the same and returns -EPROBE_DEFER,
> so that endpoint controller driver probe can be reattempted later
> in case controller_group is not allocated because pci_ep_cfs_init is
> not yet called.
> 
> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>

Looking for Kishon's ack here.

> ---
>  drivers/pci/endpoint/pci-ep-cfs.c   | 7 ++++++-
>  drivers/pci/endpoint/pci-epc-core.c | 4 ++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c
> index 424fdd6..3cac818 100644
> --- a/drivers/pci/endpoint/pci-ep-cfs.c
> +++ b/drivers/pci/endpoint/pci-ep-cfs.c
> @@ -172,7 +172,12 @@ struct config_group *pci_ep_cfs_add_epc_group(const char *name)
>  	group = &epc_group->group;
>  
>  	config_group_init_type_name(group, name, &pci_epc_type);
> -	ret = configfs_register_group(controllers_group, group);
> +
> +	if (controllers_group)
> +		ret = configfs_register_group(controllers_group, group);
> +	else
> +		ret = -EPROBE_DEFER;
> +
>  	if (ret) {
>  		pr_err("failed to register configfs group for %s\n", name);
>  		goto err_register_group;
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index 42c2a11..d327a2a 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -518,6 +518,10 @@ __pci_epc_create(struct device *dev, const struct pci_epc_ops *ops,
>  		goto put_dev;
>  
>  	epc->group = pci_ep_cfs_add_epc_group(dev_name(dev));
> +	if (IS_ERR(epc->group)) {
> +		ret = -EPROBE_DEFER;
> +		goto put_dev;
> +	}
>  
>  	return epc;
>  
> -- 
> 2.7.4
>
Kishon Vijay Abraham I Oct. 25, 2017, 12:02 p.m. | #2
Hi,

On Wednesday 25 October 2017 01:32 AM, Bjorn Helgaas wrote:
> On Thu, Oct 12, 2017 at 09:27:57AM +0530, Pankaj Dubey wrote:
>> controller_group allocation in pci_ep_cfs_init function can fail
>> so we should have a check while using it in pci_ep_cfs_add_epc_group
>> for registering group, else we will hit NULL pointer access.
>>
>> This patch adds required check for the same and returns -EPROBE_DEFER,
>> so that endpoint controller driver probe can be reattempted later
>> in case controller_group is not allocated because pci_ep_cfs_init is
>> not yet called.
>>
>> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> 
> Looking for Kishon's ack here.
> 
>> ---
>>  drivers/pci/endpoint/pci-ep-cfs.c   | 7 ++++++-
>>  drivers/pci/endpoint/pci-epc-core.c | 4 ++++
>>  2 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c
>> index 424fdd6..3cac818 100644
>> --- a/drivers/pci/endpoint/pci-ep-cfs.c
>> +++ b/drivers/pci/endpoint/pci-ep-cfs.c
>> @@ -172,7 +172,12 @@ struct config_group *pci_ep_cfs_add_epc_group(const char *name)
>>  	group = &epc_group->group;
>>  
>>  	config_group_init_type_name(group, name, &pci_epc_type);
>> -	ret = configfs_register_group(controllers_group, group);
>> +
>> +	if (controllers_group)
>> +		ret = configfs_register_group(controllers_group, group);
>> +	else
>> +		ret = -EPROBE_DEFER;
>> +

Do you ever face this issue? Ideally controllers_group should always be
initialized if the dependencies are modeled properly.
>>  	if (ret) {
>>  		pr_err("failed to register configfs group for %s\n", name);
>>  		goto err_register_group;
>> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
>> index 42c2a11..d327a2a 100644
>> --- a/drivers/pci/endpoint/pci-epc-core.c
>> +++ b/drivers/pci/endpoint/pci-epc-core.c
>> @@ -518,6 +518,10 @@ __pci_epc_create(struct device *dev, const struct pci_epc_ops *ops,
>>  		goto put_dev;
>>  
>>  	epc->group = pci_ep_cfs_add_epc_group(dev_name(dev));
>> +	if (IS_ERR(epc->group)) {
>> +		ret = -EPROBE_DEFER;

should use the return value of pci_ep_cfs_add_epc_group().

However I don't think this is required since drivers might not actually need cfs.

Thanks
Kishon
Pankaj Dubey Oct. 28, 2017, 10:43 a.m. | #3
On 25 October 2017 at 17:32, Kishon Vijay Abraham I <kishon@ti.com> wrote:
> Hi,
>
> On Wednesday 25 October 2017 01:32 AM, Bjorn Helgaas wrote:
>> On Thu, Oct 12, 2017 at 09:27:57AM +0530, Pankaj Dubey wrote:
>>> controller_group allocation in pci_ep_cfs_init function can fail
>>> so we should have a check while using it in pci_ep_cfs_add_epc_group
>>> for registering group, else we will hit NULL pointer access.
>>>
>>> This patch adds required check for the same and returns -EPROBE_DEFER,
>>> so that endpoint controller driver probe can be reattempted later
>>> in case controller_group is not allocated because pci_ep_cfs_init is
>>> not yet called.
>>>
>>> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
>>
>> Looking for Kishon's ack here.
>>
>>> ---
>>>  drivers/pci/endpoint/pci-ep-cfs.c   | 7 ++++++-
>>>  drivers/pci/endpoint/pci-epc-core.c | 4 ++++
>>>  2 files changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c
>>> index 424fdd6..3cac818 100644
>>> --- a/drivers/pci/endpoint/pci-ep-cfs.c
>>> +++ b/drivers/pci/endpoint/pci-ep-cfs.c
>>> @@ -172,7 +172,12 @@ struct config_group *pci_ep_cfs_add_epc_group(const char *name)
>>>      group = &epc_group->group;
>>>
>>>      config_group_init_type_name(group, name, &pci_epc_type);
>>> -    ret = configfs_register_group(controllers_group, group);
>>> +
>>> +    if (controllers_group)
>>> +            ret = configfs_register_group(controllers_group, group);
>>> +    else
>>> +            ret = -EPROBE_DEFER;
>>> +
>
> Do you ever face this issue?

Yes, I was adding support for PCIe endpoint in Exynos driver and if we
see pci-exynos.c,
platform_driver_probe is called via subsys_initcall, which will happen
much before that module_init
and during endpoint probe sequence I got kernel panic for NULL pointer access.

> Ideally controllers_group should always be
> initialized if the dependencies are modeled properly.

Ideally Yes.

But we can't ignore error cases. Even though dependencies are modeled properly,
this check is mandatory, if we see pci_ep_cfs_init function where
"controllers_group" is suppose
to be allocated via call to "configfs_register_default_group", is
prone to failure as allocated via
kzalloc. We are handling error condition in pci_ep_cfs_init if it
fails to allocate "controllers_group"
but during EP initialization sequence, there is no check on
"controllers_group" pointer in
"configfs_register_default_group" function. So I feel it should have a
check for valid pointer.


>>>      if (ret) {
>>>              pr_err("failed to register configfs group for %s\n", name);
>>>              goto err_register_group;
>>> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
>>> index 42c2a11..d327a2a 100644
>>> --- a/drivers/pci/endpoint/pci-epc-core.c
>>> +++ b/drivers/pci/endpoint/pci-epc-core.c
>>> @@ -518,6 +518,10 @@ __pci_epc_create(struct device *dev, const struct pci_epc_ops *ops,
>>>              goto put_dev;
>>>
>>>      epc->group = pci_ep_cfs_add_epc_group(dev_name(dev));
>>> +    if (IS_ERR(epc->group)) {
>>> +            ret = -EPROBE_DEFER;
>
> should use the return value of pci_ep_cfs_add_epc_group().
>

OK. Will modify in next version.

> However I don't think this is required since drivers might not actually need cfs.

Ok, we can avoid propagating error to the caller here, but in case if
ERR_PTR there should be
at least one warn message. What do you say?


Thanks,
Pankaj Dubey
>
> Thanks
> Kishon
Bjorn Helgaas Nov. 6, 2017, 7:29 p.m. | #4
On Sat, Oct 28, 2017 at 04:13:56PM +0530, Pankaj Dubey wrote:
> On 25 October 2017 at 17:32, Kishon Vijay Abraham I <kishon@ti.com> wrote:
> > Hi,
> >
> > On Wednesday 25 October 2017 01:32 AM, Bjorn Helgaas wrote:
> >> On Thu, Oct 12, 2017 at 09:27:57AM +0530, Pankaj Dubey wrote:
> >>> controller_group allocation in pci_ep_cfs_init function can fail
> >>> so we should have a check while using it in pci_ep_cfs_add_epc_group
> >>> for registering group, else we will hit NULL pointer access.
> >>>
> >>> This patch adds required check for the same and returns -EPROBE_DEFER,
> >>> so that endpoint controller driver probe can be reattempted later
> >>> in case controller_group is not allocated because pci_ep_cfs_init is
> >>> not yet called.
> >>>
> >>> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> >>
> >> Looking for Kishon's ack here.
> >>
> >>> ---
> >>>  drivers/pci/endpoint/pci-ep-cfs.c   | 7 ++++++-
> >>>  drivers/pci/endpoint/pci-epc-core.c | 4 ++++
> >>>  2 files changed, 10 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c
> >>> index 424fdd6..3cac818 100644
> >>> --- a/drivers/pci/endpoint/pci-ep-cfs.c
> >>> +++ b/drivers/pci/endpoint/pci-ep-cfs.c
> >>> @@ -172,7 +172,12 @@ struct config_group *pci_ep_cfs_add_epc_group(const char *name)
> >>>      group = &epc_group->group;
> >>>
> >>>      config_group_init_type_name(group, name, &pci_epc_type);
> >>> -    ret = configfs_register_group(controllers_group, group);
> >>> +
> >>> +    if (controllers_group)
> >>> +            ret = configfs_register_group(controllers_group, group);
> >>> +    else
> >>> +            ret = -EPROBE_DEFER;
> >>> +
> >
> > Do you ever face this issue?
> 
> Yes, I was adding support for PCIe endpoint in Exynos driver and if we
> see pci-exynos.c,
> platform_driver_probe is called via subsys_initcall, which will happen
> much before that module_init
> and during endpoint probe sequence I got kernel panic for NULL pointer access.
> 
> > Ideally controllers_group should always be
> > initialized if the dependencies are modeled properly.
> 
> Ideally Yes.
> 
> But we can't ignore error cases. Even though dependencies are modeled properly,
> this check is mandatory, if we see pci_ep_cfs_init function where
> "controllers_group" is suppose
> to be allocated via call to "configfs_register_default_group", is
> prone to failure as allocated via
> kzalloc. We are handling error condition in pci_ep_cfs_init if it
> fails to allocate "controllers_group"
> but during EP initialization sequence, there is no check on
> "controllers_group" pointer in
> "configfs_register_default_group" function. So I feel it should have a
> check for valid pointer.

It seems plausible to me to check whether controllers_group is NULL,
but it'd be nicer to do it *first* in the function so there's no
cleanup to do, e.g.,

  if (!controllers_group)
    return -EPROBE_DEFER;

  epc_group = kzalloc(sizeof(*epc_group), GFP_KERNEL);
  ...

> >>>      if (ret) {
> >>>              pr_err("failed to register configfs group for %s\n", name);
> >>>              goto err_register_group;
> >>> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> >>> index 42c2a11..d327a2a 100644
> >>> --- a/drivers/pci/endpoint/pci-epc-core.c
> >>> +++ b/drivers/pci/endpoint/pci-epc-core.c
> >>> @@ -518,6 +518,10 @@ __pci_epc_create(struct device *dev, const struct pci_epc_ops *ops,
> >>>              goto put_dev;
> >>>
> >>>      epc->group = pci_ep_cfs_add_epc_group(dev_name(dev));
> >>> +    if (IS_ERR(epc->group)) {
> >>> +            ret = -EPROBE_DEFER;
> >
> > should use the return value of pci_ep_cfs_add_epc_group().
> >
> 
> OK. Will modify in next version.
> 
> > However I don't think this is required since drivers might not actually need cfs.
> 
> Ok, we can avoid propagating error to the caller here, but in case if
> ERR_PTR there should be
> at least one warn message. What do you say?
> 
> 
> Thanks,
> Pankaj Dubey
> >
> > Thanks
> > Kishon

Patch

diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c
index 424fdd6..3cac818 100644
--- a/drivers/pci/endpoint/pci-ep-cfs.c
+++ b/drivers/pci/endpoint/pci-ep-cfs.c
@@ -172,7 +172,12 @@  struct config_group *pci_ep_cfs_add_epc_group(const char *name)
 	group = &epc_group->group;
 
 	config_group_init_type_name(group, name, &pci_epc_type);
-	ret = configfs_register_group(controllers_group, group);
+
+	if (controllers_group)
+		ret = configfs_register_group(controllers_group, group);
+	else
+		ret = -EPROBE_DEFER;
+
 	if (ret) {
 		pr_err("failed to register configfs group for %s\n", name);
 		goto err_register_group;
diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
index 42c2a11..d327a2a 100644
--- a/drivers/pci/endpoint/pci-epc-core.c
+++ b/drivers/pci/endpoint/pci-epc-core.c
@@ -518,6 +518,10 @@  __pci_epc_create(struct device *dev, const struct pci_epc_ops *ops,
 		goto put_dev;
 
 	epc->group = pci_ep_cfs_add_epc_group(dev_name(dev));
+	if (IS_ERR(epc->group)) {
+		ret = -EPROBE_DEFER;
+		goto put_dev;
+	}
 
 	return epc;