Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1334840 > unrolled thread

ser_gigaset: fix memory leak

Back to article view | Back to linux.kernel

Contents

  ser_gigaset: fix memory leak Paul Bolle <pebolle@tiscali.nl> - 2016-02-15 22:40 +0100
    [PATCH 1/2] ser_gigaset: bail out if gigaset_initcs() fails Paul Bolle <pebolle@tiscali.nl> - 2016-02-15 22:40 +0100
    [PATCH 2/2] ser_gigaset: use container_of() instead of detour Paul Bolle <pebolle@tiscali.nl> - 2016-02-15 22:40 +0100
    Re: ser_gigaset: fix memory leak Dmitry Vyukov <dvyukov@google.com> - 2016-02-16 10:40 +0100

#1334840 — ser_gigaset: fix memory leak

Hi Dmitry,

I've cobbled together a two part series to fix the leak syzkaller uncovered in
ser_gigaset. I'd really appreciate it if you'd unleash syzkaller on this small
series one more time. Just to be sure that this also fixes the leak on your box
and that it doesn't introduce neww horrors.

Thanks,


Paul Bolle

[toc] | [next] | [standalone]

#1334842 — [PATCH 1/2] ser_gigaset: bail out if gigaset_initcs() fails

The first substantial thing that ser_gigaset's open() operation does, is
calling gigaset_initcs(). That function is well behaved: if it fails it
cleans up after itself and returns NULL. So if we receive a NULL here we
might as well bail out directly. (Note that both the bas_gigaset driver
and the usb_gigaset driver already do that.)

Besides, in the error path tty->disc_data will be set to NULL. But
tty->disc_data hasn't been touched yet, so there's no reason to set it
to NULL.

Not-yet-signed-off-by: Paul Bolle <pebolle@tiscali.nl>
---
 drivers/isdn/gigaset/ser-gigaset.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/isdn/gigaset/ser-gigaset.c b/drivers/isdn/gigaset/ser-gigaset.c
index 2a506fe0c8a4..ae69ab89c5c0 100644
--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -514,10 +514,8 @@ gigaset_tty_open(struct tty_struct *tty)
 
 	/* allocate memory for our device state and initialize it */
 	cs = gigaset_initcs(driver, 1, 1, 0, cidmode, GIGASET_MODULENAME);
-	if (!cs) {
-		rc = -ENODEV;
-		goto error;
-	}
+	if (!cs)
+		return -ENODEV;
 
 	cs->dev = &cs->hw.ser->dev.dev;
 	cs->hw.ser->tty = tty;
-- 
2.4.3

[toc] | [prev] | [next] | [standalone]

#1334844 — [PATCH 2/2] ser_gigaset: use container_of() instead of detour

The purpose of gigaset_device_release() is to kfree() the struct
ser_cardstate that contains our struct device. This is done via a bit of
a detour. First we make our struct device's driver_data point to the
container of our struct ser_cardstate (which is a struct cardstate). In
gigaset_device_release() we then retrieve that driver_data again. And
after that we finally kfree() the struct ser_cardstate that was saved in
the struct cardstate.

All of this can be achieved much easier by using container_of() to get
from our struct device to its container, struct ser_cardstate. Do so.

Note that the detour via driver_data broke down in commit 25cad69f21f5
("base/platform: Fix platform drivers with no probe callback"). That
commit reconnected our platform_device and our platform_driver again.
And one of the consequences of that fix was that
__device_release_driver() no longer is a NOP for our struct device but
actually does stuff again. One of the things it does, is setting our
driver_data to NULL. That, in turn, makes it impossible for
gigaset_device_release() to get to our struct cardstate. Which has the
net effect of leaking a struct ser_cardstate at every call of this
driver's tty close() operation. So using container_of() has the
additional benefit of actually working.

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Not-yet-signed-off-by: Paul Bolle <pebolle@tiscali.nl>
---
 drivers/isdn/gigaset/ser-gigaset.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/isdn/gigaset/ser-gigaset.c b/drivers/isdn/gigaset/ser-gigaset.c
index ae69ab89c5c0..6d40800f362c 100644
--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -373,13 +373,7 @@ static void gigaset_freecshw(struct cardstate *cs)
 
 static void gigaset_device_release(struct device *dev)
 {
-	struct cardstate *cs = dev_get_drvdata(dev);
-
-	if (!cs)
-		return;
-	dev_set_drvdata(dev, NULL);
-	kfree(cs->hw.ser);
-	cs->hw.ser = NULL;
+	kfree(container_of(dev, struct ser_cardstate, dev.dev));
 }
 
 /*
@@ -408,7 +402,6 @@ static int gigaset_initcshw(struct cardstate *cs)
 		cs->hw.ser = NULL;
 		return rc;
 	}
-	dev_set_drvdata(&cs->hw.ser->dev.dev, cs);
 
 	tasklet_init(&cs->write_tasklet,
 		     gigaset_modem_fill, (unsigned long) cs);
-- 
2.4.3

[toc] | [prev] | [next] | [standalone]

#1335218

On Mon, Feb 15, 2016 at 10:30 PM, Paul Bolle <pebolle@tiscali.nl> wrote:
> Hi Dmitry,
>
> I've cobbled together a two part series to fix the leak syzkaller uncovered in
> ser_gigaset. I'd really appreciate it if you'd unleash syzkaller on this small
> series one more time. Just to be sure that this also fixes the leak on your box
> and that it doesn't introduce neww horrors.


I've applied these two patches and they fixed the leak for me. Thanks!

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web