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

[mikhail.v.gavrilov@xxxxxxxxx]

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.

-- 
Thanks,
Mikhail