* [PATCH 1/4] tty: n_gsm: add some locking around gsm_mux[]
[not found] <20180420083028.7fq3hw2mjjd7nrra@mwanda>
@ 2018-04-26 5:52 ` Dan Carpenter
2018-04-26 9:27 ` Dan Carpenter
2018-04-26 5:53 ` [PATCH 2/4] tty: n_gsm: Prevent a potential use after free Dan Carpenter
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2018-04-26 5:52 UTC (permalink / raw)
To: Greg Kroah-Hartman, Sun Peng
Cc: Jiri Slaby, linux-kernel, security, Tony Lindgren, Lars Poeschel,
Sascha Hauer
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/4] tty: n_gsm: Prevent a potential use after free
[not found] <20180420083028.7fq3hw2mjjd7nrra@mwanda>
2018-04-26 5:52 ` [PATCH 1/4] tty: n_gsm: add some locking around gsm_mux[] Dan Carpenter
@ 2018-04-26 5:53 ` Dan Carpenter
2018-04-27 18:50 ` Tony Lindgren
2018-04-26 5:53 ` [PATCH 3/4] tty: n_gsm: Remove an unused lock Dan Carpenter
2018-04-26 5:54 ` [PATCH 4/4] tty: n_gsm: Fix the test for if DLCI0 is open Dan Carpenter
3 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2018-04-26 5:53 UTC (permalink / raw)
To: Greg Kroah-Hartman, Sun Peng
Cc: Jiri Slaby, linux-kernel, security, Tony Lindgren, Lars Poeschel,
Sascha Hauer
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)
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 2/4] tty: n_gsm: Prevent a potential use after free
2018-04-26 5:53 ` [PATCH 2/4] tty: n_gsm: Prevent a potential use after free Dan Carpenter
@ 2018-04-27 18:50 ` Tony Lindgren
0 siblings, 0 replies; 6+ messages in thread
From: Tony Lindgren @ 2018-04-27 18:50 UTC (permalink / raw)
To: Dan Carpenter
Cc: Greg Kroah-Hartman, Sun Peng, Jiri Slaby, linux-kernel, security,
Lars Poeschel, Sascha Hauer
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 3/4] tty: n_gsm: Remove an unused lock
[not found] <20180420083028.7fq3hw2mjjd7nrra@mwanda>
2018-04-26 5:52 ` [PATCH 1/4] tty: n_gsm: add some locking around gsm_mux[] Dan Carpenter
2018-04-26 5:53 ` [PATCH 2/4] tty: n_gsm: Prevent a potential use after free Dan Carpenter
@ 2018-04-26 5:53 ` Dan Carpenter
2018-04-26 5:54 ` [PATCH 4/4] tty: n_gsm: Fix the test for if DLCI0 is open Dan Carpenter
3 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2018-04-26 5:53 UTC (permalink / raw)
To: Greg Kroah-Hartman, Sun Peng
Cc: Jiri Slaby, linux-kernel, security, Tony Lindgren, Lars Poeschel,
Sascha Hauer
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);
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 4/4] tty: n_gsm: Fix the test for if DLCI0 is open
[not found] <20180420083028.7fq3hw2mjjd7nrra@mwanda>
` (2 preceding siblings ...)
2018-04-26 5:53 ` [PATCH 3/4] tty: n_gsm: Remove an unused lock Dan Carpenter
@ 2018-04-26 5:54 ` Dan Carpenter
3 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2018-04-26 5:54 UTC (permalink / raw)
To: Greg Kroah-Hartman, Sun Peng
Cc: Jiri Slaby, linux-kernel, security, Tony Lindgren, Lars Poeschel,
Sascha Hauer
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;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread