linux-kernel - Re: gigaset: freeing an active 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: <5664545C.90607@imap.cc>
Date:	Sun, 6 Dec 2015 16:29:32 +0100
From:	Tilman Schmidt <tilman@...p.cc>
To:	Paul Bolle <pebolle@...cali.nl>,
	Peter Hurley <peter@...leysoftware.com>,
	Sasha Levin <sasha.levin@...cle.com>
Cc:	isdn@...ux-pingi.de, davem@...emloft.net,
	gigaset307x-common@...ts.sourceforge.net,
	LKML <linux-kernel@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	syzkaller <syzkaller@...glegroups.com>
Subject: Re: gigaset: freeing an active object

Am 06.12.2015 um 14:31 schrieb Paul Bolle:
> On wo, 2015-12-02 at 18:48 -0500, Peter Hurley wrote:
>> On 11/30/2015 01:01 PM, Paul Bolle wrote:

>>> --- a/drivers/isdn/gigaset/ser-gigaset.c
>>> +++ b/drivers/isdn/gigaset/ser-gigaset.c
>>> @@ -42,8 +42,9 @@ MODULE_PARM_DESC(cidmode, "stay in CID mode when
>>> idle");
>>>  
>>>  static struct gigaset_driver *driver;
>>>  
>>> +static struct platform_device pdev;
>>> +
>>>  struct ser_cardstate {
>>> -	struct platform_device	dev;
>>>  	struct tty_struct	*tty;
>>>  	atomic_t		refcnt;
>>>  	struct completion	dead_cmp;
>>> @@ -370,8 +371,8 @@ static void gigaset_freecshw(struct cardstate
>>> *cs)
>>>  	tasklet_kill(&cs->write_tasklet);
>>>  	if (!cs->hw.ser)
>>>  		return;
>>> -	dev_set_drvdata(&cs->hw.ser->dev.dev, NULL);
>>> -	platform_device_unregister(&cs->hw.ser->dev);
>>> +	dev_set_drvdata(&pdev.dev, NULL);
>>> +	platform_device_unregister(&pdev);
>>>  	kfree(cs->hw.ser);
>>
>> Tilman,
>>
>> Is there a 1:1 correspondence and lifetime for the embedded platform
>> device and it's containing memory?
> 
> (Haven't heard from Tilman, so I'll give this a try.)

Sorry for that. Been busy.

> That containing memory is a struct ser_cardstate. And currently
> instances of struct _ser_cardstate are malloced and freed in routines
> that also call platform_device_register() and
> platform_device_unregister(). So yes, I think there's a 1:1
> correspondence.

Correct.

>> I ask because the typical approach for device teardown is to put the
>> kfree() in the release method;
> 
> (Side note: the (struct device) release method of this driver 
> -gigaset_device_release() - is actually a nop. It only frees device
> ->platform_data and platform_device->resource, but neither are actually
> used: they remain NULL through their entire life.)

Yeah, that was just copied unthinkingly from driver/base/platform.c.

So the solution might be as simple as moving the kfree() call from
gigaset_freecshw() to gigaset_device_release(). Something like this:

--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -370,19 +370,18 @@ static void gigaset_freecshw(struct cardstate *cs)
        tasklet_kill(&cs->write_tasklet);
        if (!cs->hw.ser)
                return;
-       dev_set_drvdata(&cs->hw.ser->dev.dev, NULL);
        platform_device_unregister(&cs->hw.ser->dev);
-       kfree(cs->hw.ser);
-       cs->hw.ser = NULL;
 }

 static void gigaset_device_release(struct device *dev)
 {
-       struct platform_device *pdev = to_platform_device(dev);
+       struct cardstate *cs = dev_get_drvdata(dev);

-       /* adapted from platform_device_release() in
drivers/base/platform.c */
-       kfree(dev->platform_data);
-       kfree(pdev->resource);
+       if (!cs)
+               return;
+       dev_set_drvdata(dev, NULL);
+       kfree(cs->hw.ser);
+       cs->hw.ser = NULL;
 }

 /*

(Off the top of my hat, completely untested, don't even know if that
will compile.)

-- 
Tilman Schmidt                              E-Mail: tilman@...p.cc
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.


Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)