[V4.1] Regression: Bluetooth mouse not working.

[V4.1] Regression: Bluetooth mouse not working.

[Jörg Otte]

The BT mouse is "death" in v4.1.
The BT mouse has been working in 4.0 and previous kernels, so this
is a regression.

BT adapter is an intel 8087:07da. The mouse is an MS Notebook Mouse 500.

It just doesn't work without any errors displayed in dmesg.

Thanks, Jörg

Re: [V4.1] Regression: Bluetooth mouse not working.

[Luiz Augusto von Dentz]

Hi Jörg,

On Fri, Apr 17, 2015 at 12:36 PM, Jörg Otte <jrg.otte@gmail.com> wrote:
> The BT mouse is "death" in v4.1.
> The BT mouse has been working in 4.0 and previous kernels, so this
> is a regression.
>
> BT adapter is an intel 8087:07da. The mouse is an MS Notebook Mouse 500.
>
> It just doesn't work without any errors displayed in dmesg.

We might need some logs, bluetoothd logs and btmon trace, to start
with, I don't recall any recent changes in HID just in HoG (Bluetooth
LE/Smart) which uses uHID.


-- 
Luiz Augusto von Dentz

Re: [V4.1] Regression: Bluetooth mouse not working.

[Linus Torvalds]

On Fri, Apr 17, 2015 at 5:36 AM, Jörg Otte <jrg.otte@gmail.com> wrote:
> The BT mouse is "death" in v4.1.
> The BT mouse has been working in 4.0 and previous kernels, so this
> is a regression.

Any chance of bisecting it?

              Linus

Re: [V4.1] Regression: Bluetooth mouse not working.

[Jörg Otte]

2015-04-17 15:44 GMT+02:00 Linus Torvalds <torvalds@linux-foundation.org>:
> On Fri, Apr 17, 2015 at 5:36 AM, Jörg Otte <jrg.otte@gmail.com> wrote:
>> The BT mouse is "death" in v4.1.
>> The BT mouse has been working in 4.0 and previous kernels, so this
>> is a regression.
>
> Any chance of bisecting it?
>
>               Linus
I will try that.

Thanks, Jörg

Re: [V4.1] Regression: Bluetooth mouse not working.

[Jörg Otte]

2015-04-17 16:51 GMT+02:00 Jörg Otte <jrg.otte@gmail.com>:
> 2015-04-17 15:44 GMT+02:00 Linus Torvalds <torvalds@linux-foundation.org>:
>> On Fri, Apr 17, 2015 at 5:36 AM, Jörg Otte <jrg.otte@gmail.com> wrote:
>>> The BT mouse is "death" in v4.1.
>>> The BT mouse has been working in 4.0 and previous kernels, so this
>>> is a regression.
>>
>> Any chance of bisecting it?
>>
>>               Linus
> I will try that.
>
> Thanks, Jörg

I first tried to bisect over all. But than I got an unbootable kernel.
Then I did a bisect over net/bluetooth and I get the following fist bad
commit:

5f5da99f1da5b01c7c45473a500c7dbb77a00958 is the first bad commit
commit 5f5da99f1da5b01c7c45473a500c7dbb77a00958
Author: Marcel Holtmann <marcel@holtmann.org>
Date:   Wed Apr 1 13:51:53 2015 -0700

    Bluetooth: Restrict HIDP flags to only valid ones

    The HIDP flags should be clearly restricted to valid ones. So this puts
    extra checks in place to ensure this.

    Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
    Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>

:040000 040000 b51ac3634c9d44f4d9df0e7f548b524954b99c76
63bfb47283609849f1b3b8f05fe61743ccddfee6 M      net


Thanks, Jörg

Re: [V4.1] Regression: Bluetooth mouse not working.

[Marcel Holtmann]

Hi Joerg,

>>> On Fri, Apr 17, 2015 at 5:36 AM, Jörg Otte <jrg.otte@gmail.com> wrote:
>>>> The BT mouse is "death" in v4.1.
>>>> The BT mouse has been working in 4.0 and previous kernels, so this
>>>> is a regression.
>>> 
>>> Any chance of bisecting it?
>>> 
>>>              Linus
>> I will try that.
>> 
>> Thanks, Jörg
> 
> I first tried to bisect over all. But than I got an unbootable kernel.
> Then I did a bisect over net/bluetooth and I get the following fist bad
> commit:
> 
> 5f5da99f1da5b01c7c45473a500c7dbb77a00958 is the first bad commit
> commit 5f5da99f1da5b01c7c45473a500c7dbb77a00958
> Author: Marcel Holtmann <marcel@holtmann.org>
> Date:   Wed Apr 1 13:51:53 2015 -0700
> 
>    Bluetooth: Restrict HIDP flags to only valid ones
> 
>    The HIDP flags should be clearly restricted to valid ones. So this puts
>    extra checks in place to ensure this.
> 
>    Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
>    Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> 
> :040000 040000 b51ac3634c9d44f4d9df0e7f548b524954b99c76
> 63bfb47283609849f1b3b8f05fe61743ccddfee6 M      net

thanks for bi-secting this. I looked at our existing userspace and restricted it to the flags that are currently in use. However it seems that I made a mistake. What version of BlueZ userspace are you using (bluetoothd --version).

Regards

Marcel

Re: [V4.1] Regression: Bluetooth mouse not working.

[Jörg Otte]

2015-04-17 18:51 GMT+02:00 Marcel Holtmann <marcel@holtmann.org>:
> Hi Joerg,
>
>>>> On Fri, Apr 17, 2015 at 5:36 AM, Jörg Otte <jrg.otte@gmail.com> wrote:
>>>>> The BT mouse is "death" in v4.1.
>>>>> The BT mouse has been working in 4.0 and previous kernels, so this
>>>>> is a regression.
>>>>
>>>> Any chance of bisecting it?
>>>>
>>>>              Linus
>>> I will try that.
>>>
>>> Thanks, Jörg
>>
>> I first tried to bisect over all. But than I got an unbootable kernel.
>> Then I did a bisect over net/bluetooth and I get the following fist bad
>> commit:
>>
>> 5f5da99f1da5b01c7c45473a500c7dbb77a00958 is the first bad commit
>> commit 5f5da99f1da5b01c7c45473a500c7dbb77a00958
>> Author: Marcel Holtmann <marcel@holtmann.org>
>> Date:   Wed Apr 1 13:51:53 2015 -0700
>>
>>    Bluetooth: Restrict HIDP flags to only valid ones
>>
>>    The HIDP flags should be clearly restricted to valid ones. So this puts
>>    extra checks in place to ensure this.
>>
>>    Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
>>    Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
>>
>> :040000 040000 b51ac3634c9d44f4d9df0e7f548b524954b99c76
>> 63bfb47283609849f1b3b8f05fe61743ccddfee6 M      net
>
> thanks for bi-secting this. I looked at our existing userspace and restricted it to the flags that are currently in use. However it seems that I made a mistake. What version of BlueZ userspace are you using (bluetoothd --version).
>
> Regards
>
> Marcel
>
bluetoothd --version
4.98

Thanks, Jörg

Re: [V4.1] Regression: Bluetooth mouse not working.

[Jörg Otte]

2015-04-17 18:55 GMT+02:00 Jörg Otte <jrg.otte@gmail.com>:
> 2015-04-17 18:51 GMT+02:00 Marcel Holtmann <marcel@holtmann.org>:
>> Hi Joerg,
>>
>>>>> On Fri, Apr 17, 2015 at 5:36 AM, Jörg Otte <jrg.otte@gmail.com> wrote:
>>>>>> The BT mouse is "death" in v4.1.
>>>>>> The BT mouse has been working in 4.0 and previous kernels, so this
>>>>>> is a regression.
>>>>>
>>>>> Any chance of bisecting it?
>>>>>
>>>>>              Linus
>>>> I will try that.
>>>>
>>>> Thanks, Jörg
>>>
>>> I first tried to bisect over all. But than I got an unbootable kernel.
>>> Then I did a bisect over net/bluetooth and I get the following fist bad
>>> commit:
>>>
>>> 5f5da99f1da5b01c7c45473a500c7dbb77a00958 is the first bad commit
>>> commit 5f5da99f1da5b01c7c45473a500c7dbb77a00958
>>> Author: Marcel Holtmann <marcel@holtmann.org>
>>> Date:   Wed Apr 1 13:51:53 2015 -0700
>>>
>>>    Bluetooth: Restrict HIDP flags to only valid ones
>>>
>>>    The HIDP flags should be clearly restricted to valid ones. So this puts
>>>    extra checks in place to ensure this.
>>>
>>>    Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
>>>    Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
>>>
>>> :040000 040000 b51ac3634c9d44f4d9df0e7f548b524954b99c76
>>> 63bfb47283609849f1b3b8f05fe61743ccddfee6 M      net
>>
>> thanks for bi-secting this. I looked at our existing userspace and restricted it to the flags that are currently in use. However it seems that I made a mistake. What version of BlueZ userspace are you using (bluetoothd --version).
>>
>> Regards
>>
>> Marcel
>>
> bluetoothd --version
> 4.98
>
> Thanks, Jörg

Just reverted that commit. It fixed the problem for me.

Thanks, Jörg

Re: [V4.1] Regression: Bluetooth mouse not working.

[Marcel Holtmann]

Hi Joerg,

>>>>> On Fri, Apr 17, 2015 at 5:36 AM, Jörg Otte <jrg.otte@gmail.com> wrote:
>>>>>> The BT mouse is "death" in v4.1.
>>>>>> The BT mouse has been working in 4.0 and previous kernels, so this
>>>>>> is a regression.
>>>>> 
>>>>> Any chance of bisecting it?
>>>>> 
>>>>>             Linus
>>>> I will try that.
>>>> 
>>>> Thanks, Jörg
>>> 
>>> I first tried to bisect over all. But than I got an unbootable kernel.
>>> Then I did a bisect over net/bluetooth and I get the following fist bad
>>> commit:
>>> 
>>> 5f5da99f1da5b01c7c45473a500c7dbb77a00958 is the first bad commit
>>> commit 5f5da99f1da5b01c7c45473a500c7dbb77a00958
>>> Author: Marcel Holtmann <marcel@holtmann.org>
>>> Date:   Wed Apr 1 13:51:53 2015 -0700
>>> 
>>>   Bluetooth: Restrict HIDP flags to only valid ones
>>> 
>>>   The HIDP flags should be clearly restricted to valid ones. So this puts
>>>   extra checks in place to ensure this.
>>> 
>>>   Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
>>>   Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
>>> 
>>> :040000 040000 b51ac3634c9d44f4d9df0e7f548b524954b99c76
>>> 63bfb47283609849f1b3b8f05fe61743ccddfee6 M      net
>> 
>> thanks for bi-secting this. I looked at our existing userspace and restricted it to the flags that are currently in use. However it seems that I made a mistake. What version of BlueZ userspace are you using (bluetoothd --version).
>> 
>> 
> bluetoothd --version
> 4.98

okay. I only looked at BlueZ 5.x and that might have been my mistake. Let me check this and fix this properly.

Regards

Marcel

Re: [V4.1] Regression: Bluetooth mouse not working.

[Marcel Holtmann]

Hi Joerg,

>>>>>> On Fri, Apr 17, 2015 at 5:36 AM, Jörg Otte <jrg.otte@gmail.com> wrote:
>>>>>>> The BT mouse is "death" in v4.1.
>>>>>>> The BT mouse has been working in 4.0 and previous kernels, so this
>>>>>>> is a regression.
>>>>>> 
>>>>>> Any chance of bisecting it?
>>>>>> 
>>>>>>            Linus
>>>>> I will try that.
>>>>> 
>>>>> Thanks, Jörg
>>>> 
>>>> I first tried to bisect over all. But than I got an unbootable kernel.
>>>> Then I did a bisect over net/bluetooth and I get the following fist bad
>>>> commit:
>>>> 
>>>> 5f5da99f1da5b01c7c45473a500c7dbb77a00958 is the first bad commit
>>>> commit 5f5da99f1da5b01c7c45473a500c7dbb77a00958
>>>> Author: Marcel Holtmann <marcel@holtmann.org>
>>>> Date:   Wed Apr 1 13:51:53 2015 -0700
>>>> 
>>>>  Bluetooth: Restrict HIDP flags to only valid ones
>>>> 
>>>>  The HIDP flags should be clearly restricted to valid ones. So this puts
>>>>  extra checks in place to ensure this.
>>>> 
>>>>  Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
>>>>  Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
>>>> 
>>>> :040000 040000 b51ac3634c9d44f4d9df0e7f548b524954b99c76
>>>> 63bfb47283609849f1b3b8f05fe61743ccddfee6 M      net
>>> 
>>> thanks for bi-secting this. I looked at our existing userspace and restricted it to the flags that are currently in use. However it seems that I made a mistake. What version of BlueZ userspace are you using (bluetoothd --version).
>>> 
>>> 
>> bluetoothd --version
>> 4.98
> 
> okay. I only looked at BlueZ 5.x and that might have been my mistake. Let me check this and fix this properly.

I think that I know what I screwed up here. I sent you a patch to fix this. Can you please test it and report back. If that fixes it for you, then I will send it to Linus for inclusion.

Regards

Marcel

Re: [V4.1] Regression: Bluetooth mouse not working.

[Linus Torvalds]

On Fri, Apr 17, 2015 at 1:02 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
>
> okay. I only looked at BlueZ 5.x and that might have been my mistake. Let me check this and fix this properly.

Why not just revert that commit. It looks like garbage. It has odd code like

+       u32 valid_flags = 0;
+       ci->flags = session->flags & valid_flags;

which is basically saying "no flags are valid, and we are silently
just clearing them all when copying".

The reason I think it's garbage is

 (a) the commit clearly breaks something, so the whole "let's check
flags that we've never checked before" is already fundamentally
suspicious

 (b) code like the above is just crap to begin with, because it makes
things superficially "look" sensible when looking at individual lines
of code (for example, when grepping things), and then when you look at
the actual bigger picture, it turns out that the code doesn't actually
care about the flags it is "copying", it just clears them all.

The other code sequences do things like

+       u32 valid_flags = 0;
+       if (req->flags & ~valid_flags)
+               return -EINVAL;

Which again is just a very unreadable way of saying "if any flags are
set, return an error". This kind of thing is presumably what breaks
things, because clearly people *have* set flags that you thought are
invalid.

Now *IF* the interfaces had had these kinds of flag validation checks
from day one, that would be one thing. But adding these kinds of
things after the fact, when somebody then reports that they break
things, then that's just a big big flag that you shouldn't try to do
this at all. It's water under the bridge. That ship has sailed. It's
too late. Give up on it.

So I don't think this code is "fixable". It really smells like a
fundamental mistake to begin with. Just revert it, chalk it up as "ok,
that was a stupid idea", and move on.

           Linus

Re: [V4.1] Regression: Bluetooth mouse not working.

[Marcel Holtmann]

Hi Linus,

>> okay. I only looked at BlueZ 5.x and that might have been my mistake. Let me check this and fix this properly.
> 
> Why not just revert that commit. It looks like garbage. It has odd code like
> 
> +       u32 valid_flags = 0;
> +       ci->flags = session->flags & valid_flags;
> 
> which is basically saying "no flags are valid, and we are silently
> just clearing them all when copying".
> 
> The reason I think it's garbage is
> 
> (a) the commit clearly breaks something, so the whole "let's check
> flags that we've never checked before" is already fundamentally
> suspicious
> 
> (b) code like the above is just crap to begin with, because it makes
> things superficially "look" sensible when looking at individual lines
> of code (for example, when grepping things), and then when you look at
> the actual bigger picture, it turns out that the code doesn't actually
> care about the flags it is "copying", it just clears them all.
> 
> The other code sequences do things like
> 
> +       u32 valid_flags = 0;
> +       if (req->flags & ~valid_flags)
> +               return -EINVAL;
> 
> Which again is just a very unreadable way of saying "if any flags are
> set, return an error". This kind of thing is presumably what breaks
> things, because clearly people *have* set flags that you thought are
> invalid.
> 
> Now *IF* the interfaces had had these kinds of flag validation checks
> from day one, that would be one thing. But adding these kinds of
> things after the fact, when somebody then reports that they break
> things, then that's just a big big flag that you shouldn't try to do
> this at all. It's water under the bridge. That ship has sailed. It's
> too late. Give up on it.
> 
> So I don't think this code is "fixable". It really smells like a
> fundamental mistake to begin with. Just revert it, chalk it up as "ok,
> that was a stupid idea", and move on.

accepting all flags regardless was an oversight on my part in the first place. What this patch tried to do is to limit it to what userspace is currently actually using. My mistake was to look only at BlueZ 5.x userspace and not at BlueZ 4.x userspace. The fix to not break existing userspace is essentially this:

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index a05b9dbf14c9..9070dfd6b4ad 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -1313,7 +1313,8 @@ int hidp_connection_add(struct hidp_connadd_req *req,
                        struct socket *ctrl_sock,
                        struct socket *intr_sock)
 {
-       u32 valid_flags = 0;
+       u32 valid_flags = BIT(HIDP_VIRTUAL_CABLE_UNPLUG) |
+                         BIT(HIDP_BOOT_PROTOCOL_MODE);

I ask Joerg to test this patch, but looking at old userspace is that is what is happening there.

Regards

Marcel

Re: [V4.1] Regression: Bluetooth mouse not working.

[Linus Torvalds]

On Fri, Apr 17, 2015 at 4:35 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
>
> accepting all flags regardless was an oversight on my part in the first place. What this patch tried to do is to limit it to what userspace is currently actually using. My mistake was to look only at BlueZ 5.x userspace and not at BlueZ 4.x userspace.

So what about anybody else? Android doesn't use BlueZ, afaik. Any
other direct accesses?

If we already know that BlueZ 4.x did something else, what makes you
so sure that this now covers all cases?

The thing is, the bluetooth code has clearly never cared about these
bits before. Is there any real reason to think that people haven't
passed in garbage? Do we even know that those flags were *initialized*
at all by user space in all use cases?

So I'm ok with trying to fix things up, but I have to say that if the
fixed-up case also causes problems (because there was some other case
that you didn't think of), I'm going to be pissed off, and I'm going
to expect you to *jump* on it, and revert the whole thing.

Ok?

          Linus

Re: [V4.1] Regression: Bluetooth mouse not working.

[Marcel Holtmann]

Hi Linus,

>> accepting all flags regardless was an oversight on my part in the first place. What this patch tried to do is to limit it to what userspace is currently actually using. My mistake was to look only at BlueZ 5.x userspace and not at BlueZ 4.x userspace.
> 
> So what about anybody else? Android doesn't use BlueZ, afaik. Any
> other direct accesses?

this interface is for bluetoothd (Bluetooth userspace daemon) since you need to do a lot of initial setup before you can hand this over to the kernel to drive HID. On Android this was never used. And even BlueZ for Android (replacement for Bluedroid) is not using it today either.

Google Fiber (their set-top boxes) actually moved this all over to /dev/uhid now since it gives them better re-connect experience for their remotes.

> If we already know that BlueZ 4.x did something else, what makes you
> so sure that this now covers all cases?

I am certain since nothing else than bluetoothd ever used this interface.

> The thing is, the bluetooth code has clearly never cared about these
> bits before. Is there any real reason to think that people haven't
> passed in garbage? Do we even know that those flags were *initialized*
> at all by user space in all use cases?
> 
> So I'm ok with trying to fix things up, but I have to say that if the
> fixed-up case also causes problems (because there was some other case
> that you didn't think of), I'm going to be pissed off, and I'm going
> to expect you to *jump* on it, and revert the whole thing.

The reason why I starting cleaning this up is because there is an overlay with internal and external flags mixed together. This is clearly a bug, but sadly that also can open up security issues since we clearly do not want userspace allowing messing with internal flags. That is actually worse.

My viewpoint is the reverting the whole patch is actually not helping here either. So either we take the patch that I just send around to fix the breakage that I caused with BlueZ 4.x userspace. Or an as alternative we keep allowing userspace to provide whatever flags it wants, but clear all unknown ones before using them in the HIDP logic. My intent was to make this old code less vulnerable.

Is one of these options acceptable for you compared to reverting the whole patch?

Regards

Marcel

Re: [V4.1] Regression: Bluetooth mouse not working.

[Jörg Otte]

What this patch tried to do is to limit it to what userspace is
currently actually using. My mistake was to look only at BlueZ 5.x
userspace and not at BlueZ 4.x userspace. The fix to not break
existing userspace is essentially this:
>
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index a05b9dbf14c9..9070dfd6b4ad 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -1313,7 +1313,8 @@ int hidp_connection_add(struct hidp_connadd_req *req,
>                         struct socket *ctrl_sock,
>                         struct socket *intr_sock)
>  {
> -       u32 valid_flags = 0;
> +       u32 valid_flags = BIT(HIDP_VIRTUAL_CABLE_UNPLUG) |
> +                         BIT(HIDP_BOOT_PROTOCOL_MODE);
>
> I ask Joerg to test this patch, but looking at old userspace is that is what is happening there.
>

I think the last 3 lines are missing, the complete patch is:

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index a05b9db..02298bc 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -1313,7 +1313,8 @@ int hidp_connection_add(struct hidp_connadd_req *req,
  struct socket *ctrl_sock,
  struct socket *intr_sock)
 {
- u32 valid_flags = 0;
+ u32 valid_flags = BIT(HIDP_VIRTUAL_CABLE_UNPLUG) |
+                         BIT(HIDP_BOOT_PROTOCOL_MODE);
  struct hidp_session *session;
  struct l2cap_conn *conn;
  struct l2cap_chan *chan;

that patch works for me.

Thanks, Jörg