[PATCH] nvdimm: btt_devs: fix a NULL pointer dereference and a memory leak

[PATCH] nvdimm: btt_devs: fix a NULL pointer dereference and a memory leak

[Kangjie Lu]

In case kmemdup fails, the fix releases resources and returns to
avoid the NULL pointer dereference.
Also, the error paths in the following code should release
resources to avoid memory leaks.

Signed-off-by: Kangjie Lu <kjlu@umn.edu>
---
 drivers/nvdimm/btt_devs.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c
index 795ad4ff35ca..565ea0b6f765 100644
--- a/drivers/nvdimm/btt_devs.c
+++ b/drivers/nvdimm/btt_devs.c
@@ -196,8 +196,13 @@ static struct device *__nd_btt_create(struct nd_region *nd_region,
 	}
 
 	nd_btt->lbasize = lbasize;
-	if (uuid)
+	if (uuid) {
 		uuid = kmemdup(uuid, 16, GFP_KERNEL);
+		if (!uuid) {
+			kfree(nd_btt);
+			return NULL;
+		}
+	}
 	nd_btt->uuid = uuid;
 	dev = &nd_btt->dev;
 	dev_set_name(dev, "btt%d.%d", nd_region->id, nd_btt->id);
@@ -209,6 +214,7 @@ static struct device *__nd_btt_create(struct nd_region *nd_region,
 		dev_dbg(&ndns->dev, "failed, already claimed by %s\n",
 				dev_name(ndns->claim));
 		put_device(dev);
+		kfree(uuid);
 		return NULL;
 	}
 	return dev;
-- 
2.17.1

Re: [PATCH] nvdimm: btt_devs: fix a NULL pointer dereference and a memory leak

[Verma, Vishal L]

On Tue, 2019-03-12 at 03:15 -0500, Kangjie Lu wrote:
> In case kmemdup fails, the fix releases resources and returns to
> avoid the NULL pointer dereference.
> Also, the error paths in the following code should release
> resources to avoid memory leaks.
> 
> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> ---
>  drivers/nvdimm/btt_devs.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 

Looks good,
Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>

> diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c
> index 795ad4ff35ca..565ea0b6f765 100644
> --- a/drivers/nvdimm/btt_devs.c
> +++ b/drivers/nvdimm/btt_devs.c
> @@ -196,8 +196,13 @@ static struct device *__nd_btt_create(struct
> nd_region *nd_region,
>  	}
>  
>  	nd_btt->lbasize = lbasize;
> -	if (uuid)
> +	if (uuid) {
>  		uuid = kmemdup(uuid, 16, GFP_KERNEL);
> +		if (!uuid) {
> +			kfree(nd_btt);
> +			return NULL;
> +		}
> +	}
>  	nd_btt->uuid = uuid;
>  	dev = &nd_btt->dev;
>  	dev_set_name(dev, "btt%d.%d", nd_region->id, nd_btt->id);
> @@ -209,6 +214,7 @@ static struct device *__nd_btt_create(struct
> nd_region *nd_region,
>  		dev_dbg(&ndns->dev, "failed, already claimed by %s\n",
>  				dev_name(ndns->claim));
>  		put_device(dev);
> +		kfree(uuid);
>  		return NULL;
>  	}
>  	return dev;

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] nvdimm: btt_devs: fix a NULL pointer dereference and a memory leak

[Verma, Vishal L]

On Tue, 2019-03-12 at 03:15 -0500, Kangjie Lu wrote:
> In case kmemdup fails, the fix releases resources and returns to
> avoid the NULL pointer dereference.
> Also, the error paths in the following code should release
> resources to avoid memory leaks.
> 
> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> ---
>  drivers/nvdimm/btt_devs.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 

Looks good,
Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>

> diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c
> index 795ad4ff35ca..565ea0b6f765 100644
> --- a/drivers/nvdimm/btt_devs.c
> +++ b/drivers/nvdimm/btt_devs.c
> @@ -196,8 +196,13 @@ static struct device *__nd_btt_create(struct
> nd_region *nd_region,
>  	}
>  
>  	nd_btt->lbasize = lbasize;
> -	if (uuid)
> +	if (uuid) {
>  		uuid = kmemdup(uuid, 16, GFP_KERNEL);
> +		if (!uuid) {
> +			kfree(nd_btt);
> +			return NULL;
> +		}
> +	}
>  	nd_btt->uuid = uuid;
>  	dev = &nd_btt->dev;
>  	dev_set_name(dev, "btt%d.%d", nd_region->id, nd_btt->id);
> @@ -209,6 +214,7 @@ static struct device *__nd_btt_create(struct
> nd_region *nd_region,
>  		dev_dbg(&ndns->dev, "failed, already claimed by %s\n",
>  				dev_name(ndns->claim));
>  		put_device(dev);
> +		kfree(uuid);
>  		return NULL;
>  	}
>  	return dev;

Re: [PATCH] nvdimm: btt_devs: fix a NULL pointer dereference and a memory leak

[Dan Williams]

On Tue, Mar 12, 2019 at 1:16 AM Kangjie Lu <kjlu@umn.edu> wrote:
>
> In case kmemdup fails, the fix releases resources and returns to
> avoid the NULL pointer dereference.
> Also, the error paths in the following code should release
> resources to avoid memory leaks.
>
> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> ---
>  drivers/nvdimm/btt_devs.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c
> index 795ad4ff35ca..565ea0b6f765 100644
> --- a/drivers/nvdimm/btt_devs.c
> +++ b/drivers/nvdimm/btt_devs.c
> @@ -196,8 +196,13 @@ static struct device *__nd_btt_create(struct nd_region *nd_region,
>         }
>
>         nd_btt->lbasize = lbasize;
> -       if (uuid)
> +       if (uuid) {
>                 uuid = kmemdup(uuid, 16, GFP_KERNEL);
> +               if (!uuid) {
> +                       kfree(nd_btt);
> +                       return NULL;

What about nd_btt->id? That needs to be released as well.

> +               }
> +       }
>         nd_btt->uuid = uuid;
>         dev = &nd_btt->dev;
>         dev_set_name(dev, "btt%d.%d", nd_region->id, nd_btt->id);
> @@ -209,6 +214,7 @@ static struct device *__nd_btt_create(struct nd_region *nd_region,
>                 dev_dbg(&ndns->dev, "failed, already claimed by %s\n",
>                                 dev_name(ndns->claim));
>                 put_device(dev);
> +               kfree(uuid);

This will be a double free because put_device() will arrange for
nd_btt_release() to be called which does kfree(nd_btt->uuid);
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] nvdimm: btt_devs: fix a NULL pointer dereference and a memory leak

[Dan Williams]

On Tue, Mar 12, 2019 at 1:16 AM Kangjie Lu <kjlu@umn.edu> wrote:
>
> In case kmemdup fails, the fix releases resources and returns to
> avoid the NULL pointer dereference.
> Also, the error paths in the following code should release
> resources to avoid memory leaks.
>
> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> ---
>  drivers/nvdimm/btt_devs.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c
> index 795ad4ff35ca..565ea0b6f765 100644
> --- a/drivers/nvdimm/btt_devs.c
> +++ b/drivers/nvdimm/btt_devs.c
> @@ -196,8 +196,13 @@ static struct device *__nd_btt_create(struct nd_region *nd_region,
>         }
>
>         nd_btt->lbasize = lbasize;
> -       if (uuid)
> +       if (uuid) {
>                 uuid = kmemdup(uuid, 16, GFP_KERNEL);
> +               if (!uuid) {
> +                       kfree(nd_btt);
> +                       return NULL;

What about nd_btt->id? That needs to be released as well.

> +               }
> +       }
>         nd_btt->uuid = uuid;
>         dev = &nd_btt->dev;
>         dev_set_name(dev, "btt%d.%d", nd_region->id, nd_btt->id);
> @@ -209,6 +214,7 @@ static struct device *__nd_btt_create(struct nd_region *nd_region,
>                 dev_dbg(&ndns->dev, "failed, already claimed by %s\n",
>                                 dev_name(ndns->claim));
>                 put_device(dev);
> +               kfree(uuid);

This will be a double free because put_device() will arrange for
nd_btt_release() to be called which does kfree(nd_btt->uuid);

Re: [PATCH] nvdimm: btt_devs: fix a NULL pointer dereference and a memory leak

[Verma, Vishal L]

On Fri, 2019-03-22 at 22:49 +0000, Verma, Vishal L wrote:
> On Tue, 2019-03-12 at 03:15 -0500, Kangjie Lu wrote:
> > In case kmemdup fails, the fix releases resources and returns to
> > avoid the NULL pointer dereference.
> > Also, the error paths in the following code should release
> > resources to avoid memory leaks.
> > 
> > Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> > ---
> >  drivers/nvdimm/btt_devs.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> 
> Looks good,
> Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>

Looking again, there is another kmemdup() call in __nd_btt_probe which
is lacking error checking.
And referring to your other related patch to namespace_devs, that has
/several/ instances of the same thing.

Instead of fixing just a couple of these, it might be worthwhile to send
a wider cleanup patch to catch all of these, at least within
drivers/nvdimm/ for starters, instead of a select few.

> 
> > diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c
> > index 795ad4ff35ca..565ea0b6f765 100644
> > --- a/drivers/nvdimm/btt_devs.c
> > +++ b/drivers/nvdimm/btt_devs.c
> > @@ -196,8 +196,13 @@ static struct device *__nd_btt_create(struct
> > nd_region *nd_region,
> >  	}
> >  
> >  	nd_btt->lbasize = lbasize;
> > -	if (uuid)
> > +	if (uuid) {
> >  		uuid = kmemdup(uuid, 16, GFP_KERNEL);
> > +		if (!uuid) {
> > +			kfree(nd_btt);
> > +			return NULL;
> > +		}
> > +	}
> >  	nd_btt->uuid = uuid;
> >  	dev = &nd_btt->dev;
> >  	dev_set_name(dev, "btt%d.%d", nd_region->id, nd_btt->id);
> > @@ -209,6 +214,7 @@ static struct device *__nd_btt_create(struct
> > nd_region *nd_region,
> >  		dev_dbg(&ndns->dev, "failed, already claimed by %s\n",
> >  				dev_name(ndns->claim));
> >  		put_device(dev);
> > +		kfree(uuid);
> >  		return NULL;
> >  	}
> >  	return dev;
> 
> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] nvdimm: btt_devs: fix a NULL pointer dereference and a memory leak

[Verma, Vishal L]

On Fri, 2019-03-22 at 22:49 +0000, Verma, Vishal L wrote:
> On Tue, 2019-03-12 at 03:15 -0500, Kangjie Lu wrote:
> > In case kmemdup fails, the fix releases resources and returns to
> > avoid the NULL pointer dereference.
> > Also, the error paths in the following code should release
> > resources to avoid memory leaks.
> > 
> > Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> > ---
> >  drivers/nvdimm/btt_devs.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> 
> Looks good,
> Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>

Looking again, there is another kmemdup() call in __nd_btt_probe which
is lacking error checking.
And referring to your other related patch to namespace_devs, that has
/several/ instances of the same thing.

Instead of fixing just a couple of these, it might be worthwhile to send
a wider cleanup patch to catch all of these, at least within
drivers/nvdimm/ for starters, instead of a select few.

> 
> > diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c
> > index 795ad4ff35ca..565ea0b6f765 100644
> > --- a/drivers/nvdimm/btt_devs.c
> > +++ b/drivers/nvdimm/btt_devs.c
> > @@ -196,8 +196,13 @@ static struct device *__nd_btt_create(struct
> > nd_region *nd_region,
> >  	}
> >  
> >  	nd_btt->lbasize = lbasize;
> > -	if (uuid)
> > +	if (uuid) {
> >  		uuid = kmemdup(uuid, 16, GFP_KERNEL);
> > +		if (!uuid) {
> > +			kfree(nd_btt);
> > +			return NULL;
> > +		}
> > +	}
> >  	nd_btt->uuid = uuid;
> >  	dev = &nd_btt->dev;
> >  	dev_set_name(dev, "btt%d.%d", nd_region->id, nd_btt->id);
> > @@ -209,6 +214,7 @@ static struct device *__nd_btt_create(struct
> > nd_region *nd_region,
> >  		dev_dbg(&ndns->dev, "failed, already claimed by %s\n",
> >  				dev_name(ndns->claim));
> >  		put_device(dev);
> > +		kfree(uuid);
> >  		return NULL;
> >  	}
> >  	return dev;
> 
> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] nvdimm: btt_devs: fix a NULL pointer dereference and a memory leak

[Verma, Vishal L]

On Fri, 2019-03-22 at 22:56 +0000, Verma, Vishal L wrote:
> On Fri, 2019-03-22 at 22:49 +0000, Verma, Vishal L wrote:
> > On Tue, 2019-03-12 at 03:15 -0500, Kangjie Lu wrote:
> > > In case kmemdup fails, the fix releases resources and returns to
> > > avoid the NULL pointer dereference.
> > > Also, the error paths in the following code should release
> > > resources to avoid memory leaks.
> > > 
> > > Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> > > ---
> > >  drivers/nvdimm/btt_devs.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > 
> > Looks good,
> > Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
> 
> Looking again, there is another kmemdup() call in __nd_btt_probe which
> is lacking error checking.
> And referring to your other related patch to namespace_devs, that has
> /several/ instances of the same thing.
> 
> Instead of fixing just a couple of these, it might be worthwhile to send
> a wider cleanup patch to catch all of these, at least within
> drivers/nvdimm/ for starters, instead of a select few.

Hm, ignore this comment - these are the only two unchecked instances.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] nvdimm: btt_devs: fix a NULL pointer dereference and a memory leak

[Verma, Vishal L]

On Fri, 2019-03-22 at 22:56 +0000, Verma, Vishal L wrote:
> On Fri, 2019-03-22 at 22:49 +0000, Verma, Vishal L wrote:
> > On Tue, 2019-03-12 at 03:15 -0500, Kangjie Lu wrote:
> > > In case kmemdup fails, the fix releases resources and returns to
> > > avoid the NULL pointer dereference.
> > > Also, the error paths in the following code should release
> > > resources to avoid memory leaks.
> > > 
> > > Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> > > ---
> > >  drivers/nvdimm/btt_devs.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > 
> > Looks good,
> > Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
> 
> Looking again, there is another kmemdup() call in __nd_btt_probe which
> is lacking error checking.
> And referring to your other related patch to namespace_devs, that has
> /several/ instances of the same thing.
> 
> Instead of fixing just a couple of these, it might be worthwhile to send
> a wider cleanup patch to catch all of these, at least within
> drivers/nvdimm/ for starters, instead of a select few.

Hm, ignore this comment - these are the only two unchecked instances.