From: Tilman Schmidt <tilman@imap•cc>
To: Paul Bolle <pebolle@tiscali•nl>, netdev@vger•kernel.org
Cc: Peter Hurley <peter@hurleysoftware•com>,
Sasha Levin <sasha.levin@oracle•com>,
syzkaller@googlegroups•com, David Miller <davem@davemloft•net>,
Karsten Keil <isdn@linux-pingi•de>,
isdn4linux@listserv•isdn4linux.de,
gigaset307x-common@lists•sourceforge.net,
linux-kernel@vger•kernel.org
Subject: Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure
Date: Wed, 9 Dec 2015 12:10:29 +0100 [thread overview]
Message-ID: <56680C25.1050704@imap.cc> (raw)
In-Reply-To: <1449616321.2384.36.camel@tiscali.nl>
[-- Attachment #1: Type: text/plain, Size: 2179 bytes --]
Am 09.12.2015 um 00:12 schrieb Paul Bolle:
>> --- a/drivers/isdn/gigaset/ser-gigaset.c
>> +++ b/drivers/isdn/gigaset/ser-gigaset.c
>> @@ -370,19 +370,23 @@ 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);
>
> dev equals cs->hw.ser->dev.dev, doesn't it?
Correct.
> So what does setting
> cs->hw.ser->dev.dev.driver_data to NULL just before freeing it buy us?
We're freeing cs->hw.ser, not cs->hw.ser->dev.
Clearing the reference to cs from the device structure before freeing cs
guards against possible use-after-free.
>> + kfree(cs->hw.ser);
>> + cs->hw.ser = NULL;
>
> I might be missing something, but what does setting this to NULL buy us
> here?
Just defensive programming. Guarding against possible use-after-free or
double-free.
>
> (I realize that I'm asking questions to code that isn't actually new but
> only moved around, but I think that's still an opportunity to have
> another look at that code.)
I'm a big fan of one change per patch. If we also want to modify the
moved code then that should be done in a separate patch. It makes
bisecting so much easier. Same reason why I separated out patch 3/3. And
btw same reason why I think patch 1/3 should go in as-is, as an obvious
fix to commit f34d7a5b, and any concerns about whether those tests are
useful should be addressed by a separate patch.
Regards,
Tilman
--
Tilman Schmidt E-Mail: tilman@imap•cc
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
next prev parent reply other threads:[~2015-12-09 11:10 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-12-08 11:00 [PATCH 0/3] ser_gigaset fixes Tilman Schmidt
2015-12-08 11:00 ` [PATCH 3/3] ser_gigaset: remove unnecessary kfree() calls from release method Tilman Schmidt
2015-12-08 23:15 ` Paul Bolle
2015-12-08 11:00 ` [PATCH 1/3] ser_gigaset: fix up NULL checks Tilman Schmidt
2015-12-08 19:45 ` Paul Bolle
2015-12-08 22:16 ` One Thousand Gnomes
2015-12-09 10:45 ` Tilman Schmidt
2015-12-09 12:12 ` One Thousand Gnomes
2015-12-09 19:18 ` Paul Bolle
2015-12-08 11:00 ` [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure Tilman Schmidt
2015-12-08 23:12 ` Paul Bolle
2015-12-09 11:10 ` Tilman Schmidt [this message]
2015-12-10 11:20 ` Paul Bolle
2015-12-10 14:04 ` Peter Hurley
2015-12-12 17:52 ` Tilman Schmidt
2015-12-10 11:31 ` [PATCH 0/3] ser_gigaset fixes Paul Bolle
2015-12-12 18:09 ` Tilman Schmidt
2015-12-12 18:32 ` Paul Bolle
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56680C25.1050704@imap.cc \
--to=tilman@imap$(echo .)cc \
--cc=davem@davemloft$(echo .)net \
--cc=gigaset307x-common@lists$(echo .)sourceforge.net \
--cc=isdn4linux@listserv$(echo .)isdn4linux.de \
--cc=isdn@linux-pingi$(echo .)de \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pebolle@tiscali$(echo .)nl \
--cc=peter@hurleysoftware$(echo .)com \
--cc=sasha.levin@oracle$(echo .)com \
--cc=syzkaller@googlegroups$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox