Re: [PATCH v2] Bluetooth: RFCOMM: defer security confirmation to krfcommd

[Luiz Augusto von Dentz]

Hi Mikhail,

On Sat, Sep 5, 2026 at 10:49 AM mikhail.v.gavrilov@xxxxxxxxx
<mikhail.v.gavrilov@xxxxxxxxx> wrote:
>
> Hi Pauli,
>
> thanks for running the checker, and for looking at this again.
>
> > This v2 checks session hci_conn is the same as original, however it
> is
> > unclear if delayed processing of confirmation on the same hci_conn,
> can
> > result to wrong outcomes.
> >
> > What makes it not introduce new race conditions?
>
> Only two values cross the window: status and encrypt.  Everything else
> is read when the confirmation is applied - the session lookup, the DLC
> list, d->state, d->sec_level, and conn->sec_level in
> hci_conn_check_secure(), the last one under hdev->lock as before.
>
> Confirmations are queued and drained FIFO, and krfcommd drains the
> whole queue in one pass before rfcomm_process_sessions(), so none is
> dropped or reordered.  Two encryption changes in the same window are
> applied in the order they arrived, and the last one wins, same as
> without the queue.
>
> That leaves stale status/encrypt applied to a DLC list that may have
> changed meanwhile, and every path they feed fails closed:
>
>   - RFCOMM_SEC_PENDING with stale status or encrypt == 0 sets
>     RFCOMM_ENC_DROP, which drops the DLC;
>   - a stale success does not clear anything the next confirmation would
>     have acted on: if encryption really went away, that event is queued
>     too and reaches step two, which re-arms SEC_PENDING for
>     BT_SECURITY_MEDIUM and sets ENC_DROP for HIGH/FIPS;
>   - RFCOMM_AUTH_PENDING is answered with
>     !status && hci_conn_check_secure(conn, d->sec_level), and that
>     function reads the live conn->sec_level.  A stale failure can only
>     reject.  An accept still requires the link to be secure enough at
>     the time the decision is made.
>
> So a stale confirmation can close a DLC that would have survived; it
> cannot accept one that the current state of the link does not justify.
> A confirmation that cannot be allocated has the same effect - the DLC
> closes on its auth timeout.
>
> > GPT-5.6 produced report of pre-existing race condition where
> > security_cfm() races with DLC open and results to intermittent wrong
> > security level, but I didn't verify this was not nonsense.
>
> There is such a window and it is older than this patch.
> __rfcomm_dlc_open() sets RFCOMM_AUTH_PENDING on a new DLC when
> rfcomm_check_security() finds the request still in flight, and the next
> confirmation for that link clears the bit and answers with the status
> of whatever event it came from, which is not necessarily the request
> that DLC is waiting for.  Before this patch the callback took
> rfcomm_mutex and hit exactly the same DLC as soon as the opener
> released it.  The queue makes the window longer, it does not add a case
> that was not reachable.  Untangling that needs per-request state on the
> DLC and looks like separate work to me.
>
> > I wonder if the kernel_connect() could be moved out from under
> > rfcomm_mutex, since the RFCOMM channels should already have to handle
> > transition to CONNECTED state and possible failures there, and the
> lock
> > cycle solved from the other side.
>
> I agree that is the better place to fix it - it removes the inversion
> for any future callback that needs rfcomm_mutex, and it leaves the
> security confirmation synchronous, so none of the above has to be
> reasoned about at all.  Luiz suggested the same direction on the
> original thread.
>
> It is a rework of the connect path rather than a regression fix,
> though: rfcomm_session_create() would have to build and connect the
> socket outside the lock, and __rfcomm_dlc_open() would have to look the
> session up again after re-acquiring rfcomm_mutex and drop the socket it
> just made if another thread won the race.
>
> Luiz, which one do you want?  759c185d0bbd is in v7.3-rc1 and marked
> for stable, so the inversion is in a released tree and heading for the
> stable trees.  If you would rather have the small fix now and the
> connect-side rework in -next, v2 is here; if you want the connect side
> instead, I will write it - I would just rather not write it twice.

This is not small fix though, so I rather have the kernel_connect
rework fix this for good than doing one on top of the other.

> --
> Thanks,
> Mikhail



-- 
Luiz Augusto von Dentz