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

[Helge Deller]

Hi Linus,

On 02.06.2015 03:49, Linus Torvalds wrote:
> 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"

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/