linux-kernel - Re: [PATCH] compat: fix possible out-of-bound accesses in compat_get_bitmap() and compat_put

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 for Android: free password hash cracker in your pocket

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

Message-ID: <55705670.9000808@gmx.de>
Date:	Thu, 04 Jun 2015 15:45:20 +0200
From:	Helge Deller <deller@....de>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
CC:	Al Viro <viro@...iv.linux.org.uk>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] compat: fix possible out-of-bound accesses in compat_get_bitmap()
 and compat_put_bitmap()

Hi Linus,

On 02.06.2015 03:49, Linus Torvalds wrote:
> On Mon, Jun 1, 2015 at 11:44 AM, Helge Deller <deller@....de> wrote:
>>
>> Since nr_compat_longs gets unconditionally decremented in each loop, it's type
>> needs to be signed instead of unsigned to avoid possibly accessing userspace
>> memory behind the bitmap which shouldn't be accessed.
>
> I'd actually prefer to instead just make the decrement conditional,
> since that would seem to be the more obvious code. Make the logic be
> "iff I have more to go, do the access, and then decrement the counter"

That's fine for me.
I just wanted to keep the patch small, but your proposal makes the code
of course more human readable.
  
> Also, compat_put_bitmap() has the exact same code, and should have the same fix.
>
> Finally, I don't think this is an *actual* bug, just bad and stupid
> code. The thing is, the inner loop is only executed twice anyway, and
> on that last iteration where "nr_compat_longs" could go negative, the
> _outer_ loop will break out too. So there is no actual way we can
> enter the thing with nr_compat_longs <= 1 to begin with.
>
> So I don't think the code ever really actually overflows.

Yes, it probably doesn't overflows. It's not that easy to follow all code
paths (and take into account that the bitmap sizes might be different), but
the code in general looks clean.

>  I do agree
> that the code looks bad, so I think a patch like the attached would be
> a good idea. Not necessarily marked for stable, unless you can point
> out why I'm wrong about the edge condition.

No, your code proposal is fine.
Do you want me to send it again cleaned up, or will you just take yours?

Thanks!
Helge
--
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/