Re: [PATCH net v7 1/3] nfc: nci: uart: serialize driver selection

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message understates what the bug does.

--

nfc: nci: uart: serialize driver selection

This commit allocates the NCI UART context when the line discipline is
opened and claims it atomically in NCIUARTSETDRIVER, so two concurrent
callers can no longer both register a device. It also puts the transport
registry under a mutex and pins the transport module before calling into
it.

> NCIUARTSETDRIVER assumes that tty->disc_data remains NULL until a
> transport has been selected.  Two concurrent callers can therefore both
> allocate and register an NCI device for the same line discipline.
>
> The transport registry can also be changed while a caller is copying a
> template from it, before the transport module has been pinned.

[Severity: Low]
This isn't a bug in the code, but could the commit message say what these
races lead to? As written, it reads like a duplicate registration. The old
code looks like it allowed memory-safety problems.

In the old nci_uart_set_driver(), both racing callers allocate a context
and call ops.open(). For nfcmrvl, that path is nfcmrvl_nci_uart_open() ->
nfcmrvl_nci_register_dev() -> nci_register_device(), so each caller
registers its own NFC device. Both then do:

	tty->disc_data = nu;

The last write wins. nci_uart_tty_close() only tears down tty->disc_data,
so the losing nu and its NFC device are never closed and are leaked.

That leaked device still has nu->tty pointing at a tty that is freed
later. Bringing it up afterwards would go nci_uart_send() ->
nci_uart_write_work() -> tty->ops->write() on the freed tty_struct.

The old code also read the registry twice without a lock:

	if (!nci_uart_drivers[driver])
		return -ENOENT;
	...
	memcpy(nu, nci_uart_drivers[driver], sizeof(struct nci_uart));

If nci_uart_unregister() runs between the two reads, the memcpy() reads
through a NULL pointer.

It also ran ops.open(), which is code in the transport module, before
pinning that module:

	ret = nu->ops.open(nu);
	if (ret) {
		kfree(nu);
		return ret;
	} else if (!try_module_get(nu->owner)) {

The reproducer used a pty, and NCIUARTSETDRIVER has no capability check.
Would it make sense to name the leak, the use-after-free, the NULL
dereference and the call into an unloading module explicitly?

Step 9 of Documentation/process/coding-assistants.rst asks AI-assisted
reports to check Documentation/process/threat-model.rst and classify the
bug as a vulnerability or a regular bug. Should the commit message include
that classification?

Does this also need a Cc: stable@vger.kernel.org tag? According to
Documentation/process/maintainer-netdev.rst, netdev follows the standard
stable rules for fixes.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104062026.80599-1-shubham%40octane.security