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

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

[Rolf Evers-Fischer]

This is version 2 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.

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 (2):
  pci: endpoint: Simplify name allocation for epf device
  pci: endpoint: Fix kernel panic after put_device()

 drivers/pci/endpoint/pci-epf-core.c | 24 +++++-------------------
 1 file changed, 5 insertions(+), 19 deletions(-)

-- 
2.16.2

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

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

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

[Andy Shevchenko]

On Mon, Feb 26, 2018 at 7:16 PM, Rolf Evers-Fischer
<embedded24@evers-fischer.de> wrote:
> 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()'.

>  put_dev:
>         put_device(dev);
> -       kfree(epf->name);
> +       return ERR_PTR(ret);

Ouch, if it's double free (sounds like it is) it should be fixed up to
initial commit which brought that.
Thus, Fixes tag would be good to see.

-- 
With Best Regards,
Andy Shevchenko

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

[Bjorn Helgaas]

On Mon, Feb 26, 2018 at 06:16:15PM +0100, Rolf Evers-Fischer wrote:
> This is version 2 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.
> 
> 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 (2):
>   pci: endpoint: Simplify name allocation for epf device
>   pci: endpoint: Fix kernel panic after put_device()

$ git log --oneline --no-merges drivers/pci/endpoint/
fc41df28f89e PCI: endpoint: Fix EPF device name to support multi-function devices
4494738de0d9 PCI: endpoint: Add the function number as argument to EPC ops
b330104fa76d PCI: endpoint: Use EPC's device in dma_alloc_coherent()/dma_free_coherent()
35ad61921f49 PCI: endpoint: Fix find_first_zero_bit() usage
0c47cd7a9b6c PCI: endpoint: Populate func_no before calling pci_epc_add_epf()
...

Please make yours match.