Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure

From	Tilman Schmidt <tilman@imap.cc>
Newsgroups	linux.kernel
Subject	Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure
Date	2015-12-09 12:20 +0100
Message-ID	<qDKWS-7EC-11@gated-at.bofh.it> (permalink)
References	<qDotk-1E7-5@gated-at.bofh.it> <qDotk-1E7-19@gated-at.bofh.it> <qDzI5-jK-1@gated-at.bofh.it>
Organization	linux.* mail to news gateway

Show all headers | View raw

[Multipart message — attachments visible in raw view] - view raw

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.

Thread

[PATCH 0/3] ser_gigaset fixes Tilman Schmidt <tilman@imap.cc> - 2015-12-08 12:20 +0100
  [PATCH 3/3] ser_gigaset: remove unnecessary kfree() calls from release method Tilman Schmidt <tilman@imap.cc> - 2015-12-08 12:20 +0100
    Re: [PATCH 3/3] ser_gigaset: remove unnecessary kfree() calls from  release method Paul Bolle <pebolle@tiscali.nl> - 2015-12-09 00:20 +0100
  [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure Tilman Schmidt <tilman@imap.cc> - 2015-12-08 12:20 +0100
    Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device  structure Paul Bolle <pebolle@tiscali.nl> - 2015-12-09 00:20 +0100
      Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device  structure Tilman Schmidt <tilman@imap.cc> - 2015-12-09 12:20 +0100
        Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device  structure Paul Bolle <pebolle@tiscali.nl> - 2015-12-10 12:30 +0100
        Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device  structure Peter Hurley <peter@hurleysoftware.com> - 2015-12-10 15:10 +0100
  [PATCH 1/3] ser_gigaset: fix up NULL checks Tilman Schmidt <tilman@imap.cc> - 2015-12-08 12:20 +0100
    Re: [PATCH 1/3] ser_gigaset: fix up NULL checks Paul Bolle <pebolle@tiscali.nl> - 2015-12-08 20:50 +0100
      Re: [PATCH 1/3] ser_gigaset: fix up NULL checks One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2015-12-08 23:20 +0100
        Re: [PATCH 1/3] ser_gigaset: fix up NULL checks Tilman Schmidt <tilman@imap.cc> - 2015-12-09 11:50 +0100
          Re: [PATCH 1/3] ser_gigaset: fix up NULL checks One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2015-12-09 13:20 +0100
            Re: [PATCH 1/3] ser_gigaset: fix up NULL checks Paul Bolle <pebolle@tiscali.nl> - 2015-12-09 20:20 +0100
  Re: [PATCH 0/3] ser_gigaset fixes Paul Bolle <pebolle@tiscali.nl> - 2015-12-10 12:40 +0100

csiph-web