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

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

[Rolf Evers-Fischer]

This is version 4 of a patchset to avoid double free in function
'pci_epf_create()'.

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. The third patch removes the goto labels.

Thank you Andy and Kishon for your Ack/Review on v3 for patches 1 and 2.
In v4 of these patches only the first lines of the commit messages
have been changed.

Changes in v4:
- s/pci/PCI and s/epf/EPF in the first line of
  recent commit messages (thanks, Bjorn!)
- The new patch #3 removes the goto labels
  in function 'pci_epf_create()' (thanks, Lorenzo!)

Changes in v3:
- Matched to other pending pci endpoint commits (thanks, Bjorn!)
- Added "Fixes" tag in patch 2 (thanks, Andy!)

Changes in v2:
- Based on feedback from Lorenzo, Andy and Kishon (thanks!)
- Change IDs removed
- First patch completely reworked in order to eliminate the
  need for the second 'kstrdup' allocation and the 'kfree' of
  the first allocation.
  It was tested with name="pci_epf_test.0" and name="pci_epb":
  The 'epf->name' was "pci_epf_test" or "pci_epb" (=unchanged).

Rolf Evers-Fischer (3):
  PCI: endpoint: Simplify name allocation for EPF device
  PCI: endpoint: Fix kernel panic after put_device()
  PCI: endpoint: pci_epf_create: remove goto labels

 drivers/pci/endpoint/pci-epf-core.c | 50 +++++++++----------------------------
 1 file changed, 12 insertions(+), 38 deletions(-)

-- 
2.16.2

[PATCH v4 1/3] PCI: endpoint: Simplify name allocation for EPF device

[Rolf Evers-Fischer]

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

This commit replaces allocating and freeing the intermediate
'buf'/'func_name' with a combination of 'kstrndup()' and 'len'.

'len' is the required length of 'epf->name'.
'epf->name' should be either the first part of 'name' preceding the '.'
or the complete 'name', if there is no '.' in the name.

Signed-off-by: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com>
---
 drivers/pci/endpoint/pci-epf-core.c | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
index 766ce1dca2ec..1f2506f32bb9 100644
--- a/drivers/pci/endpoint/pci-epf-core.c
+++ b/drivers/pci/endpoint/pci-epf-core.c
@@ -200,8 +200,7 @@ struct pci_epf *pci_epf_create(const char *name)
 	int ret;
 	struct pci_epf *epf;
 	struct device *dev;
-	char *func_name;
-	char *buf;
+	int len;
 
 	epf = kzalloc(sizeof(*epf), GFP_KERNEL);
 	if (!epf) {
@@ -209,20 +208,11 @@ struct pci_epf *pci_epf_create(const char *name)
 		goto err_ret;
 	}
 
-	buf = kstrdup(name, GFP_KERNEL);
-	if (!buf) {
-		ret = -ENOMEM;
-		goto free_epf;
-	}
-
-	func_name = buf;
-	buf = strchrnul(buf, '.');
-	*buf = '\0';
-
-	epf->name = kstrdup(func_name, GFP_KERNEL);
+	len = strchrnul(name, '.') - name;
+	epf->name = kstrndup(name, len, GFP_KERNEL);
 	if (!epf->name) {
 		ret = -ENOMEM;
-		goto free_func_name;
+		goto free_epf;
 	}
 
 	dev = &epf->dev;
@@ -238,16 +228,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

[PATCH v4 2/3] 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()'.

Fixes: 5e8cb4033807 ("PCI: endpoint: Add EP core layer to enable EP controller and EP functions")

Signed-off-by: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com>
---
 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 1f2506f32bb9..1878a6776519 100644
--- a/drivers/pci/endpoint/pci-epf-core.c
+++ b/drivers/pci/endpoint/pci-epf-core.c
@@ -232,7 +232,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

[PATCH v4 3/3] PCI: endpoint: pci_epf_create: remove goto labels

[Rolf Evers-Fischer]

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

Removes the goto labels completely, handles the errors at the
respective call site and just returns instead of jumping around.

Signed-off-by: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com>
---
 drivers/pci/endpoint/pci-epf-core.c | 30 +++++++++---------------------
 1 file changed, 9 insertions(+), 21 deletions(-)

diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
index 1878a6776519..cf2c4f6590a0 100644
--- a/drivers/pci/endpoint/pci-epf-core.c
+++ b/drivers/pci/endpoint/pci-epf-core.c
@@ -203,16 +203,14 @@ struct pci_epf *pci_epf_create(const char *name)
 	int len;
 
 	epf = kzalloc(sizeof(*epf), GFP_KERNEL);
-	if (!epf) {
-		ret = -ENOMEM;
-		goto err_ret;
-	}
+	if (!epf)
+		return ERR_PTR(-ENOMEM);
 
 	len = strchrnul(name, '.') - name;
 	epf->name = kstrndup(name, len, GFP_KERNEL);
 	if (!epf->name) {
-		ret = -ENOMEM;
-		goto free_epf;
+		kfree(epf);
+		return ERR_PTR(-ENOMEM);
 	}
 
 	dev = &epf->dev;
@@ -221,24 +219,14 @@ struct pci_epf *pci_epf_create(const char *name)
 	dev->type = &pci_epf_type;
 
 	ret = dev_set_name(dev, "%s", name);
-	if (ret)
-		goto put_dev;
-
-	ret = device_add(dev);
-	if (ret)
-		goto put_dev;
-
-	return epf;
+	if (!ret) {
+		ret = device_add(dev);
+		if (!ret)
+			return epf;
+	}
 
-put_dev:
 	put_device(dev);
 	return ERR_PTR(ret);
-
-free_epf:
-	kfree(epf);
-
-err_ret:
-	return ERR_PTR(ret);
 }
 EXPORT_SYMBOL_GPL(pci_epf_create);
 
-- 
2.16.2

Re: [PATCH v4 3/3] PCI: endpoint: pci_epf_create: remove goto labels

[Kishon Vijay Abraham I]

Hi,

On Wednesday 28 February 2018 06:37 PM, Rolf Evers-Fischer wrote:
> From: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com>
> 
> Removes the goto labels completely, handles the errors at the
> respective call site and just returns instead of jumping around.
> 
> Signed-off-by: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com>
> ---
>  drivers/pci/endpoint/pci-epf-core.c | 30 +++++++++---------------------
>  1 file changed, 9 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> index 1878a6776519..cf2c4f6590a0 100644
> --- a/drivers/pci/endpoint/pci-epf-core.c
> +++ b/drivers/pci/endpoint/pci-epf-core.c
> @@ -203,16 +203,14 @@ struct pci_epf *pci_epf_create(const char *name)
>  	int len;
>  
>  	epf = kzalloc(sizeof(*epf), GFP_KERNEL);
> -	if (!epf) {
> -		ret = -ENOMEM;
> -		goto err_ret;
> -	}
> +	if (!epf)
> +		return ERR_PTR(-ENOMEM);
>  
>  	len = strchrnul(name, '.') - name;
>  	epf->name = kstrndup(name, len, GFP_KERNEL);
>  	if (!epf->name) {
> -		ret = -ENOMEM;
> -		goto free_epf;
> +		kfree(epf);
> +		return ERR_PTR(-ENOMEM);
>  	}
>  
>  	dev = &epf->dev;
> @@ -221,24 +219,14 @@ struct pci_epf *pci_epf_create(const char *name)
>  	dev->type = &pci_epf_type;
>  
>  	ret = dev_set_name(dev, "%s", name);
> -	if (ret)
> -		goto put_dev;
> -
> -	ret = device_add(dev);
> -	if (ret)
> -		goto put_dev;
> -
> -	return epf;
> +	if (!ret) {
> +		ret = device_add(dev);
> +		if (!ret)
> +			return epf;

I don't prefer the correct return value being hidden under two levels of 'if'.

Thanks
Kishon

Re: [PATCH v4 3/3] PCI: endpoint: pci_epf_create: remove goto labels

[Lorenzo Pieralisi]

On Wed, Feb 28, 2018 at 02:07:19PM +0100, Rolf Evers-Fischer wrote:
> From: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com>
> 
> Removes the goto labels completely, handles the errors at the
> respective call site and just returns instead of jumping around.
> 
> Signed-off-by: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com>
> ---
>  drivers/pci/endpoint/pci-epf-core.c | 30 +++++++++---------------------
>  1 file changed, 9 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> index 1878a6776519..cf2c4f6590a0 100644
> --- a/drivers/pci/endpoint/pci-epf-core.c
> +++ b/drivers/pci/endpoint/pci-epf-core.c
> @@ -203,16 +203,14 @@ struct pci_epf *pci_epf_create(const char *name)
>  	int len;
>  
>  	epf = kzalloc(sizeof(*epf), GFP_KERNEL);
> -	if (!epf) {
> -		ret = -ENOMEM;
> -		goto err_ret;
> -	}
> +	if (!epf)
> +		return ERR_PTR(-ENOMEM);
>  
>  	len = strchrnul(name, '.') - name;
>  	epf->name = kstrndup(name, len, GFP_KERNEL);
>  	if (!epf->name) {
> -		ret = -ENOMEM;
> -		goto free_epf;
> +		kfree(epf);
> +		return ERR_PTR(-ENOMEM);
>  	}
>  
>  	dev = &epf->dev;
> @@ -221,24 +219,14 @@ struct pci_epf *pci_epf_create(const char *name)
>  	dev->type = &pci_epf_type;
>  
>  	ret = dev_set_name(dev, "%s", name);
> -	if (ret)
> -		goto put_dev;
> -
> -	ret = device_add(dev);
> -	if (ret)
> -		goto put_dev;
> -
> -	return epf;
> +	if (!ret) {
> +		ret = device_add(dev);
> +		if (!ret)
> +			return epf;
> +	}

This is ugly, sorry. I should have been more explicit, I prefer
this construct:

ret = dev_set_name(dev, "%s", name);
if (ret) {
	kfree(epf->name);
	kfree(epf);
	return ERR_PTR(ret);
}

ret = device_add(dev);
if (ret) {
	put_device(dev);
	return ERR_PTR(ret);
}

return epf;

Please send a v5 and let's be done with it.

Thanks,
Lorenzo

> -put_dev:
>  	put_device(dev);
>  	return ERR_PTR(ret);
> -
> -free_epf:
> -	kfree(epf);
> -
> -err_ret:
> -	return ERR_PTR(ret);
>  }
>  EXPORT_SYMBOL_GPL(pci_epf_create);
>  
> -- 
> 2.16.2
> 

Re: [PATCH v4 3/3] PCI: endpoint: pci_epf_create: remove goto labels

[Rolf Evers-Fischer]

Hi Kishon and Lorenzo,

On Wed, 28 Feb 2018, Lorenzo Pieralisi wrote:

> On Wed, Feb 28, 2018 at 02:07:19PM +0100, Rolf Evers-Fischer wrote:
> > From: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com>
> > 
> > Removes the goto labels completely, handles the errors at the
> > respective call site and just returns instead of jumping around.
> > 
> > Signed-off-by: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com>
> > ---
> >  drivers/pci/endpoint/pci-epf-core.c | 30 +++++++++---------------------
> >  1 file changed, 9 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> > index 1878a6776519..cf2c4f6590a0 100644
> > --- a/drivers/pci/endpoint/pci-epf-core.c
> > +++ b/drivers/pci/endpoint/pci-epf-core.c
> > @@ -203,16 +203,14 @@ struct pci_epf *pci_epf_create(const char *name)
> >  	int len;
> >  
> >  	epf = kzalloc(sizeof(*epf), GFP_KERNEL);
> > -	if (!epf) {
> > -		ret = -ENOMEM;
> > -		goto err_ret;
> > -	}
> > +	if (!epf)
> > +		return ERR_PTR(-ENOMEM);
> >  
> >  	len = strchrnul(name, '.') - name;
> >  	epf->name = kstrndup(name, len, GFP_KERNEL);
> >  	if (!epf->name) {
> > -		ret = -ENOMEM;
> > -		goto free_epf;
> > +		kfree(epf);
> > +		return ERR_PTR(-ENOMEM);
> >  	}
> >  
> >  	dev = &epf->dev;
> > @@ -221,24 +219,14 @@ struct pci_epf *pci_epf_create(const char *name)
> >  	dev->type = &pci_epf_type;
> >  
> >  	ret = dev_set_name(dev, "%s", name);
> > -	if (ret)
> > -		goto put_dev;
> > -
> > -	ret = device_add(dev);
> > -	if (ret)
> > -		goto put_dev;
> > -
> > -	return epf;
> > +	if (!ret) {
> > +		ret = device_add(dev);
> > +		if (!ret)
> > +			return epf;
> > +	}
> 
> This is ugly, sorry. I should have been more explicit, I prefer
> this construct:
> 
> ret = dev_set_name(dev, "%s", name);
> if (ret) {
> 	kfree(epf->name);
> 	kfree(epf);
> 	return ERR_PTR(ret);
> }
> 
> ret = device_add(dev);
> if (ret) {
> 	put_device(dev);
> 	return ERR_PTR(ret);
> }
> 
> return epf;
> 
> Please send a v5 and let's be done with it.
> 

Thank you for your feedback. I agree with you. Hiding the correct return 
value under two levels of 'if' is not the best idea.

I will send a v5 containing the proposal from Lorenzo. There is only one
difference: We must call 'put_device()' in both error cases, because
we have already initialized the device before. We had also jumped into
'put_dev:' in both situations before.

The modified code will then look like this:

 ret = dev_set_name(dev, "%s", name);
 if (ret) {
 	put_device(dev);
 	return ERR_PTR(ret);
 }

 ret = device_add(dev);
 if (ret) {
 	put_device(dev);
 	return ERR_PTR(ret);
 }

 return epf;

Best regards,
 Rolf

> > -put_dev:
> >  	put_device(dev);
> >  	return ERR_PTR(ret);
> > -
> > -free_epf:
> > -	kfree(epf);
> > -
> > -err_ret:
> > -	return ERR_PTR(ret);
> >  }
> >  EXPORT_SYMBOL_GPL(pci_epf_create);
> >  
> > -- 
> > 2.16.2
> > 
>