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
