Received-SPF: pass (google.com: best guess record for domain of linux-bluetooth-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sun, 26 Jan 2020 15:20:04 +0200
From:   Johan Hedberg <johan.hedberg@gmail.com>
To:     Dan Carpenter <dan.carpenter@oracle.com>
Cc:     Marcel Holtmann <marcel@holtmann.org>,
        syzbot <syzbot+eba992608adf3d796bcc@syzkaller.appspotmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        linux-bluetooth@vger.kernel.org, netdev@vger.kernel.org,
        syzkaller-bugs@googlegroups.com
Subject: Re: [PATCH] Bluetooth: Fix race condition in hci_release_sock()
Message-ID: <20200126132004.GA70786@pmessmer-mobl1.ger.corp.intel.com>
Mail-Followup-To: Dan Carpenter <dan.carpenter@oracle.com>,
        Marcel Holtmann <marcel@holtmann.org>,
        syzbot <syzbot+eba992608adf3d796bcc@syzkaller.appspotmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        linux-bluetooth@vger.kernel.org, netdev@vger.kernel.org,
        syzkaller-bugs@googlegroups.com
References: <00000000000012309d059c27b724@google.com>
 <20200115174903.shuanlfvnly3anqk@kili.mountain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200115174903.shuanlfvnly3anqk@kili.mountain>
User-Agent: Mutt/1.12.2 (2019-09-21)
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Dan,

On Wed, Jan 15, 2020, Dan Carpenter wrote:
> Syzbot managed to trigger a use after free "KASAN: use-after-free Write
> in hci_sock_bind".  I have reviewed the code manually and one possibly
> cause I have found is that we are not holding lock_sock(sk) when we do
> the hci_dev_put(hdev) in hci_sock_release().  My theory is that the bind
> and the release are racing against each other which results in this use
> after free.
> 
> Reported-by: syzbot+eba992608adf3d796bcc@syzkaller.appspotmail.com
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Not tested!  Please review very very carefully!
> 
> I feel like maybe someone should audit the (struct proto_ops)->release()
> functions because there may be similar bugs to this in other drivers.
> 
>  net/bluetooth/hci_sock.c | 3 +++
>  1 file changed, 3 insertions(+)

The patch looks correct to me since it's following the same locking
conventions as all the other socket callbacks in hci_sock.c. I also
verified that it doesn't cause regressions in the various socket testers
we have in user space BlueZ (e.g. tools/l2cap-tester). The patch is now
applied to the bluetooth-next tree. Thanks!

Johan