Hi Luiz, I sent an incomplete patch. I realized that there was a follow up to this patch and I have sent the revision titled "device: Disconnect att before creating a new one". It contains the explanation why the kernel reports connected 2 times as well. Please ignore this patch and review the other patch instead. Thanks. On Fri, Aug 21, 2020 at 10:04 AM Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx> wrote: > > Hi Sonny, > > On Thu, Aug 20, 2020 at 11:40 PM Sonny Sasaka <sonnysasaka@xxxxxxxxxxxx> wrote: > > > > 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); > > It might be better just returning instead of cleaning up just to > recreate the instance below or is there a problem reusing the existing > instance? btw we need to investigate why the kernel is reporting > connected 2 times in a row since that is probably a bug there. > > > dev->attrib = attrib; > > dev->att = g_attrib_get_att(attrib); > > > > -- > > 2.26.2 > > > > > -- > Luiz Augusto von Dentz
