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

[Al Viro]

On Fri, Jun 05, 2015 at 09:49:46AM -0700, Linus Torvalds wrote:
> 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.

Right you are.

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

Yes, but... if we decrement that sucker at all, why do we need to play with
i at all?  We need exactly nr_compat_longs get_user(), so why not make _that_
the condition in the single-level loop?  Just do store when we would've started
to shift by BITS_PER_LONG (and reset the shift, obviously).  And instead of
this if (), just check if we have something collected in 'm' once after the
end of loop.  AFAICS, it's faster and not harder to understand that way.
What I suggest is
	m = 0
	shift = 0
	repeat nr_compat_long times
		fetch um, return -EFAULT if that fails
		m |= (unsigned long)um << shift
		shift += BITS_PER_COMPAT_LONG
		if (shift == BITS_PER_LONG)
			shift = 0
			store m
	if (shift)
		store m
--
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/