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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 17 Apr 2015 16:18:20 -0400
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Marcel Holtmann <marcel@...tmann.org>
Cc:	Jörg Otte <jrg.otte@...il.com>,
	"Gustavo F. Padovan" <gustavo@...ovan.org>,
	Johan Hedberg <johan.hedberg@...il.com>,
	"bluez mailin list (linux-bluetooth@...r.kernel.org)" 
	<linux-bluetooth@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [V4.1] Regression: Bluetooth mouse not working.

On Fri, Apr 17, 2015 at 1:02 PM, Marcel Holtmann <marcel@...tmann.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/