[PATCH 0/2] pci: endpoint: Fix double free in pci_epf_create()

[PATCH 0/2] pci: endpoint: Fix double free in pci_epf_create()

[Rolf Evers-Fischer]

When I accidentally created a new endpoint device with an empty name,
the kernel warned about "attempted to be registered with empty name!"
and crashed afterwards.

It turned out that the crash was not caused by the 'device_add()'
function itself, but by a double kfree of 'epf->name' and 'epf'.

The first patch just simplifies the code, while the second patch
fixes the problem.

Rolf Evers-Fischer (2):
  pci: endpoint: Free func_name after last usage
  pci: endpoint: Fix kernel panic after put_device()

 drivers/pci/endpoint/pci-epf-core.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

-- 
2.16.2

[PATCH 1/2] pci: endpoint: Free func_name after last usage

[Rolf Evers-Fischer]

From: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com>

This commit decreases the number of jump labels and ensures
that the next commit doesn't increase the number of occurrences
of 'kfree(func_name)'.

Change-Id: I0d1b6fd652395b85f82b11c43bf9b7db512854d1
Signed-off-by: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com>
Signed-off-by: Rolf Evers-Fischer <embedded24@evers-fischer.de>
---
 drivers/pci/endpoint/pci-epf-core.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
index 766ce1dca2ec..23d0e128d1a5 100644
--- a/drivers/pci/endpoint/pci-epf-core.c
+++ b/drivers/pci/endpoint/pci-epf-core.c
@@ -220,9 +220,10 @@ struct pci_epf *pci_epf_create(const char *name)
 	*buf = '\0';
 
 	epf->name = kstrdup(func_name, GFP_KERNEL);
+	kfree(func_name);
 	if (!epf->name) {
 		ret = -ENOMEM;
-		goto free_func_name;
+		goto free_epf;
 	}
 
 	dev = &epf->dev;
@@ -238,16 +239,12 @@ struct pci_epf *pci_epf_create(const char *name)
 	if (ret)
 		goto put_dev;
 
-	kfree(func_name);
 	return epf;
 
 put_dev:
 	put_device(dev);
 	kfree(epf->name);
 
-free_func_name:
-	kfree(func_name);
-
 free_epf:
 	kfree(epf);
 
-- 
2.16.2

Re: [PATCH 1/2] pci: endpoint: Free func_name after last usage

[Andy Shevchenko]

On Wed, Feb 21, 2018 at 2:47 PM, Rolf Evers-Fischer
<embedded24@evers-fischer.de> wrote:

> This commit decreases the number of jump labels and ensures
> that the next commit doesn't increase the number of occurrences
> of 'kfree(func_name)'.

> Change-Id: I0d1b6fd652395b85f82b11c43bf9b7db512854d1

First of all, this shouldn't be here.

> Signed-off-by: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com>
> Signed-off-by: Rolf Evers-Fischer <embedded24@evers-fischer.de>

Looks like bipolar disorder?

To the topic.
This is a slow path and your change makes code slightly less readable
b/c of patterns used in the kernel.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] pci: endpoint: Free func_name after last usage

[Rolf Evers-Fischer]

On Wed, 21 Feb 2018, Andy Shevchenko wrote:

> > Change-Id: I0d1b6fd652395b85f82b11c43bf9b7db512854d1
> 
> First of all, this shouldn't be here.
>
Please excuse me. I know that I have to remove this, but
unfortunately I forgot to do so (here and also in the other patch).
 
> > Signed-off-by: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com>
> > Signed-off-by: Rolf Evers-Fischer <embedded24@evers-fischer.de>
> 
> Looks like bipolar disorder?
>
You are right: One "Signed-off" should be enough.
 
> To the topic.
> This is a slow path and your change makes code slightly less readable
> b/c of patterns used in the kernel.
> 
I see. It is probably not a good idea to free the
memory in the middle of the function.
If you don't mind, I will remove this patch and change the other
one accordingly.

With best regards,
Rolf Evers-Fischer

Re: [PATCH 1/2] pci: endpoint: Free func_name after last usage

[Andy Shevchenko]

On Thu, Feb 22, 2018 at 6:21 PM, Rolf Evers-Fischer
<embedded24@evers-fischer.de> wrote:
> On Wed, 21 Feb 2018, Andy Shevchenko wrote:

>> This is a slow path and your change makes code slightly less readable
>> b/c of patterns used in the kernel.
>>
> I see. It is probably not a good idea to free the
> memory in the middle of the function.
> If you don't mind, I will remove this patch and change the other
> one accordingly.

Sounds good to me!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] pci: endpoint: Free func_name after last usage

[Lorenzo Pieralisi]

On Wed, Feb 21, 2018 at 01:47:06PM +0100, Rolf Evers-Fischer wrote:
> From: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com>
> 
> This commit decreases the number of jump labels and ensures
> that the next commit doesn't increase the number of occurrences
> of 'kfree(func_name)'.
> 
> Change-Id: I0d1b6fd652395b85f82b11c43bf9b7db512854d1
> Signed-off-by: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com>
> Signed-off-by: Rolf Evers-Fischer <embedded24@evers-fischer.de>
> ---
>  drivers/pci/endpoint/pci-epf-core.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> index 766ce1dca2ec..23d0e128d1a5 100644
> --- a/drivers/pci/endpoint/pci-epf-core.c
> +++ b/drivers/pci/endpoint/pci-epf-core.c
> @@ -220,9 +220,10 @@ struct pci_epf *pci_epf_create(const char *name)
>  	*buf = '\0';
>  
>  	epf->name = kstrdup(func_name, GFP_KERNEL);
> +	kfree(func_name);

I am certainly missing something but what if we reworked the code
and just:

kstrdup(name, GFP_KERNEL);

once instead of allocating another local copy (that we then have to
free) ?

Reworded: why

epf->name = func_name;

would not work ?

Thanks,
Lorenzo

>  	if (!epf->name) {
>  		ret = -ENOMEM;
> -		goto free_func_name;
> +		goto free_epf;
>  	}
>  
>  	dev = &epf->dev;
> @@ -238,16 +239,12 @@ struct pci_epf *pci_epf_create(const char *name)
>  	if (ret)
>  		goto put_dev;
>  
> -	kfree(func_name);
>  	return epf;
>  
>  put_dev:
>  	put_device(dev);
>  	kfree(epf->name);
>  
> -free_func_name:
> -	kfree(func_name);
> -
>  free_epf:
>  	kfree(epf);
>  
> -- 
> 2.16.2
> 

Re: [PATCH 1/2] pci: endpoint: Free func_name after last usage

[Kishon Vijay Abraham I]

Hi Lorenzo,

On Thursday 22 February 2018 11:49 PM, Lorenzo Pieralisi wrote:
> On Wed, Feb 21, 2018 at 01:47:06PM +0100, Rolf Evers-Fischer wrote:
>> From: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com>
>>
>> This commit decreases the number of jump labels and ensures
>> that the next commit doesn't increase the number of occurrences
>> of 'kfree(func_name)'.
>>
>> Change-Id: I0d1b6fd652395b85f82b11c43bf9b7db512854d1
>> Signed-off-by: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com>
>> Signed-off-by: Rolf Evers-Fischer <embedded24@evers-fischer.de>
>> ---
>>  drivers/pci/endpoint/pci-epf-core.c | 7 ++-----
>>  1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
>> index 766ce1dca2ec..23d0e128d1a5 100644
>> --- a/drivers/pci/endpoint/pci-epf-core.c
>> +++ b/drivers/pci/endpoint/pci-epf-core.c
>> @@ -220,9 +220,10 @@ struct pci_epf *pci_epf_create(const char *name)
>>  	*buf = '\0';
>>  
>>  	epf->name = kstrdup(func_name, GFP_KERNEL);
>> +	kfree(func_name);
> 
> I am certainly missing something but what if we reworked the code
> and just:
> 
> kstrdup(name, GFP_KERNEL);
> 
> once instead of allocating another local copy (that we then have to
> free) ?

name will be something like pci_epf_test.0 and in epf->name we want to just
have pci_epf_test.
> 
> Reworded: why
> 
> epf->name = func_name;

memory should be allocated for epf->name before it can be initialized. IMO
without kstrdup, there will be a null pointer exception.

Thanks
Kishon

Re: [PATCH 1/2] pci: endpoint: Free func_name after last usage

[Lorenzo Pieralisi]

On Fri, Feb 23, 2018 at 11:40:49AM +0530, Kishon Vijay Abraham I wrote:
> Hi Lorenzo,
> 
> On Thursday 22 February 2018 11:49 PM, Lorenzo Pieralisi wrote:
> > On Wed, Feb 21, 2018 at 01:47:06PM +0100, Rolf Evers-Fischer wrote:
> >> From: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com>
> >>
> >> This commit decreases the number of jump labels and ensures
> >> that the next commit doesn't increase the number of occurrences
> >> of 'kfree(func_name)'.
> >>
> >> Change-Id: I0d1b6fd652395b85f82b11c43bf9b7db512854d1
> >> Signed-off-by: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com>
> >> Signed-off-by: Rolf Evers-Fischer <embedded24@evers-fischer.de>
> >> ---
> >>  drivers/pci/endpoint/pci-epf-core.c | 7 ++-----
> >>  1 file changed, 2 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> >> index 766ce1dca2ec..23d0e128d1a5 100644
> >> --- a/drivers/pci/endpoint/pci-epf-core.c
> >> +++ b/drivers/pci/endpoint/pci-epf-core.c
> >> @@ -220,9 +220,10 @@ struct pci_epf *pci_epf_create(const char *name)
> >>  	*buf = '\0';
> >>  
> >>  	epf->name = kstrdup(func_name, GFP_KERNEL);
> >> +	kfree(func_name);
> > 
> > I am certainly missing something but what if we reworked the code
> > and just:
> > 
> > kstrdup(name, GFP_KERNEL);
> > 
> > once instead of allocating another local copy (that we then have to
> > free) ?
> 
> name will be something like pci_epf_test.0 and in epf->name we want to just
> have pci_epf_test.
> > 
> > Reworded: why
> > 
> > epf->name = func_name;
> 
> memory should be allocated for epf->name before it can be initialized. IMO
> without kstrdup, there will be a null pointer exception.

I understand that but the point is that func_name *was* allocated with
kstrdup() already I would like to understand why we need to do it twice
(and kfree the first allocation).

Lorenzo

Re: [PATCH 1/2] pci: endpoint: Free func_name after last usage

[Kishon Vijay Abraham I]

On Friday 23 February 2018 03:06 PM, Lorenzo Pieralisi wrote:
> On Fri, Feb 23, 2018 at 11:40:49AM +0530, Kishon Vijay Abraham I wrote:
>> Hi Lorenzo,
>>
>> On Thursday 22 February 2018 11:49 PM, Lorenzo Pieralisi wrote:
>>> On Wed, Feb 21, 2018 at 01:47:06PM +0100, Rolf Evers-Fischer wrote:
>>>> From: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com>
>>>>
>>>> This commit decreases the number of jump labels and ensures
>>>> that the next commit doesn't increase the number of occurrences
>>>> of 'kfree(func_name)'.
>>>>
>>>> Change-Id: I0d1b6fd652395b85f82b11c43bf9b7db512854d1
>>>> Signed-off-by: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com>
>>>> Signed-off-by: Rolf Evers-Fischer <embedded24@evers-fischer.de>
>>>> ---
>>>>  drivers/pci/endpoint/pci-epf-core.c | 7 ++-----
>>>>  1 file changed, 2 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
>>>> index 766ce1dca2ec..23d0e128d1a5 100644
>>>> --- a/drivers/pci/endpoint/pci-epf-core.c
>>>> +++ b/drivers/pci/endpoint/pci-epf-core.c
>>>> @@ -220,9 +220,10 @@ struct pci_epf *pci_epf_create(const char *name)
>>>>  	*buf = '\0';
>>>>  
>>>>  	epf->name = kstrdup(func_name, GFP_KERNEL);
>>>> +	kfree(func_name);
>>>
>>> I am certainly missing something but what if we reworked the code
>>> and just:
>>>
>>> kstrdup(name, GFP_KERNEL);
>>>
>>> once instead of allocating another local copy (that we then have to
>>> free) ?
>>
>> name will be something like pci_epf_test.0 and in epf->name we want to just
>> have pci_epf_test.
>>>
>>> Reworded: why
>>>
>>> epf->name = func_name;
>>
>> memory should be allocated for epf->name before it can be initialized. IMO
>> without kstrdup, there will be a null pointer exception.
> 
> I understand that but the point is that func_name *was* allocated with
> kstrdup() already I would like to understand why we need to do it twice
> (and kfree the first allocation).

func_name would be allocated for a size greater than what will be in epf->name.
It won't be significant though.

Thanks
Kishon

Re: [PATCH 1/2] pci: endpoint: Free func_name after last usage

[Rolf Evers-Fischer]

Hi Kishon,

On Fri, 23 Feb 2018, Kishon Vijay Abraham I wrote:
> On Friday 23 February 2018 03:06 PM, Lorenzo Pieralisi wrote:
> > On Fri, Feb 23, 2018 at 11:40:49AM +0530, Kishon Vijay Abraham I wrote:
> >> Hi Lorenzo,
> >>
> >> On Thursday 22 February 2018 11:49 PM, Lorenzo Pieralisi wrote:
> >>> On Wed, Feb 21, 2018 at 01:47:06PM +0100, Rolf Evers-Fischer wrote:
> >>>> From: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com>
> >>>>
> >>>> This commit decreases the number of jump labels and ensures
> >>>> that the next commit doesn't increase the number of occurrences
> >>>> of 'kfree(func_name)'.
> >>>>
> >>>> Change-Id: I0d1b6fd652395b85f82b11c43bf9b7db512854d1
> >>>> Signed-off-by: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com>
> >>>> Signed-off-by: Rolf Evers-Fischer <embedded24@evers-fischer.de>
> >>>> ---
> >>>>  drivers/pci/endpoint/pci-epf-core.c | 7 ++-----
> >>>>  1 file changed, 2 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> >>>> index 766ce1dca2ec..23d0e128d1a5 100644
> >>>> --- a/drivers/pci/endpoint/pci-epf-core.c
> >>>> +++ b/drivers/pci/endpoint/pci-epf-core.c
> >>>> @@ -220,9 +220,10 @@ struct pci_epf *pci_epf_create(const char *name)
> >>>>  	*buf = '\0';
> >>>>  
> >>>>  	epf->name = kstrdup(func_name, GFP_KERNEL);
> >>>> +	kfree(func_name);
> >>>
> >>> I am certainly missing something but what if we reworked the code
> >>> and just:
> >>>
> >>> kstrdup(name, GFP_KERNEL);
> >>>
> >>> once instead of allocating another local copy (that we then have to
> >>> free) ?
> >>
> >> name will be something like pci_epf_test.0 and in epf->name we want to just
> >> have pci_epf_test.
> >>>
> >>> Reworded: why
> >>>
> >>> epf->name = func_name;
> >>
> >> memory should be allocated for epf->name before it can be initialized. IMO
> >> without kstrdup, there will be a null pointer exception.
> > 
> > I understand that but the point is that func_name *was* allocated with
> > kstrdup() already I would like to understand why we need to do it twice
> > (and kfree the first allocation).
> 
> func_name would be allocated for a size greater than what will be in epf->name.
> It won't be significant though.
> 
> Thanks
> Kishon
> 
I have just an idea, how to use only one copy, but with the shorter 
length. What about changing the code like this?
  int len = strchrnul(name, '.') - name;
  epf->name = kstrndup(name, len, GFP_KERNEL);

In this case we are only allocating the smaller amount of memory and we 
don't need the 'buf' and 'func_name' pointers anymore. They will be 
replaced with 'len' and some pointer arithmetic.

Kind regards,
 Rolf

Re: [PATCH 1/2] pci: endpoint: Free func_name after last usage

[Lorenzo Pieralisi]

On Fri, Feb 23, 2018 at 06:38:42PM +0100, Rolf Evers-Fischer wrote:
> Hi Kishon,
> 
> On Fri, 23 Feb 2018, Kishon Vijay Abraham I wrote:
> > On Friday 23 February 2018 03:06 PM, Lorenzo Pieralisi wrote:
> > > On Fri, Feb 23, 2018 at 11:40:49AM +0530, Kishon Vijay Abraham I wrote:
> > >> Hi Lorenzo,
> > >>
> > >> On Thursday 22 February 2018 11:49 PM, Lorenzo Pieralisi wrote:
> > >>> On Wed, Feb 21, 2018 at 01:47:06PM +0100, Rolf Evers-Fischer wrote:
> > >>>> From: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com>
> > >>>>
> > >>>> This commit decreases the number of jump labels and ensures
> > >>>> that the next commit doesn't increase the number of occurrences
> > >>>> of 'kfree(func_name)'.
> > >>>>
> > >>>> Change-Id: I0d1b6fd652395b85f82b11c43bf9b7db512854d1
> > >>>> Signed-off-by: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com>
> > >>>> Signed-off-by: Rolf Evers-Fischer <embedded24@evers-fischer.de>
> > >>>> ---
> > >>>>  drivers/pci/endpoint/pci-epf-core.c | 7 ++-----
> > >>>>  1 file changed, 2 insertions(+), 5 deletions(-)
> > >>>>
> > >>>> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> > >>>> index 766ce1dca2ec..23d0e128d1a5 100644
> > >>>> --- a/drivers/pci/endpoint/pci-epf-core.c
> > >>>> +++ b/drivers/pci/endpoint/pci-epf-core.c
> > >>>> @@ -220,9 +220,10 @@ struct pci_epf *pci_epf_create(const char *name)
> > >>>>  	*buf = '\0';
> > >>>>  
> > >>>>  	epf->name = kstrdup(func_name, GFP_KERNEL);
> > >>>> +	kfree(func_name);
> > >>>
> > >>> I am certainly missing something but what if we reworked the code
> > >>> and just:
> > >>>
> > >>> kstrdup(name, GFP_KERNEL);
> > >>>
> > >>> once instead of allocating another local copy (that we then have to
> > >>> free) ?
> > >>
> > >> name will be something like pci_epf_test.0 and in epf->name we want to just
> > >> have pci_epf_test.
> > >>>
> > >>> Reworded: why
> > >>>
> > >>> epf->name = func_name;
> > >>
> > >> memory should be allocated for epf->name before it can be initialized. IMO
> > >> without kstrdup, there will be a null pointer exception.
> > > 
> > > I understand that but the point is that func_name *was* allocated with
> > > kstrdup() already I would like to understand why we need to do it twice
> > > (and kfree the first allocation).
> > 
> > func_name would be allocated for a size greater than what will be in epf->name.
> > It won't be significant though.
> > 
> > Thanks
> > Kishon
> > 
> I have just an idea, how to use only one copy, but with the shorter 
> length. What about changing the code like this?
>   int len = strchrnul(name, '.') - name;
>   epf->name = kstrndup(name, len, GFP_KERNEL);
> 
> In this case we are only allocating the smaller amount of memory and we 
> don't need the 'buf' and 'func_name' pointers anymore. They will be 
> replaced with 'len' and some pointer arithmetic.

Yes, something along those lines, I do not see the point of having to
allocate two buffers (and actually this simplifies the error path too).

Thanks,
Lorenzo

[PATCH 2/2] pci: endpoint: Fix kernel panic after put_device()

[Rolf Evers-Fischer]

From: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com>

'put_device()' calls the relase function 'pci_epf_dev_release()',
which already frees 'epf->name' and 'epf'.

Therefore we must not free them again after 'put_device()'.

Change-Id: I14ca19f96abfbbb489dd1f4d489e085329d7112e
Signed-off-by: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com>
Signed-off-by: Rolf Evers-Fischer <embedded24@evers-fischer.de>
---
 drivers/pci/endpoint/pci-epf-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
index 23d0e128d1a5..3718b4c5814a 100644
--- a/drivers/pci/endpoint/pci-epf-core.c
+++ b/drivers/pci/endpoint/pci-epf-core.c
@@ -243,7 +243,7 @@ struct pci_epf *pci_epf_create(const char *name)
 
 put_dev:
 	put_device(dev);
-	kfree(epf->name);
+	return ERR_PTR(ret);
 
 free_epf:
 	kfree(epf);
-- 
2.16.2