[PATCH BlueZ] device: Cleanup att of a device object before assigning a new one.

[PATCH BlueZ] device: Cleanup att of a device object before assigning a new one.

[Sonny Sasaka]

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

RE: [BlueZ] device: Cleanup att of a device object before assigning a new one.

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 431 bytes --]


This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
While we are preparing for reviewing the patches, we found the following
issue/warning.

Test Result:
checkgitlint Failed

Outputs:
1: T3 Title has trailing punctuation (.): "device: Cleanup att of a device object before assigning a new one."



---
Regards,
Linux Bluetooth

Re: [PATCH BlueZ] device: Cleanup att of a device object before assigning a new one.

[Luiz Augusto von Dentz]

Hi Sonny,

On Thu, Aug 20, 2020 at 11:40 PM Sonny Sasaka <sonnysasaka@chromium.org> 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

Re: [PATCH BlueZ] device: Cleanup att of a device object before assigning a new one.

[Sonny Sasaka]

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@gmail.com> wrote:
>
> Hi Sonny,
>
> On Thu, Aug 20, 2020 at 11:40 PM Sonny Sasaka <sonnysasaka@chromium.org> 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