Re: [PATCH] Bluetooth: RFCOMM: serialize session teardown

[Ali Ahmet Memis]

I reproduced the splat here with the same recipe, forced unload plus the
two mdelay()s:

  BUG: KASAN: slab-use-after-free in rfcomm_run+0x3dda/0x3f20 [rfcomm]
  Read of size 8 at addr ffff8880037b3140 by task krfcommd/75
  Tainted: [R]=FORCED_RMMOD

faddr2line agrees with your report:

  rfcomm_session_del at net/bluetooth/rfcomm/core.c:713
  (inlined by) rfcomm_kill_listener at core.c:2167
  (inlined by) rfcomm_run at core.c:2190

With your patch applied and the same delays in place, five runs stayed
quiet. I also could not reproduce the other crash I mentioned. With a
delay in rfcomm_session_create(), forced unload would kill the box while
it was executing freed module text. Eight runs with the patch were clean.
So the change does fix the reported race.

The main thing left is the changelog. As written, it sounds like a plain
connect() can race with krfcommd. I don't think that's possible. Without
CAP_SYS_MODULE and delete_module(O_TRUNC), the module cannot be unloaded
while the socket is open. That means rfcomm_kill_listener() cannot run at
the same time as rfcomm_dlc_open(). The trigger here is a forced unload
and the delays need to be mentioned.

I'd also drop Cc: stable and the Fixes: tag. Forced unload requires root,
taints the kernel and is documented as unsafe: MODULE_FORCE_UNLOAD removes
the module "even if the kernel believes it is unsafe". This isn't
something that affects normal users, and Fixes: 1da177e4c3f4 invites
backports into every stable tree.

For consistency I think the change still makes sense. Every other
session_list traversal takes rfcomm_mutex, apart from
rfcomm_security_cfm(), which I mention below, and this one should too.

With a changelog along those lines and without the stable tag:

Reviewed-by: Ali Ahmet Memis <ali@xxxxxxxxxxxxxx>
Tested-by: Ali Ahmet Memis <ali@xxxxxxxxxxxxxx>

I'm not a Bluetooth maintainer and I don't have merge access.
get_maintainer.pl lists me here because of a recent commit to this file,
so this is only a review and test report from me. Whether the change
should be merged is up to the Bluetooth maintainers.

Pauli raised the same two spots while I was writing this. On rfcomm_run()
-> rfcomm_session_add(), I don't think it is reachable. kthread_run() runs
from rfcomm_init() before rfcomm_init_ttys() and rfcomm_init_sockets(), so
there is no socket or rfcomm dev yet and nothing else can touch
session_list.

rfcomm_security_cfm() -> rfcomm_session_get() is the one I'd worry about.
It runs from the hci_cb callback with only hci_cb_list_lock held, while
krfcommd can free the same session under rfcomm_mutex. I haven't tried to
reproduce that one.

I also agree with Pauli that taking the lock is better than relying on the
invariant. My objection is only to the stable tag.

One unrelated nit: the ### headings and fenced text blocks are a bit
unusual for LKML, but I don't see any problem with keeping them.