On Mon, Jun 1, 2015 at 11:44 AM, Helge Deller <del...@gmx.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
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; }