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=C3=B6rg
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 *re=
q,
> struct socket *ctrl_sock,
> struct socket *intr_sock)
> {
> - u32 valid_flags =3D 0;
> + u32 valid_flags =3D 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 w=
hat 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 =3D 0;
+ u32 valid_flags =3D 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=C3=B6rg
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
On Fri, Apr 17, 2015 at 4:35 PM, Marcel Holtmann <[email protected]> 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
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
On Fri, Apr 17, 2015 at 1:02 PM, Marcel Holtmann <[email protected]> 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
Hi Joerg,
>>>>>> On Fri, Apr 17, 2015 at 5:36 AM, Jörg Otte <[email protected]> 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 <[email protected]>
>>>> 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 <[email protected]>
>>>> Signed-off-by: Johan Hedberg <[email protected]>
>>>>
>>>> :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
Hi Joerg,
>>>>> On Fri, Apr 17, 2015 at 5:36 AM, Jörg Otte <[email protected]> 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 <[email protected]>
>>> 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 <[email protected]>
>>> Signed-off-by: Johan Hedberg <[email protected]>
>>>
>>> :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
2015-04-17 18:55 GMT+02:00 J=C3=B6rg Otte <[email protected]>:
> 2015-04-17 18:51 GMT+02:00 Marcel Holtmann <[email protected]>:
>> Hi Joerg,
>>
>>>>> On Fri, Apr 17, 2015 at 5:36 AM, J=C3=B6rg Otte <[email protected]> =
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=C3=B6rg
>>>
>>> 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 <[email protected]>
>>> 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 p=
uts
>>> extra checks in place to ensure this.
>>>
>>> Signed-off-by: Marcel Holtmann <[email protected]>
>>> Signed-off-by: Johan Hedberg <[email protected]>
>>>
>>> :040000 040000 b51ac3634c9d44f4d9df0e7f548b524954b99c76
>>> 63bfb47283609849f1b3b8f05fe61743ccddfee6 M net
>>
>> thanks for bi-secting this. I looked at our existing userspace and restr=
icted it to the flags that are currently in use. However it seems that I ma=
de a mistake. What version of BlueZ userspace are you using (bluetoothd --v=
ersion).
>>
>> Regards
>>
>> Marcel
>>
> bluetoothd --version
> 4.98
>
> Thanks, J=C3=B6rg
Just reverted that commit. It fixed the problem for me.
Thanks, J=C3=B6rg
2015-04-17 18:51 GMT+02:00 Marcel Holtmann <[email protected]>:
> Hi Joerg,
>
>>>> On Fri, Apr 17, 2015 at 5:36 AM, J=C3=B6rg Otte <[email protected]> w=
rote:
>>>>> 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=C3=B6rg
>>
>> 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 <[email protected]>
>> 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 pu=
ts
>> extra checks in place to ensure this.
>>
>> Signed-off-by: Marcel Holtmann <[email protected]>
>> Signed-off-by: Johan Hedberg <[email protected]>
>>
>> :040000 040000 b51ac3634c9d44f4d9df0e7f548b524954b99c76
>> 63bfb47283609849f1b3b8f05fe61743ccddfee6 M net
>
> thanks for bi-secting this. I looked at our existing userspace and restri=
cted it to the flags that are currently in use. However it seems that I mad=
e a mistake. What version of BlueZ userspace are you using (bluetoothd --ve=
rsion).
>
> Regards
>
> Marcel
>
bluetoothd --version
4.98
Thanks, J=C3=B6rg
Hi Joerg,
>>> On Fri, Apr 17, 2015 at 5:36 AM, Jörg Otte <[email protected]> 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 <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Johan Hedberg <[email protected]>
>
> :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
2015-04-17 16:51 GMT+02:00 J=C3=B6rg Otte <[email protected]>:
> 2015-04-17 15:44 GMT+02:00 Linus Torvalds <[email protected]>=
:
>> On Fri, Apr 17, 2015 at 5:36 AM, J=C3=B6rg Otte <[email protected]> wro=
te:
>>> 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=C3=B6rg
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Johan Hedberg <[email protected]>
:040000 040000 b51ac3634c9d44f4d9df0e7f548b524954b99c76
63bfb47283609849f1b3b8f05fe61743ccddfee6 M net
Thanks, J=C3=B6rg
2015-04-17 15:44 GMT+02:00 Linus Torvalds <[email protected]>:
> On Fri, Apr 17, 2015 at 5:36 AM, J=C3=B6rg Otte <[email protected]> wrot=
e:
>> 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=C3=B6rg
2015-04-17 13:56 GMT+02:00 Johan Hedberg <[email protected]>:
> Hi J=C3=B6rg,
>
> (unnecessary people and lists dropped from CC)
>
> On Fri, Apr 17, 2015, J=C3=B6rg Otte wrote:
>> The BT mouse is "death" in v4.1.
>
> Could you be a bit more specific what you mean by v4.1, considering that
> it's not yet released (we don't even have a 4.1-rc1 yet).
The kernel is 4.0.0-07300-g4fc8adc.
The top of my tree is:
4fc8adc Merge branch 'for-linus' of
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs
84588e7 Merge branch 'for_linus' of
git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs
a39ef1a Merge branch 'for-4.1/drivers' of git://git.kernel.dk/linux-block
d82312c Merge branch 'for-4.1/core' of git://git.kernel.dk/linux-block
7d69cff Merge tag 'scsi-misc' of
git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi
8fa6f49 Merge branch 'mailbox-for-next' of
git://git.linaro.org/landing-teams/working/fujitsu/integration
...
>
>> 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.
>
> There was an initialization issue with 8087:07da that was already fixed,
> and the fix should be going to the 4.1 kernel. Could it be that you're
> simply missing this commit in your tree? It is this one:
>
> commit 407550fe2ccfca3fa0ac4bcdb0a412adeabf84ba
> Author: Marcel Holtmann <[email protected]>
> Date: Sun Feb 22 15:41:18 2015 -0800
>
> Bluetooth: btusb: Fix issue with CSR based Intel Wireless controllers
I already have this commit in the tree.
Thanks, J=C3=B6rg
On Fri, Apr 17, 2015 at 5:36 AM, J=C3=B6rg Otte <[email protected]> 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
Hi J?rg,
(unnecessary people and lists dropped from CC)
On Fri, Apr 17, 2015, J?rg Otte wrote:
> The BT mouse is "death" in v4.1.
Could you be a bit more specific what you mean by v4.1, considering that
it's not yet released (we don't even have a 4.1-rc1 yet).
> 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.
There was an initialization issue with 8087:07da that was already fixed,
and the fix should be going to the 4.1 kernel. Could it be that you're
simply missing this commit in your tree? It is this one:
commit 407550fe2ccfca3fa0ac4bcdb0a412adeabf84ba
Author: Marcel Holtmann <[email protected]>
Date: Sun Feb 22 15:41:18 2015 -0800
Bluetooth: btusb: Fix issue with CSR based Intel Wireless controllers
Johan
Hi J=C3=B6rg,
On Fri, Apr 17, 2015 at 12:36 PM, J=C3=B6rg Otte <[email protected]> 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.
--=20
Luiz Augusto von Dentz