• Dean Jenkins's avatar
    Bluetooth: Avoid bt_accept_unlink() double unlinking · 27bfbc21
    Dean Jenkins authored
    There is a race condition between a thread calling bt_accept_dequeue()
    and a different thread calling bt_accept_unlink(). Protection against
    concurrency is implemented using sk locking. However, sk locking causes
    serialisation of the bt_accept_dequeue() and bt_accept_unlink() threads.
    This serialisation can cause bt_accept_dequeue() to obtain the sk from the
    parent list but becomes blocked waiting for the sk lock held by the
    bt_accept_unlink() thread. bt_accept_unlink() unlinks sk and this thread
    releases the sk lock unblocking bt_accept_dequeue() which potentially runs
    bt_accept_unlink() again on the same sk causing a crash. The attempt to
    double unlink the same sk from the parent list can cause a NULL pointer
    dereference crash due to bt_sk(sk)->parent becoming NULL on the first
    unlink, followed by the second unlink trying to execute
    bt_sk(sk)->parent->sk_ack_backlog-- in bt_accept_unlink() which crashes.
    
    When sk is in the parent list, bt_sk(sk)->parent will be not be NULL.
    When sk is removed from the parent list, bt_sk(sk)->parent is set to
    NULL. Therefore, add a defensive check for bt_sk(sk)->parent not being
    NULL to ensure that sk is still in the parent list after the sk lock has
    been taken in bt_accept_dequeue(). If bt_sk(sk)->parent is detected as
    being NULL then restart the loop so that the loop variables are refreshed
    to use the latest values. This is necessary as list_for_each_entry_safe()
    is not thread safe so causing a risk of an infinite loop occurring as sk
    could point to itself.
    
    In addition, in bt_accept_dequeue() increase the sk reference count to
    protect against early freeing of sk. Early freeing can be possible if the
    bt_accept_unlink() thread calls l2cap_sock_kill() or rfcomm_sock_kill()
    functions before bt_accept_dequeue() gets the sk lock.
    
    For test purposes, the probability of failure can be increased by putting
    a msleep of 1 second in bt_accept_dequeue() between getting the sk and
    waiting for the sk lock. This exposes the fact that the loop
    list_for_each_entry_safe(p, n, &bt_sk(parent)->accept_q) is not safe from
    threads that unlink sk from the list in parallel with the loop which can
    cause sk to become stale within the loop.
    Signed-off-by: default avatarDean Jenkins <Dean_Jenkins@mentor.com>
    Signed-off-by: default avatarMarcel Holtmann <marcel@holtmann.org>
    27bfbc21
af_bluetooth.c 17.6 KB