linux-kernel - Re: [PATCH] tty/n_gsm.c: fix a memory leak when gsmtty is removed

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: <55102FD5.5050605@suse.cz>
Date:	Mon, 23 Mar 2015 16:23:01 +0100
From:	Jiri Slaby <jslaby@...e.cz>
To:	Pan Xinhui <xinhuix.pan@...el.com>, gregkh@...uxfoundation.org,
	linux-kernel@...r.kernel.org
CC:	mnipxh@...il.com, yanmin_zhang@...ux.intel.com
Subject: Re: [PATCH] tty/n_gsm.c: fix a memory leak when gsmtty is removed

On 03/24/2015, 08:22 AM, Pan Xinhui wrote:
> In gsmtty_remove, we will put dlci. when dlci's ref-count is zero,
> tty_port_destructor will be called, and it will check if port->itty is
> NULL.
> However port->itty will be set to NULL in release_tty after gsmtty_remove.
> that may cause memory leak. so we use queue_work to put the dlci later.
> 
> Signed-off-by: xinhui.pan <xinhuix.pan@...el.com>
> ---
>  drivers/tty/n_gsm.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index c434376..50f4660 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -135,6 +135,7 @@ struct gsm_dlci {
>  #define DLCI_OPEN        2    /* SABM/UA complete */
>  #define DLCI_CLOSING        3    /* Sending DISC not seen UA/DM */
>      struct mutex mutex;
> +    struct work_struct putself_work;
>  
>      /* Link layer */
>      spinlock_t lock;    /* Protects the internal state */
> @@ -3170,14 +3171,25 @@ static int gsmtty_break_ctl(struct tty_struct
> *tty, int state)
>      return gsmtty_modem_update(dlci, encode);
>  }
>  
> -static void gsmtty_remove(struct tty_driver *driver, struct tty_struct
> *tty)
> +static void put_gsm_dlci(struct work_struct *work)
>  {
> -    struct gsm_dlci *dlci = tty->driver_data;
> +    struct gsm_dlci *dlci =
> +        container_of(work, struct gsm_dlci, putself_work);
>      struct gsm_mux *gsm = dlci->gsm;
>  
> +    mutex_lock(&gsm->mutex);
>      dlci_put(dlci);
>      dlci_put(gsm->dlci[0]);
> +    mutex_unlock(&gsm->mutex);
>      mux_put(gsm);
> +}
> +
> +static void gsmtty_remove(struct tty_driver *driver, struct tty_struct
> *tty)
> +{
> +    struct gsm_dlci *dlci = tty->driver_data;
> +
> +    INIT_WORK(&dlci->putself_work, put_gsm_dlci);
> +    schedule_work(&dlci->putself_work)

I am afraid you cannot guarantee it is "late enough" by this approach.
The work can be already running before itty is set to NULL.

If I am looking correctly the work can be moved from ->remove to
->cleanup, right?

And it would be worth to add a Fixes line to the commit log.
dfabf7ffa30585 introduced this.

And could you describe the scenario when it happens to the commit log
too? Like closing the other end first, before the tty.

thanks,
-- 
js
suse labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/