* [PATCH] PCI: endpoint: handle probable NULL pointer access
[not found] <CGME20171012035806epcas1p16df484a0565cfe780416253710464a8f@epcas1p1.samsung.com>
@ 2017-10-12 3:57 ` Pankaj Dubey
2017-10-24 20:02 ` Bjorn Helgaas
0 siblings, 1 reply; 5+ messages in thread
From: Pankaj Dubey @ 2017-10-12 3:57 UTC (permalink / raw)
To: linux-pci, linux-kernel
Cc: Kishon Vijay Abraham I, Bjorn Helgaas, Pankaj Dubey
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(-)
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] PCI: endpoint: handle probable NULL pointer access
2017-10-12 3:57 ` [PATCH] PCI: endpoint: handle probable NULL pointer access Pankaj Dubey
@ 2017-10-24 20:02 ` Bjorn Helgaas
2017-10-25 12:02 ` Kishon Vijay Abraham I
0 siblings, 1 reply; 5+ messages in thread
From: Bjorn Helgaas @ 2017-10-24 20:02 UTC (permalink / raw)
To: Pankaj Dubey
Cc: linux-pci, linux-kernel, Kishon Vijay Abraham I, Bjorn Helgaas
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] PCI: endpoint: handle probable NULL pointer access
2017-10-24 20:02 ` Bjorn Helgaas
@ 2017-10-25 12:02 ` Kishon Vijay Abraham I
2017-10-28 10:43 ` Pankaj Dubey
0 siblings, 1 reply; 5+ messages in thread
From: Kishon Vijay Abraham I @ 2017-10-25 12:02 UTC (permalink / raw)
To: Bjorn Helgaas, Pankaj Dubey; +Cc: linux-pci, linux-kernel, Bjorn Helgaas
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] PCI: endpoint: handle probable NULL pointer access
2017-10-25 12:02 ` Kishon Vijay Abraham I
@ 2017-10-28 10:43 ` Pankaj Dubey
2017-11-06 19:29 ` Bjorn Helgaas
0 siblings, 1 reply; 5+ messages in thread
From: Pankaj Dubey @ 2017-10-28 10:43 UTC (permalink / raw)
To: Kishon Vijay Abraham I
Cc: Bjorn Helgaas, linux-pci, linux-kernel, Bjorn Helgaas
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] PCI: endpoint: handle probable NULL pointer access
2017-10-28 10:43 ` Pankaj Dubey
@ 2017-11-06 19:29 ` Bjorn Helgaas
0 siblings, 0 replies; 5+ messages in thread
From: Bjorn Helgaas @ 2017-11-06 19:29 UTC (permalink / raw)
To: Pankaj Dubey
Cc: Kishon Vijay Abraham I, linux-pci, linux-kernel, Bjorn Helgaas
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-11-06 19:29 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20171012035806epcas1p16df484a0565cfe780416253710464a8f@epcas1p1.samsung.com>
2017-10-12 3:57 ` [PATCH] PCI: endpoint: handle probable NULL pointer access Pankaj Dubey
2017-10-24 20:02 ` Bjorn Helgaas
2017-10-25 12:02 ` Kishon Vijay Abraham I
2017-10-28 10:43 ` Pankaj Dubey
2017-11-06 19:29 ` Bjorn Helgaas
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.