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;
From:   Sonny Sasaka <sonnysasaka@chromium.org>
To:     linux-bluetooth@vger.kernel.org
Cc:     Sonny Sasaka <sonnysasaka@chromium.org>
Subject: [PATCH BlueZ] device: Cleanup att of a device object before assigning a new one.
Date:   Thu, 20 Aug 2020 23:38:44 -0700
Message-Id: <20200821063844.17349-1-sonnysasaka@chromium.org>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

For some unknown reason, sometimes the controller replies "Command
Disallowed" to a Disconnect command. When this happens, bluez kernel
strangely reports 2 MGMT_EV_DEVICE_CONNECTED events to bluetoothd.
Unfortunately bluetoothd doesn't handle this case so this situation will
lead to bluetoothd crashing due to UAF at later time.

This patch protects this situation by always cleaning up the att of a
device object before assigning a new one. This way the old att will not
at later time fire disconnect event which would operate on the already
freed device pointer.

Tested by repeatedly connecting/disconnecting to a device until the
situation happens and checking that bluetoothd doesn't crash.

---
 src/device.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/device.c b/src/device.c
index 7b7808405..e696ba1c6 100644
--- a/src/device.c
+++ b/src/device.c
@@ -5304,6 +5304,12 @@ bool device_attach_att(struct btd_device *dev, GIOChannel *io)
 		return false;
 	}
 
+	// This may be reached when the device already has att attached to it.
+	// In this case cleanup the att first before assigning the new one,
+	// otherwise the old att may fire a disconnect event at later time
+	// and will invoke operations on the already freed device pointer.
+	if (dev->attrib || dev->att)
+		attio_cleanup(dev);
 	dev->attrib = attrib;
 	dev->att = g_attrib_get_att(attrib);
 
-- 
2.26.2