RE: [PATCH v2 0/2] tty: n_gsm: fix gsm_queue() UAF and add a base regression test

[Starke, Daniel]

> > > 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?
> >
> A pending gsmtty operation stays bound to the old object. gsmtty_install()
> takes a dlci_get() and the gsmtty ops use tty->driver_data, not gsm->dlci[],
> so an open fd keeps its own reference and never reaches the recreated dlci
> (a fresh gsm_dlci_alloc()). On teardown gsm_dlci_release() does
> tty_vhangup(), which unblocks the pending ops, and sets DLCI_CLOSED, which
> every gsmtty op checks first.
> 
> This is the existing tty_port refcount, and the receive path here uses the
> same dlci_get/put, so I don't think the pin adds a device-node race. But
> you know this hardware better than I do; if there's a path I'm missing,
> point me at it.

Sounds right. Thank you for the explanation. My tty internals understanding
is rather shallow.

> > 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.
> >
> 
> The checks aren't dropped, just folded into gsm_dlci_open_get(), which
> returns NULL for addr >= NUM_DLCI and for an empty slot. But hiding them
> in the lookup makes the diff read like the guard is gone, which was a bad
> call. For v4, maybe we should keep the addr checks at the call sites and
> et the helper only do the locked lookup and pin, so the guard stays
> visible where the DLCI is used? Does that match what you'd expect?

Let's revisit the code for gsm_control_modem for example:
	len = gsm_read_ea_val(&addr, data, cl);
	if (len < 1)
		return;

	addr >>= 1;
	/* Closed port, or invalid ? */
	if (addr == 0 || addr >= NUM_DLCI || gsm->dlci[addr] == NULL)
		return;
	dlci = gsm->dlci[addr];
Without the addr >= NUM_DLCI part you access the dlci array at a possible
out of bounds index if the received data was broken. Let us keep input data
validation at system boundaries. You also risk dereferencing a null
pointer if the requested DLCI has not been allocated, i.e. opened, yet.
Hence, both checks should be kept in place.

Best regards,
Daniel Starke