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

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

[Rolf Evers-Fischer]

This is version 5 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. In v5 these two patches have not been changed.
Therefore the 'Acked-By'/'Reviewed-By' lines have been added. I hope
that's acceptable.

Changes in v5:
- Beautified the ugly part of Patch #3 (v4), where the correct return
  value was hidden under two levels of 'if'.

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 | 52 +++++++++++--------------------------
 1 file changed, 15 insertions(+), 37 deletions(-)

-- 
2.16.2

[PATCH v5 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>
Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.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 v5 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>
Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.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 v5 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 | 32 ++++++++++++--------------------
 1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
index 1878a6776519..59ed29e550e9 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,18 @@ 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;
+	if (ret) {
+		put_device(dev);
+		return ERR_PTR(ret);
+	}
 
 	ret = device_add(dev);
-	if (ret)
-		goto put_dev;
+	if (ret) {
+		put_device(dev);
+		return ERR_PTR(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 v5 0/3] pci: endpoint: Fix double free in pci_epf_create()

[Lorenzo Pieralisi]

On Wed, Feb 28, 2018 at 06:32:17PM +0100, Rolf Evers-Fischer wrote:
> This is version 5 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. In v5 these two patches have not been changed.
> Therefore the 'Acked-By'/'Reviewed-By' lines have been added. I hope
> that's acceptable.
> 
> Changes in v5:
> - Beautified the ugly part of Patch #3 (v4), where the correct return
>   value was hidden under two levels of 'if'.
> 
> 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 | 52 +++++++++++--------------------------
>  1 file changed, 15 insertions(+), 37 deletions(-)

Applied to pci/endpoint for v4.17, thanks.

Lorenzo