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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CA+55aFw=Pag0YzBZRFz9nkxJWp5Me0EitdTGmWAvWQ5LX5tFZg@mail.gmail.com>
Date:	Mon, 1 Jun 2015 18:49:25 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Helge Deller <deller@....de>
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()

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"

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. 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.

Hmm?

                            Linus

View attachment "patch.diff" of type "text/plain" (873 bytes)