netdev - Re: [syzbot] WARNING: ODEBUG bug in mgmt_index

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Sat, 10 Sep 2022 20:18:22 +0900
From:   Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To:     Marcel Holtmann <marcel@...tmann.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        Luiz Augusto von Dentz <luiz.dentz@...il.com>
Cc:     syzbot <syzbot+844c7bf1b1aa4119c5de@...kaller.appspotmail.com>,
        linux-bluetooth@...r.kernel.org, syzkaller-bugs@...glegroups.com
Subject: Re: [syzbot] WARNING: ODEBUG bug in mgmt_index_removed

I guess that, since hci_pi(sk)->hdev = hdev in hci_sock_bind() does not check whether a hdev
is already associated with some sk, it is possible that multiple sk are bound to the same hdev.
As a result, lock_sock(sk) is not sufficient for serializing access to hdev, and
hci_dev_lock(hdev) is needed when calling mgmt_index_*(hdev) functions.

If my guess above is correct, I think that what syzbot is telling us is that, due to lack of
serialization via hci_dev_lock(hdev), setting of HCI_MGMT flag from mgmt_init_hdev() from
hci_mgmt_cmd() from hci_sock_sendmsg() is racing with testing of HCI_MGMT flag from
mgmt_index_removed() from hci_sock_bind().

I suggest you to explicitly use lockdep_assert_held() in Bluethooth code for clarifying what
locks need to be held, instead of continue using racy operations like hci_dev_test_and_set_flag()
without holding appropriate locks.

hci_unregister_dev() {
  if (!test_bit(HCI_INIT, &hdev->flags) &&
      !hci_dev_test_flag(hdev, HCI_SETUP) &&
      !hci_dev_test_flag(hdev, HCI_CONFIG)) {
    hci_dev_lock(hdev);
    mgmt_index_removed(hdev) {
      if (!hci_dev_test_flag(hdev, HCI_MGMT))
        return;
      cancel_delayed_work_sync(&hdev->discov_off);
    }
    hci_dev_unlock(hdev);
  }
}

hci_sock_sendmsg() {
  lock_sock(sk);
  mutex_lock(&mgmt_chan_list_lock);
  chan = __hci_mgmt_chan_find(hci_pi(sk)->channel);
  if (chan)
    err = hci_mgmt_cmd(chan, sk, skb) {
      if (hdev && chan->hdev_init) // chan->hdev_init == mgmt_init_hdev
        chan->hdev_init(sk, hdev) {
          if (hci_dev_test_and_set_flag(hdev, HCI_MGMT)) // Missing hci_dev_lock(hdev)
            return;
          INIT_DELAYED_WORK(&hdev->discov_off, discov_off);
        }
      err = handler->func(sk, hdev, cp, len) { // handler->func() == set_external_config or set_public_address
        hci_dev_lock(hdev);
        mgmt_index_removed(hdev) {
          if (!hci_dev_test_flag(hdev, HCI_MGMT))
            return;
          cancel_delayed_work_sync(&hdev->discov_off);
        }
        hci_dev_unlock(hdev);
      }
    }
  else
    err = -EINVAL;
  mutex_unlock(&mgmt_chan_list_lock);
  release_sock(sk);
}

hci_sock_bind() {
  lock_sock(sk);
  mgmt_index_removed(hdev) {
    if (!hci_dev_test_flag(hdev, HCI_MGMT)) // Missing hci_dev_lock(hdev)
      return;
    cancel_delayed_work_sync(&hdev->discov_off); // ODEBUG complains missing INIT_DELAYED_WORK()
  }
  release_sock(sk);
}