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

[Linus Torvalds]

On Thu, Jun 4, 2015 at 5:36 PM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> So basically that thing will trigger only on the last pass through
> the outer loop.  The only way for it to trigger a wraparound would
> be to have sizeof(long)/sizeof(compat_long_t) greater than LONG_MAX,
> which is, not too likely.

No. You missed that the "nr_compat_longs-- > 0" thing is an *unsigned*
comparison.

So the bug would trigger whenever the last pass through the outer loop
would then have an inner loop that decremented it from 0 to ULONG_MAX.

However, it turns out that since sizeof(long)/sizeof(compat_long_t) in
practice is always just 2, the decrement itself can happen only twice,
and since we only ever enter with nr_compat_longs being non-zero, only
the second decrement can do that overflow thing. And since that's the
last iteration of the inner loop (_and_ the outer loop), it doesn't
matter that the value ends up having overflowed

So the code _works_. But it works almost by accident, and it sure as
hell does not need that "greater than LONG_MAX" thing you think.
Anything bigger than 2 would cause an active bug.

So the whole "nr_compat_longs-- > 0" is just broken. It happens to
work, but it is ugly and wrong. It's also pointless, when we might as
well just move the decrement into the conditional, and avoid the whole
"it could overflow/underflow" discussion entirely, and make the whole
signed-vs-unsigned a total non-issue.

So the patch is a good cleanup. It doesn't fix any actual bugs, but it
definitely fixes potential bugs (ie if somebody were to have a 32-bit
compat mode on a 128-bit native kernel - something we don't actually
have yet, but an example of a situation that would trigger the bug)

                        Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/