Re: [BUG] KASAN: slab-use-after-free Read in __timer_delete

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] The proposed patch does not fix the reported issue, but instead injects a 600ms busy-wait reproducer delay into the kernel.

Pre-existing issues:
- [Critical] Unlocked concurrent configuration ioctls and receive paths race to allocate and assign DLCIs, causing memory leaks and a Use-After-Free.
- [Critical] Concurrent execution of gsm_cleanup_mux() leads to a double-free of DLCIs.
- [High] gsmtty_cleanup() drops the reference of the wrong DLCI 0 if it was concurrently overwritten, leading to a Use-After-Free.
- [High] gsm_dlci_free() unconditionally clears gsm->dlci[dlci->addr], orphaning new DLCIs and causing a NULL pointer dereference in gsmtty_install().
--

--- Patch [1]: [BUG] KASAN: slab-use-after-free Read in __timer_delete ---
commit 64a236e2cd258e423904f320e674b1fd4365d267
Author: Jaeyoung Chung <[email protected]>
Subject: KASAN: slab-use-after-free Read in __timer_delete

This patch attempts to address a regression in __timer_delete found
by a race fuzzer. It provides a reproducer and a delay patch to reliably
trigger the race, rather than a root cause fix.

> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index c13e050de83b1..db5b56e82eb26 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -52,6 +52,8 @@
>  #include <linux/uaccess.h>
>  #include <linux/module.h>
>  #include <linux/timer.h>
> +#include <linux/delay.h>
> +#include <linux/sched.h>
>  #include <linux/tty_flip.h>
>  #include <linux/tty_driver.h>
>  #include <linux/serial.h>
[ ... ]
> @@ -2677,6 +2679,9 @@ static struct gsm_dlci *gsm_dlci_alloc(struct gsm_mux *gsm, int addr)
>  	} else {
>  		dlci->data = gsm_dlci_command;
>  	}
> +	if (addr == 0 && strncmp(current->comm, "syzrepro1", 9) == 0) {
> +		mdelay(600);
> +	}

[Severity: High]
Is this intended to be merged upstream?

The commit message notes this is diagnostic code to reproduce a race
condition rather than a fix.

Can this stall the kernel if triggered by an unprivileged user interacting
with the tty interface?

>  	gsm->dlci[addr] = dlci;

[Severity: Critical]
This is a pre-existing issue, but does this unlocked assignment lead to
memory leaks and orphaned timers?

Multiple threads can call the configuration ioctl concurrently, which calls
gsm_config() -> gsm_activate_mux() -> gsm_dlci_alloc() without holding any
locks. A network receive path via gsm_queue() can also call
gsm_dlci_alloc().

If one thread allocates a dlci and another thread unconditionally
overwrites it with a new dlci here, the first dlci is leaked. If the
first thread subsequently calls gsm_dlci_begin_open(), the leaked
dlci's timer is armed. 

When the mux is destroyed, the leaked timer is never stopped, which can
cause a use-after-free regression when the timer fires and accesses the
freed gsm_mux.

[Severity: High]
This isn't a bug introduced by this patch, but does overwriting the dlci
array concurrently also cause reference counting regressions in
gsmtty_cleanup()?

In gsmtty_install(), a reference is acquired via dlci_get(gsm->dlci[0]) but
the pointer isn't cached for cleanup. If a concurrent configuration ioctl
overwrites gsm->dlci[0], gsmtty_cleanup() will call dlci_put(gsm->dlci[0])
on the new dlci instead of the one it originally referenced. 

Could this prematurely free the active control dlci while it is still in
use?

[Severity: High]
This is a pre-existing issue, but can this also lead to a null pointer
dereference when freeing these objects?

When an old dlci is overwritten and finally freed, gsm_dlci_free() executes:

    dlci->gsm->dlci[dlci->addr] = NULL;

This destroys the pointer to the newly active dlci. A subsequent call to
gsmtty_install() will read gsm->dlci[0] as a null pointer and pass it to
dlci_get(), which passes &dlci->port to tty_port_get(). 

Since evaluating &NULL->port results in a non-zero struct offset, the null
check in tty_port_get() is bypassed.

[Severity: Critical]
This is a pre-existing issue, but can concurrent ioctls trigger a
double-free of these objects during cleanup?

If two threads call the configuration ioctl concurrently, they can both
trigger gsm_cleanup_mux(gsm, true). 

The first thread acquires gsm->mutex, iterates over gsm->dlci[], and calls
gsm_dlci_release() to drop the refcount. If the refcount does not reach zero
because ttys are still active, gsm_dlci_free() is not called and the array
entry remains a valid pointer.

When the second thread acquires the mutex, it observes the same pointer
and calls gsm_dlci_release() again.

Can this drop the active references prematurely and free the dlci while
ttys are still using it?

>  	return dlci;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1