linux-kernel - Re: EDAC: Fix memory leak in creating CSROW object

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190427214925.GE16338@kroah.com>
Date:   Sat, 27 Apr 2019 23:49:25 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Borislav Petkov <bp@...en8.de>
Cc:     PanBian <bianpan2016@....com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        James Morse <james.morse@....com>, linux-edac@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: EDAC: Fix memory leak in creating CSROW object

On Fri, Apr 19, 2019 at 02:45:16AM +0200, Borislav Petkov wrote:
> On Fri, Apr 19, 2019 at 08:35:36AM +0800, PanBian wrote:
> > Yes, I see that. Because the loop start with (--i), there is no put
> > operation for the device that fails to create. So, I think we cannot
> > rule out the possibility of memory leak.
> 
> Ok, so this is not something you trigger - you're basically staring at
> the code?
> 
> Well, there's something else questionable in that code which I asked
> Greg about today but we didn't finish that conversation, let me CC him.
> 
> So AFAIU, devices for which device_add() has returned success,
> should be removed with their counterpart device_del().
> edac_create_csrow_objects(), however, does put_device() on those in the
> "unwinding" loop.
> 
> And for the case where device_add() fails, you should do put_device() to
> it. I.e., what you're saying.
> 
> So I think we need to figure what needs to be done when before fixing
> this properly.
> 
> Greg?

How about this patch, I think it fixes up everything you need to do
here, right?


diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 464174685589..0fb2d1de6d0e 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -404,6 +404,8 @@ static inline int nr_pages_per_csrow(struct csrow_info *csrow)
 static int edac_create_csrow_object(struct mem_ctl_info *mci,
 				    struct csrow_info *csrow, int index)
 {
+	int retval;
+
 	csrow->dev.type = &csrow_attr_type;
 	csrow->dev.groups = csrow_dev_groups;
 	device_initialize(&csrow->dev);
@@ -415,7 +417,10 @@ static int edac_create_csrow_object(struct mem_ctl_info *mci,
 	edac_dbg(0, "creating (virtual) csrow node %s\n",
 		 dev_name(&csrow->dev));
 
-	return device_add(&csrow->dev);
+	retval = device_add(&csrow->dev);
+	if (retval)
+		put_device(&csrow->dev);
+	return retval;
 }
 
 /* Create a CSROW object under specifed edac_mc_device */
@@ -649,6 +654,8 @@ static int edac_create_dimm_object(struct mem_ctl_info *mci,
 
 	edac_dbg(0, "creating rank/dimm device %s\n", dev_name(&dimm->dev));
 
+	if (err)
+		put_device(&dimm->dev);
 	return err;
 }
 
@@ -928,6 +935,7 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
 	err = device_add(&mci->dev);
 	if (err < 0) {
 		edac_dbg(1, "failure: create device %s\n", dev_name(&mci->dev));
+		put_device(mci->dev);
 		goto out;
 	}