[PATCH]: xarray: Fix potential out of bounds access

[PATCH]: xarray: Fix potential out of bounds access

[Cyrill Gorcunov]

Since the mark is used as an array index we should use
preincrement to not access the XA_MARK_MAX index.

Cc: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
Matthew, take a look please, I suspect we may access the
mark index out of allocated one. Compile tested only.
It comes from 58d6ea3085f2e53714810a513c61629f6d2be0a6

 lib/xarray.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-next.git/lib/xarray.c
===================================================================
--- linux-next.git.orig/lib/xarray.c
+++ linux-next.git/lib/xarray.c
@@ -129,7 +129,7 @@ static void xas_squash_marks(const struc
 			continue;
 		__set_bit(xas->xa_offset, marks);
 		bitmap_clear(marks, xas->xa_offset + 1, xas->xa_sibs);
-	} while (mark++ != (__force unsigned)XA_MARK_MAX);
+	} while (++mark != (__force unsigned)XA_MARK_MAX);
 }
 
 /* extracts the offset within this node from the index */

Re: [PATCH]: xarray: Fix potential out of bounds access

[Matthew Wilcox]

On Mon, Jan 14, 2019 at 09:47:41PM +0300, Cyrill Gorcunov wrote:
> Since the mark is used as an array index we should use
> preincrement to not access the XA_MARK_MAX index.

But XA_MARK_MAX is inclusive:

include/linux/xarray.h:#define XA_MARK_MAX              XA_MARK_2

so we actually want to access XA_MARK_MAX.  Now, we don't have a test
in the test-suite that fails as a result of your patch, so that needs to get
fixed.  How about this:

From: Matthew Wilcox <willy@infradead.org>
Date: Mon, 14 Jan 2019 13:57:31 -0500
Subject: [PATCH] XArray tests: Check mark 2 gets squashed

We do not currently check that the loop in xas_squash_marks() doesn't have
an off-by-one error in it.  It didn't, but a patch which introduced an
off-by-one error wasn't caught by any existing test.  Switch the roles
of XA_MARK_1 and XA_MARK_2 to catch that bug.

Reported-by: Cyrill Gorcunov <gorcunov@gmail.com>
Signed-off-by: Matthew Wilcox <willy@infradead.org>
---
 lib/test_xarray.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/test_xarray.c b/lib/test_xarray.c
index 3cf17338b0a4..c596a957f764 100644
--- a/lib/test_xarray.c
+++ b/lib/test_xarray.c
@@ -199,7 +199,7 @@ static noinline void check_xa_mark_1(struct xarray *xa, unsigned long index)
 		XA_BUG_ON(xa, xa_store_index(xa, index + 1, GFP_KERNEL));
 		xa_set_mark(xa, index + 1, XA_MARK_0);
 		XA_BUG_ON(xa, xa_store_index(xa, index + 2, GFP_KERNEL));
-		xa_set_mark(xa, index + 2, XA_MARK_1);
+		xa_set_mark(xa, index + 2, XA_MARK_2);
 		XA_BUG_ON(xa, xa_store_index(xa, next, GFP_KERNEL));
 		xa_store_order(xa, index, order, xa_mk_index(index),
 				GFP_KERNEL);
@@ -209,8 +209,8 @@ static noinline void check_xa_mark_1(struct xarray *xa, unsigned long index)
 			void *entry;
 
 			XA_BUG_ON(xa, !xa_get_mark(xa, i, XA_MARK_0));
-			XA_BUG_ON(xa, !xa_get_mark(xa, i, XA_MARK_1));
-			XA_BUG_ON(xa, xa_get_mark(xa, i, XA_MARK_2));
+			XA_BUG_ON(xa, xa_get_mark(xa, i, XA_MARK_1));
+			XA_BUG_ON(xa, !xa_get_mark(xa, i, XA_MARK_2));
 
 			/* We should see two elements in the array */
 			rcu_read_lock();
-- 
2.20.1

Re: [PATCH]: xarray: Fix potential out of bounds access

[Cyrill Gorcunov]

On Mon, Jan 14, 2019 at 11:01:20AM -0800, Matthew Wilcox wrote:
> On Mon, Jan 14, 2019 at 09:47:41PM +0300, Cyrill Gorcunov wrote:
> > Since the mark is used as an array index we should use
> > preincrement to not access the XA_MARK_MAX index.
> 
> But XA_MARK_MAX is inclusive:
> 
> include/linux/xarray.h:#define XA_MARK_MAX              XA_MARK_2

Indeed, I misread the variable name.

> so we actually want to access XA_MARK_MAX.  Now, we don't have a test
> in the test-suite that fails as a result of your patch, so that needs to get
> fixed.  How about this:

Looks great. Thank you!