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

[Linus Torvalds]

On Mon, Jun 1, 2015 at 11:44 AM, Helge Deller <deller@xxxxxx> 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
 kernel/compat.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/compat.c b/kernel/compat.c
index 24f00610c575..333d364be29d 100644
--- a/kernel/compat.c
+++ b/kernel/compat.c
@@ -912,7 +912,8 @@ long compat_get_bitmap(unsigned long *mask, const compat_ulong_t __user *umask,
 			 * bitmap. We must however ensure the end of the
 			 * kernel bitmap is zeroed.
 			 */
-			if (nr_compat_longs-- > 0) {
+			if (nr_compat_longs) {
+				nr_compat_longs--;
 				if (__get_user(um, umask))
 					return -EFAULT;
 			} else {
@@ -954,7 +955,8 @@ long compat_put_bitmap(compat_ulong_t __user *umask, unsigned long *mask,
 			 * We dont want to write past the end of the userspace
 			 * bitmap.
 			 */
-			if (nr_compat_longs-- > 0) {
+			if (nr_compat_longs) {
+				nr_compat_longs--;
 				if (__put_user(um, umask))
 					return -EFAULT;
 			}