| Message ID | 20180227100231.22561-2-embedded24@evers-fischer.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pci: endpoint: Fix double free in pci_epf_create() | expand |
Hi, On Tuesday 27 February 2018 03:32 PM, Rolf Evers-Fischer wrote: > 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); shouldn't the string end with '\0'? Thanks Kishon
Hi, On Tue, 27 Feb 2018, Kishon Vijay Abraham I wrote: > Hi, > > On Tuesday 27 February 2018 03:32 PM, Rolf Evers-Fischer wrote: > > 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); > > shouldn't the string end with '\0'? > I agree, it should end with '\0', but fortunately the 'kstrndup()' function already adds it for us, see mm/util.c, line 100: (...) memcpy(buf, s, len); buf[len] = '\0'; (...) Kind regards Rolf
On Tuesday 27 February 2018 03:45 PM, Rolf Evers-Fischer wrote: > Hi, > > On Tue, 27 Feb 2018, Kishon Vijay Abraham I wrote: > >> Hi, >> >> On Tuesday 27 February 2018 03:32 PM, Rolf Evers-Fischer wrote: >>> 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); >> >> shouldn't the string end with '\0'? >> > > I agree, it should end with '\0', but fortunately the 'kstrndup()' > function already adds it for us, see mm/util.c, line 100: > (...) > memcpy(buf, s, len); > buf[len] = '\0'; > (...) oh okay. missed that.. Acked-by: Kishon Vijay Abraham I <kishon@ti.com> > > Kind regards > Rolf >
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);