[PATCH 1/4] tty: n_gsm: add some locking around gsm_mux[]

[PATCH 1/4] tty: n_gsm: add some locking around gsm_mux[]

[Dan Carpenter]

We should take "gsm_mux_lock" when we access gsm_mux[].

Reported-by: Sun Peng <sun_peng@topsec.com.cn>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 3b3e1f6632d7..cc7f68814200 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2898,18 +2898,22 @@ static int gsmtty_install(struct tty_driver *driver, struct tty_struct *tty)
 	bool alloc = false;
 	int ret;
 
-	line = line & 0x3F;
 
 	if (mux >= MAX_MUX)
 		return -ENXIO;
-	/* FIXME: we need to lock gsm_mux for lifetimes of ttys eventually */
-	if (gsm_mux[mux] == NULL)
-		return -EUNATCH;
+
+	line = line & 0x3F;
 	if (line == 0 || line > 61)	/* 62/63 reserved */
 		return -ECHRNG;
+
+	spin_lock(&gsm_mux_lock);
 	gsm = gsm_mux[mux];
+	spin_unlock(&gsm_mux_lock);
+	if (!gsm)
+		return -EUNATCH;
 	if (gsm->dead)
 		return -EL2HLT;
+
 	/* If DLCI 0 is not yet fully open return an error.
 	This is ok from a locking
 	perspective as we don't have to worry about this

[PATCH 2/4] tty: n_gsm: Prevent a potential use after free

[Dan Carpenter]

We're freeing the gsm->dlci[] array elements but leaving the freed
pointers hanging around.

My concern here is if we use the ioctl to change the config, it triggers
a restart in gsmld_config().  In that case, we would only reset the
first ->dlci[0] element and not the others so it does look to me like a
possible use after free.

Reported-by: Sun Peng <sun_peng@topsec.com.cn>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index cc7f68814200..1f2fd9e76fe0 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2075,9 +2075,11 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm)
 
 	/* Free up any link layer users */
 	mutex_lock(&gsm->mutex);
-	for (i = 0; i < NUM_DLCI; i++)
+	for (i = 0; i < NUM_DLCI; i++) {
 		if (gsm->dlci[i])
 			gsm_dlci_release(gsm->dlci[i]);
+		gsm->dlci[i] = NULL;
+	}
 	mutex_unlock(&gsm->mutex);
 	/* Now wipe the queues */
 	list_for_each_entry_safe(txq, ntxq, &gsm->tx_list, list)

[PATCH 3/4] tty: n_gsm: Remove an unused lock

[Dan Carpenter]

We don't use this spin_lock so we can remove the dead code.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 1f2fd9e76fe0..44e9c5e3dbc1 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -177,7 +177,6 @@ struct gsm_control {
 
 struct gsm_mux {
 	struct tty_struct *tty;		/* The tty our ldisc is bound to */
-	spinlock_t lock;
 	struct mutex mutex;
 	unsigned int num;
 	struct kref ref;
@@ -2188,7 +2187,6 @@ static struct gsm_mux *gsm_alloc_mux(void)
 		kfree(gsm);
 		return NULL;
 	}
-	spin_lock_init(&gsm->lock);
 	mutex_init(&gsm->mutex);
 	kref_init(&gsm->ref);
 	INIT_LIST_HEAD(&gsm->tx_list);

[PATCH 4/4] tty: n_gsm: Fix the test for if DLCI0 is open

[Dan Carpenter]

Logically, if gsm->dlci[0] is NULL then it's not open.  Also if it's
NULL then we would Oops when we do dlci_get(gsm->dlci[0]); at the end
of the function.

Reported-by: Sun Peng <sun_peng@topsec.com.cn>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 44e9c5e3dbc1..660153538ca7 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2919,7 +2919,7 @@ static int gsmtty_install(struct tty_driver *driver, struct tty_struct *tty)
 	perspective as we don't have to worry about this
 	if DLCI0 is lost */
 	mutex_lock(&gsm->mutex);
-	if (gsm->dlci[0] && gsm->dlci[0]->state != DLCI_OPEN) {
+	if (!gsm->dlci[0] || gsm->dlci[0]->state != DLCI_OPEN) {
 		mutex_unlock(&gsm->mutex);
 		return -EL2NSYNC;
 	}

Re: [PATCH 1/4] tty: n_gsm: add some locking around gsm_mux[]

[Dan Carpenter]

Peter Z points out that this isn't sufficient...  Let me try again.

regards,
dan carpenter

Re: [PATCH 2/4] tty: n_gsm: Prevent a potential use after free

[Tony Lindgren]

Hi,

* Dan Carpenter <dan.carpenter@oracle.com> [180426 05:55]:
> We're freeing the gsm->dlci[] array elements but leaving the freed
> pointers hanging around.
> 
> My concern here is if we use the ioctl to change the config, it triggers
> a restart in gsmld_config().  In that case, we would only reset the
> first ->dlci[0] element and not the others so it does look to me like a
> possible use after free.
> 
> Reported-by: Sun Peng <sun_peng@topsec.com.cn>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index cc7f68814200..1f2fd9e76fe0 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -2075,9 +2075,11 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm)
>  
>  	/* Free up any link layer users */
>  	mutex_lock(&gsm->mutex);
> -	for (i = 0; i < NUM_DLCI; i++)
> +	for (i = 0; i < NUM_DLCI; i++) {
>  		if (gsm->dlci[i])
>  			gsm_dlci_release(gsm->dlci[i]);
> +		gsm->dlci[i] = NULL;
> +	}
>  	mutex_unlock(&gsm->mutex);
>  	/* Now wipe the queues */
>  	list_for_each_entry_safe(txq, ntxq, &gsm->tx_list, list)

This one causes the following oops for me on closing n_gsm:

Unable to handle kernel NULL pointer dereference at virtual address 0000025c
...
(refcount_sub_and_test) from [<c057e424>] (refcount_dec_and_test+0x18/0x1c)
(refcount_dec_and_test) from [<c06378b8>] (tty_port_put+0x24/0x9c)
(tty_port_put) from [<bf8c9614>] (gsmtty_cleanup+0x2c/0x48 [n_gsm])
(gsmtty_cleanup [n_gsm]) from [<c062e0d4>] (release_one_tty+0x3c/0xa0)
(release_one_tty) from [<c01622b0>] (process_one_work+0x2ac/0x858)

Regards,

Tony