netdev - Re: gigaset: memory leak in gigaset

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACT4Y+aNN8AAtRSvxxrP5RfsTZdd49odnQ_GRq86Y=jXr=pANw@mail.gmail.com>
Date:	Fri, 5 Feb 2016 14:28:59 +0100
From:	Dmitry Vyukov <dvyukov@...gle.com>
To:	Paul Bolle <pebolle@...cali.nl>
Cc:	Karsten Keil <isdn@...ux-pingi.de>,
	"David S. Miller" <davem@...emloft.net>,
	gigaset307x-common@...ts.sourceforge.net,
	netdev <netdev@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	syzkaller <syzkaller@...glegroups.com>,
	Kostya Serebryany <kcc@...gle.com>,
	Alexander Potapenko <glider@...gle.com>,
	Sasha Levin <sasha.levin@...cle.com>
Subject: Re: gigaset: memory leak in gigaset_initcshw

On Thu, Feb 4, 2016 at 4:06 PM, Paul Bolle <pebolle@...cali.nl> wrote:
> On do, 2016-02-04 at 15:54 +0100, Dmitry Vyukov wrote:
>> One TIOCSETD is enough to trigger the leak.
>> I've tested with different line disciplines and only N_GIGASET_M101
>> triggers the leak.
>
> So things appear to be just on my plate now. I'll see what I can come up
> with. Feel free to prod me if I stay silent for too long.
>
> Thanks for narrowing things down!

> 0) It's way too late here. And I can't really reproduce, and too many
> things jumps to 100% when running your reproducer. Anyhow, this is all I
> came up with (for drivers/isdn/gigaset/common.c):
>
> @@ -427,7 +427,11 @@ exit:
>
>  static void free_cs(struct cardstate *cs)
>  {
> -       cs->flags = 0;
> +       unsigned long flags;
> +       struct gigaset_driver *drv = cs->driver;
> +       spin_lock_irqsave(&drv->lock, flags);
> +       cs->flags &= ~VALID_MINOR;
> +       spin_unlock_irqrestore(&drv->lock, flags);
>  }
>
>  static void make_valid(struct cardstate *cs, unsigned mask)
>
> 1) On my side the logs do seem more sensible, for what it's worth. But
> does this fix the leak?
>
> 2) Note that I fear your reproducer allows to DOS the box (even if the
> leak turns out to be fixed) so the mess might be getting much worse. I
> need a clear hear to think this through. Ie, I need some sleep.

No, it does not help.

I wonder why you don't see the leak I am seeing... are you suing qemu
or real hardware? I am using qemu.

I've added the following change:

--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -396,6 +396,7 @@ static int gigaset_initcshw(struct cardstate *cs)
                pr_err("out of memory\n");
                return -ENOMEM;
        }
+       WARN_ON(cs->hw.ser != NULL);
        cs->hw.ser = scs;

        cs->hw.ser->dev.name = GIGASET_MODULENAME;

and it does fire.
Can it be a case that free_cs() runs before gigaset_device_release()?
If that would happen, then cs can be reused while the previous
cs->hw.ser is not freed yet. Just a guess.