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

[Helge Deller]

Hi Linus,

* Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>:
> On Thu, Jun 4, 2015 at 6:45 AM, Helge Deller <deller@xxxxxx> wrote:
> >
> > Do you want me to send it again cleaned up, or will you just take yours?
> 
> I'd prefer to get a re-send, I've already nuked the patch from me tree.

Sure.
The new patch is attached below.
If you think it's OK, you can either apply it, or alternatively just
pull it from my compat-bitmap-fix branch:

	git://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux.git  compat-bitmap-fix

Patch title is:
	compat: cleanup coding in compat_get_bitmap() and compat_put_bitmap()

 
> These days, 99% of the patches I write are throw-away stuff just for
> this kind of "how about this" email exchange.
> 
> I do it that way partly because I'm lazy (ok, _mostly_ because I'm
> lazy, except then the explanation emails like this are actually more
> work than just doing it myself - but I tell myself that it makes it
> easier for me be lazy in the long run ;^). But partly it's because it
> makes it more natural to credit the person who actually reported the
> issue (the patch itself is *much* less important than the fact that
> somebody noticed the problem in the first place), and also partly
> because sometimes the patches may need some extra tender care and
> loving ("Oh, the patch I sent out didn't actually compile, and it
> burnt down your house? Yeah, it was just a general 'wouldn't this way
> be nicer' thing, not a serious patch").
 
Fully understandable.

Thanks!
Helge


[PATCH] compat: cleanup coding in compat_get_bitmap() and compat_put_bitmap()

In the functions compat_get_bitmap() and compat_put_bitmap() the variable
nr_compat_longs stores how many compat_ulong_t words should be copied in a
loop.

The copy loop itself is this:
  if (nr_compat_longs-- > 0) {
      if (__get_user(um, umask)) return -EFAULT;
  } else {
      um = 0;
  }

Since nr_compat_longs gets unconditionally decremented in each loop and since
it's type is unsigned this could theoretically lead to out-of-bounds accesses
to userspace if nr_compat_longs wraps around to (unsigned)(-1).

Although the callers currently do not trigger out-of-bounds accesses, we should
better implement the loop in a safe way to completely avoid such warp-arounds.

Signed-off-by: Helge Deller <deller@xxxxxx>
Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>

diff --git a/kernel/compat.c b/kernel/compat.c
index 24f0061..333d364 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;
 			}


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