Received: by 2002:a05:6a10:f3d0:0:0:0:0 with SMTP id a16csp660279pxv; Wed, 14 Jul 2021 12:21:54 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzZNpEHSYSJi+ZeM0wDb7kmwsOt1r9Iy6e4P3abZXDXMQCigwO53/l1RvlFqYJZaeCpdG7S X-Received: by 2002:a17:906:144e:: with SMTP id q14mr14220096ejc.19.1626290514242; Wed, 14 Jul 2021 12:21:54 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1626290514; cv=none; d=google.com; s=arc-20160816; b=XKel0OeEwx5H4o+l0GGGECf+cg7fhEgFFQxBPhufa79IQ22StJ1a7exxe04hsdS/HH 8NTNEorI1hyKYQV5WjnuMIkjFSbQWujmUjZXXdYB0Rf+D84X6e8KTAC7dymslFAnq3WI aXbkNRqx0ebXKF86HzYhg5tnMC1r37KPcFCS6+wX6QaRa8LQLhUfReK8ilwnIbc0R2px IWPviW+JoALaVEJfzaXBgz3naEdyBD7mh+HXws3Cq7a+RqzyUMgt3gJUFuMnVu6lwfJF XPyPskVeL/QzFwrUJ9aUTsqexXgQZ/1Mvx+8GwiwUKNBQSMpFGfH0f311CwC/6adyLef cexA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:subject:message-id:date:from:in-reply-to :references:mime-version:dkim-signature; bh=jjSz1JmEJaotKApoc0SCfbkh4lIXQPiTHol15ysgTs0=; b=wNlTvL20tfZTmWlnZ88k79MADRxPrZCh99cD0PDsJY5eYmF418C11U738GBbwwA7Ut tDpnonE0a+cQul3OXLIZwM7IHn5fYlryl47OA81MyaDmYZSWEEfIazbP7buOUfzWcC4V eeRh3JPi0lDALWZ83SirfXouOIj6ZdzVFYuvX9pSzcpfG48qbzmgWt2AbFcpn+LPlMVC 2NEv8C5Id0z37zw/tVNOskI9wQeEFSB8d7hwbINIsYJQktVEDJiHS/7swVgfAvgwpQyB B/YRylYjWfhBwxb9IdNBj6s2PiTXpguT7XmzceIG0uk2qmitCCNg2zRjk00mb5XfLU8R XRdA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=d1q07o+6; spf=pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-bluetooth-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id z7si798339edq.389.2021.07.14.12.21.27; Wed, 14 Jul 2021 12:21:54 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=d1q07o+6; spf=pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-bluetooth-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230388AbhGNTXU (ORCPT + 99 others); Wed, 14 Jul 2021 15:23:20 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43762 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230129AbhGNTXT (ORCPT ); Wed, 14 Jul 2021 15:23:19 -0400 Received: from mail-yb1-xb34.google.com (mail-yb1-xb34.google.com [IPv6:2607:f8b0:4864:20::b34]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C0340C06175F; Wed, 14 Jul 2021 12:20:26 -0700 (PDT) Received: by mail-yb1-xb34.google.com with SMTP id r135so5054271ybc.0; Wed, 14 Jul 2021 12:20:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=jjSz1JmEJaotKApoc0SCfbkh4lIXQPiTHol15ysgTs0=; b=d1q07o+6HBG7oYjBg23TbFv555t1qmOmcDJkUv2Xzf6htne/ItLhesZHIqSii3TLg+ W+5zQRQzaMkIdnP/NdXzUmfd5qK+u8Q7DXzT6QKsPBjPGDPCrdvmHJK6OYKpWYuwJrSP I/oL4ZaBGSxetWqzlZXIktVa6xf+anEAIPgdN8EkDyVXA8sOuba7ZZ1y6NrttJDqURRL YqtIaDUzRjcIWvJI/WCcaqLrEQaGmSkniX+xw11U0P+TdOPKFrtDia+uR9Qtw4hzw5Wi V2XIIQv0JuN/prYd8nHhGZACuQ21tGgfY54ITUAkII01DMMFcx4gcx90aJ5P6n3WblPb l73Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=jjSz1JmEJaotKApoc0SCfbkh4lIXQPiTHol15ysgTs0=; b=LmLI3APJt5muIzAN4gEKhW6EikeGUN0/tV4y62/zBb2UKhk7SL66sB+qVDQnpbBKHS QiYtgUtOuVvlDFKb39Hnk5TV9ovcjhps+jZR/ikeKVVBBihZjV9tAPnU1Tnx0tykTkKr /SpTeWY2PHF63J/7/3HA4GNR4R5N8kb4GUIoP/AyWtXjATh2B1NG5CXXXWSef3ocYya2 z7PlpTDa1VkrZRP45YxH/FlJ6D/KLdBhYnsOJ7z3/Bk+rrszCi3GJakqeZ7A8YJnkQUj 7kljhwmqywTFukteoNLBnHN1NTLgc8VklE2y6GRGMqQwM7rxouGaViTjwHbzlC8gJjlo UlaA== X-Gm-Message-State: AOAM533aorhDESxD6xyHkZpBpX+iBjuMLK2HGORh0VrV6WKj2vIdwgET ih/firMQFYkB5uA18gAB5Ab7vefKBGBqAaIFNZt+SUcoX7wbYQ== X-Received: by 2002:a25:8205:: with SMTP id q5mr15052420ybk.440.1626290425963; Wed, 14 Jul 2021 12:20:25 -0700 (PDT) MIME-Version: 1.0 References: <20210627131134.5434-1-penguin-kernel@I-love.SAKURA.ne.jp> <9deece33-5d7f-9dcb-9aaa-94c60d28fc9a@i-love.sakura.ne.jp> <48d66166-4d39-4fe2-3392-7e0c84b9bdb3@i-love.sakura.ne.jp> In-Reply-To: <48d66166-4d39-4fe2-3392-7e0c84b9bdb3@i-love.sakura.ne.jp> From: Luiz Augusto von Dentz Date: Wed, 14 Jul 2021 12:20:15 -0700 Message-ID: Subject: Re: [PATCH v3] Bluetooth: call lock_sock() outside of spinlock section To: Tetsuo Handa Cc: Marcel Holtmann , Johan Hedberg , "linux-bluetooth@vger.kernel.org" , "David S. Miller" , Jakub Kicinski , Lin Ma , "open list:NETWORKING [GENERAL]" Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-bluetooth@vger.kernel.org Hi Tetsuo, On Tue, Jul 13, 2021 at 4:28 AM Tetsuo Handa wrote: > > syzbot is hitting might_sleep() warning at hci_sock_dev_event() due to > calling lock_sock() with rw spinlock held [1]. Among three possible > approaches [2], this patch chose holding a refcount via sock_hold() and > revalidating the element via sk_hashed(). > > Link: https://syzkaller.appspot.com/bug?extid=a5df189917e79d5e59c9 [1] > Link: https://lkml.kernel.org/r/05535d35-30d6-28b6-067e-272d01679d24@i-love.sakura.ne.jp [2] > Reported-by: syzbot > Signed-off-by: Tetsuo Handa > Tested-by: syzbot > Fixes: e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF of hdev object") > --- > Changes in v3: > Don't use unlocked hci_pi(sk)->hdev != hdev test, for it is racy. > No need to defer hci_dev_put(hdev), for it can't be the last reference. > > Changes in v2: > Take hci_sk_list.lock for write in case bt_sock_unlink() is called after > sk_hashed(sk) test, and defer hci_dev_put(hdev) till schedulable context. How about we revert back to use bh_lock_sock_nested but use local_bh_disable like the following patch: https://patchwork.kernel.org/project/bluetooth/patch/20210713162838.693266-1-desmondcheongzx@gmail.com/ > net/bluetooth/hci_sock.c | 30 +++++++++++++++++++++++++++++- > 1 file changed, 29 insertions(+), 1 deletion(-) > > diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c > index b04a5a02ecf3..786a06a232fd 100644 > --- a/net/bluetooth/hci_sock.c > +++ b/net/bluetooth/hci_sock.c > @@ -760,10 +760,18 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event) > struct sock *sk; > > /* Detach sockets from device */ > +restart: > read_lock(&hci_sk_list.lock); > sk_for_each(sk, &hci_sk_list.head) { > + /* This sock_hold(sk) is safe, for bt_sock_unlink(sk) > + * is not called yet. > + */ > + sock_hold(sk); > + read_unlock(&hci_sk_list.lock); > lock_sock(sk); > - if (hci_pi(sk)->hdev == hdev) { > + write_lock(&hci_sk_list.lock); > + /* Check that bt_sock_unlink(sk) is not called yet. */ > + if (sk_hashed(sk) && hci_pi(sk)->hdev == hdev) { > hci_pi(sk)->hdev = NULL; > sk->sk_err = EPIPE; > sk->sk_state = BT_OPEN; > @@ -771,7 +779,27 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event) > > hci_dev_put(hdev); > } > + write_unlock(&hci_sk_list.lock); > release_sock(sk); > + read_lock(&hci_sk_list.lock); > + /* If bt_sock_unlink(sk) is not called yet, we can > + * continue iteration. We can use __sock_put(sk) here > + * because hci_sock_release() will call sock_put(sk) > + * after bt_sock_unlink(sk). > + */ > + if (sk_hashed(sk)) { > + __sock_put(sk); > + continue; > + } > + /* Otherwise, we need to restart iteration, for the > + * next socket pointed by sk->next might be already > + * gone. We can't use __sock_put(sk) here because > + * hci_sock_release() might have already called > + * sock_put(sk) after bt_sock_unlink(sk). > + */ > + read_unlock(&hci_sk_list.lock); > + sock_put(sk); > + goto restart; > } > read_unlock(&hci_sk_list.lock); > } > -- > 2.18.4 > -- Luiz Augusto von Dentz