netdev - 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: <565F8341.7010704@hurleysoftware.com>
Date:	Wed, 2 Dec 2015 18:48:17 -0500
From:	Peter Hurley <peter@...leysoftware.com>
To:	Paul Bolle <pebolle@...cali.nl>, Tilman Schmidt <tilman@...p.cc>,
	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

On 11/30/2015 01:01 PM, Paul Bolle wrote:
> On ma, 2015-11-30 at 00:23 +0100, Paul Bolle wrote:
>> Relevant part of dmesg attached at the end of this message. This
>> should give me (and Tilman too?) an entry to get to bottom of this. 
>> Since this is relevant for anyone with just the ser-gigaset module 
>> installed, I hope to do that soon.
> 
> I'm planning to send something similar to the attached draft to netdev
> in a few days. It fixes the issue on my machine. Sascha, does it fix
> this issue for syzkaller too? 
> 
> Should (something like) this go into stable too?

Definitely for stable since it has a userspace triggerable component.

> Any further comments on that draft are appreciated too, of course.
> 
> 
> Paul Bolle
> ------
> [DRAFT] gigaset: don't free() a struct platform_device
> 
> One is not supposed to free() a struct platform_device. Instead one
> should, in the common case, only call platform_device_unregister(). That
> will drop the platform device's reference count. (Actually it's the
> reference count of the embedded kobject that is important here. But for
> users of platform devices that's basically irrelevant.)
> 
> So move struct platform_device dev out of struct ser_cardstate, because
> ser_cardstate is (malloc'ed and) free'd.
> 
> Reported-by: Sasha Levin <sasha.levin@...cle.com>
> Not-yet-signed-off-by: Paul Bolle <pebolle@...cali.nl>
> ---
>  drivers/isdn/gigaset/ser-gigaset.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/isdn/gigaset/ser-gigaset.c b/drivers/isdn/gigaset/ser-gigaset.c
> index 375be509e95f..f8ffa253496e 100644
> --- 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?

I ask because the typical approach for device teardown is to put the
kfree() in the release method; naturally, that won't work if there
is some other lifetime issue.

Regards,
Peter Hurley


>  	cs->hw.ser = NULL;
>  }
> @@ -401,17 +402,17 @@ static int gigaset_initcshw(struct cardstate *cs)
>  	}
>  	cs->hw.ser = scs;
>  
> -	cs->hw.ser->dev.name = GIGASET_MODULENAME;
> -	cs->hw.ser->dev.id = cs->minor_index;
> -	cs->hw.ser->dev.dev.release = gigaset_device_release;
> -	rc = platform_device_register(&cs->hw.ser->dev);
> +	pdev.name = GIGASET_MODULENAME;
> +	pdev.id = cs->minor_index;
> +	pdev.dev.release = gigaset_device_release;
> +	rc = platform_device_register(&pdev);
>  	if (rc != 0) {
>  		pr_err("error %d registering platform device\n", rc);
>  		kfree(cs->hw.ser);
>  		cs->hw.ser = NULL;
>  		return rc;
>  	}
> -	dev_set_drvdata(&cs->hw.ser->dev.dev, cs);
> +	dev_set_drvdata(&pdev.dev, cs);
>  
>  	tasklet_init(&cs->write_tasklet,
>  		     gigaset_modem_fill, (unsigned long) cs);
> @@ -520,7 +521,7 @@ gigaset_tty_open(struct tty_struct *tty)
>  		goto error;
>  	}
>  
> -	cs->dev = &cs->hw.ser->dev.dev;
> +	cs->dev = &pdev.dev;
>  	cs->hw.ser->tty = tty;
>  	atomic_set(&cs->hw.ser->refcnt, 1);
>  	init_completion(&cs->hw.ser->dead_cmp);
> 

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