> The receive worker walks gsm->dlci[] without gsm->mutex while a
> concurrent GSMIOC_SETCONF -> gsm_cleanup_mux() frees the DLCIs, so the
> control handlers can dereference a freed gsm_dlci. v1's NULL check only
> narrowed the window; v2 fixes the use-after-free itself.
>
> The fix pins each DLCI the dispatch dereferences with its existing
> tty_port reference (option 2), so the data path stays lock-free. See the
> patch 1 commit message for details, including why the late destructor
> uses cmpxchg() so it cannot wipe a re-created mux (Daniel's teardown
> concern).
>
> Changes since v1:
> - Fix the UAF by reference-pinning instead of a NULL check in the
> handlers; no gsm->mutex in the data path (Greg, Daniel).
> - Pin every DLCI the dispatch touches, not just the addressed one:
> MSC/RLS/PN operate on gsm->dlci[k] named in the payload.
> - Add a base selftest (patch 2), as Greg asked.
>
> Verification (KASAN, panic_on_warn=1): the originally reported splat is
> the gsm_control_reply() / CMD_TEST path (see the Link in patch 1). A
> reproducer targeting the MSC handler crashes the unpatched kernel and
> survives 270 race rounds on v2. The selftest passes on both the clean
> and patched kernel (pass:3 fail:0 skip:0).
>
> Weiming Shi (2):
> tty: n_gsm: fix use-after-free in gsm_queue() control frame dispatch
> selftests: tty: add base regression test for n_gsm line discipline
So now there is a race on device node level. We have the old virtual
gsmttys still waiting for the current pending operations to finish while
the reconfiguration of the ldisc triggers a recreation of these. How is
this handled?
PATCH 1/2:
> - if (addr == 0 || addr >= NUM_DLCI || gsm->dlci[addr] == NULL)
> + if (addr == 0)
> return;
> - dlci = gsm->dlci[addr];
Control octets can be transmitted on non-control DLCIs in convergence layer
type 2. You do not guard against invalid DLCIs anymore. Same for parameter
negotiation and RLS.
> Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
> Cc: [email protected]
> Reported-by: Xiang Mei <[email protected]>
> Link: https://lore.kernel.org/all/[email protected]/
> Signed-off-by: Weiming Shi <[email protected]>
This whole worker separation was added later, not in e1eaea46bb40. Maybe
worth updating to the corresponding commit.
Also, the code comments have a strong AI flavor but AI use was only
disclosed for PATCH 2/2.
Best regards,
Daniel Starke
