Skip to contentSkip to conversation

tty: n_gsm: add some locking around gsm_mux[]

PATCH In review Dan Carpenter 2 participants 6 messages Apr 26, 2018 – Apr 27, 2018 1 message not in the archive TTY layer and serial drivers

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): 20180420083028.7fq3hw2mjjd7nrra@mwanda

Dan Carpenter

author
#1 lore raw

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

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

Reported-by Sun Peng <[email protected]>
Signed-off-by Dan Carpenter <[email protected]>
drivers/tty/n_gsm.c +8 -4
index 3b3e1f6632d7..cc7f68814200 100644
@@ -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
author
#2 lore raw

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

regards,
dan carpenter

author
#3 lore raw

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

In reply to a message not in the archive

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 <[email protected]>
Signed-off-by Dan Carpenter <[email protected]>
drivers/tty/n_gsm.c +3 -1
index cc7f68814200..1f2fd9e76fe0 100644
@@ -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)
#4 lore raw

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

Hi,

Quoted 18 lines · 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 <[email protected]>
Signed-off-by Dan Carpenter <[email protected]>
drivers/tty/n_gsm.c +0 -0
index cc7f68814200..1f2fd9e76fe0 100644
@@ -2075,9 +2075,11 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm)
 
 	/* Free up any link layer users */

drivers/tty/n_gsm.c static void gsm_cleanup_mux(struct gsm_mux *gsm) · line 2085

Quoted 10 lines
drivers/tty/n_gsm.c +3 -1
 	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

author
#5 lore raw

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

In reply to a message not in the archive

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

Signed-off-by Dan Carpenter <[email protected]>
drivers/tty/n_gsm.c +0 -2
index 1f2fd9e76fe0..44e9c5e3dbc1 100644
@@ -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);
author
#6 lore raw

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

In reply to a message not in the archive

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 <[email protected]>
Signed-off-by Dan Carpenter <[email protected]>
drivers/tty/n_gsm.c +1 -1
index 44e9c5e3dbc1..660153538ca7 100644
@@ -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;
 	}