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

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: <5669866F.6040407@hurleysoftware.com>
Date:	Thu, 10 Dec 2015 06:04:31 -0800
From:	Peter Hurley <peter@...leysoftware.com>
To:	Tilman Schmidt <tilman@...p.cc>, Paul Bolle <pebolle@...cali.nl>
Cc:	netdev@...r.kernel.org, Sasha Levin <sasha.levin@...cle.com>,
	syzkaller@...glegroups.com, David Miller <davem@...emloft.net>,
	Karsten Keil <isdn@...ux-pingi.de>,
	isdn4linux@...tserv.isdn4linux.de,
	gigaset307x-common@...ts.sourceforge.net,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device
 structure

Hi Tilman,

On 12/09/2015 03:10 AM, Tilman Schmidt wrote:
> 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);

This is of marginal value and (I think) unnecessary; it implies
the core will use the device after release, which would trigger
many problems if true.


>> 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;

This pattern is common, and defends against much more common
driver bugs.

Unfortunately, much of the good this pattern is intended to do in finding
use-after-free bugs is undone by explicit tests for null everywhere else.
Not saying that's the case here; rather, generally speaking.

Like the
	if (!tty && !tty->ops && ....)

code.

Better just to let it crash.

Regards,
Peter Hurley


>> 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
> 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html