[PATCH] mm/slub.c: Fix wrong address during slab padding restoration

[PATCH] mm/slub.c: Fix wrong address during slab padding restoration

[Balasubramani Vivekanandan]

From: Balasubramani Vivekanandan <balasubramani_vivekanandan@mentor.com>

Start address calculated for slab padding restoration was wrong.
Wrong address would point to some section before padding and
could cause corruption

Signed-off-by: Balasubramani Vivekanandan <balasubramani_vivekanandan@mentor.com>
---
 mm/slub.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index cfd56e5..733ba32 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -838,6 +838,7 @@ static int slab_pad_check(struct kmem_cache *s, struct page *page)
 	u8 *start;
 	u8 *fault;
 	u8 *end;
+	u8 *pad;
 	int length;
 	int remainder;
 
@@ -851,8 +852,9 @@ static int slab_pad_check(struct kmem_cache *s, struct page *page)
 	if (!remainder)
 		return 1;
 
+	pad = end - remainder;
 	metadata_access_enable();
-	fault = memchr_inv(end - remainder, POISON_INUSE, remainder);
+	fault = memchr_inv(pad, POISON_INUSE, remainder);
 	metadata_access_disable();
 	if (!fault)
 		return 1;
@@ -860,9 +862,9 @@ static int slab_pad_check(struct kmem_cache *s, struct page *page)
 		end--;
 
 	slab_err(s, page, "Padding overwritten. 0x%p-0x%p", fault, end - 1);
-	print_section(KERN_ERR, "Padding ", end - remainder, remainder);
+	print_section(KERN_ERR, "Padding ", pad, remainder);
 
-	restore_bytes(s, "slab padding", POISON_INUSE, end - remainder, end);
+	restore_bytes(s, "slab padding", POISON_INUSE, fault, end);
 	return 0;
 }
 
-- 
2.7.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/slub.c: Fix wrong address during slab padding restoration

[Andrew Morton]

On Mon, 22 Jan 2018 12:32:58 +0530 Balasubramani Vivekanandan <balasubramani_vivekanandan@mentor.com> wrote:

> From: Balasubramani Vivekanandan <balasubramani_vivekanandan@mentor.com>
> 
> Start address calculated for slab padding restoration was wrong.
> Wrong address would point to some section before padding and
> could cause corruption
> 
> ...
>
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -838,6 +838,7 @@ static int slab_pad_check(struct kmem_cache *s, struct page *page)
>  	u8 *start;
>  	u8 *fault;
>  	u8 *end;
> +	u8 *pad;
>  	int length;
>  	int remainder;
>  
> @@ -851,8 +852,9 @@ static int slab_pad_check(struct kmem_cache *s, struct page *page)
>  	if (!remainder)
>  		return 1;
>  
> +	pad = end - remainder;
>  	metadata_access_enable();
> -	fault = memchr_inv(end - remainder, POISON_INUSE, remainder);
> +	fault = memchr_inv(pad, POISON_INUSE, remainder);
>  	metadata_access_disable();
>  	if (!fault)
>  		return 1;
> @@ -860,9 +862,9 @@ static int slab_pad_check(struct kmem_cache *s, struct page *page)
>  		end--;
>  
>  	slab_err(s, page, "Padding overwritten. 0x%p-0x%p", fault, end - 1);
> -	print_section(KERN_ERR, "Padding ", end - remainder, remainder);
> +	print_section(KERN_ERR, "Padding ", pad, remainder);
>  
> -	restore_bytes(s, "slab padding", POISON_INUSE, end - remainder, end);
> +	restore_bytes(s, "slab padding", POISON_INUSE, fault, end);
>  	return 0;
>  }

I don't see why it matters?  The current code will overwrite
POISON_INUSE bytes with POISON_INUSE, won't it?

That's a bit strange but not incorrect?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/slub.c: Fix wrong address during slab padding restoration

[Balasubramani Vivekanandan]

On Tue, Jan 23, 2018 at 02:50:26PM -0800, Andrew Morton wrote:
> On Mon, 22 Jan 2018 12:32:58 +0530 Balasubramani Vivekanandan <balasubramani_vivekanandan@mentor.com> wrote:
> 
> > From: Balasubramani Vivekanandan <balasubramani_vivekanandan@mentor.com>
> > 
> > Start address calculated for slab padding restoration was wrong.
> > Wrong address would point to some section before padding and
> > could cause corruption
> > 
> > ...
> >
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -838,6 +838,7 @@ static int slab_pad_check(struct kmem_cache *s, struct page *page)
> >  	u8 *start;
> >  	u8 *fault;
> >  	u8 *end;
> > +	u8 *pad;
> >  	int length;
> >  	int remainder;
> >  
> > @@ -851,8 +852,9 @@ static int slab_pad_check(struct kmem_cache *s, struct page *page)
> >  	if (!remainder)
> >  		return 1;
> >  
> > +	pad = end - remainder;
> >  	metadata_access_enable();
> > -	fault = memchr_inv(end - remainder, POISON_INUSE, remainder);
> > +	fault = memchr_inv(pad, POISON_INUSE, remainder);
> >  	metadata_access_disable();
> >  	if (!fault)
> >  		return 1;
> > @@ -860,9 +862,9 @@ static int slab_pad_check(struct kmem_cache *s, struct page *page)
> >  		end--;
> >  
> >  	slab_err(s, page, "Padding overwritten. 0x%p-0x%p", fault, end - 1);
> > -	print_section(KERN_ERR, "Padding ", end - remainder, remainder);
> > +	print_section(KERN_ERR, "Padding ", pad, remainder);
> >  
> > -	restore_bytes(s, "slab padding", POISON_INUSE, end - remainder, end);
> > +	restore_bytes(s, "slab padding", POISON_INUSE, fault, end);
> >  	return 0;
> >  }
> 
> I don't see why it matters?  The current code will overwrite
> POISON_INUSE bytes with POISON_INUSE, won't it?
> 
> That's a bit strange but not incorrect?
Not really. The bug will overwrite into the object area with
POISON_INUSE.
The end pointer initially points to end of the padding area. Then
in the loop, end is decremented till it points to the end of the fault
area.

while (end > fault && end[-1] == POISON_INUSE)
	end--;

Now using end - remainder, will not point to the begining of the padding
area but will sneak into the object area. So restore_bytes will
overwrite the object area

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

(no subject)

[Balasubramani Vivekanandan]

The start address sent to restore_bytes function is wrong. It points to
an location above the padding section. This is fixed in the attached patch

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH] mm/slub.c: Fix wrong address during slab padding restoration

[Balasubramani Vivekanandan]

From: Balasubramani Vivekanandan <balasubramani_vivekanandan@mentor.com>

Start address calculated for slab padding restoration was wrong.
Wrong address would point to some section before padding and
could cause corruption

Signed-off-by: Balasubramani Vivekanandan <balasubramani_vivekanandan@mentor.com>
---
 mm/slub.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index cfd56e5..733ba32 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -838,6 +838,7 @@ static int slab_pad_check(struct kmem_cache *s, struct page *page)
 	u8 *start;
 	u8 *fault;
 	u8 *end;
+	u8 *pad;
 	int length;
 	int remainder;
 
@@ -851,8 +852,9 @@ static int slab_pad_check(struct kmem_cache *s, struct page *page)
 	if (!remainder)
 		return 1;
 
+	pad = end - remainder;
 	metadata_access_enable();
-	fault = memchr_inv(end - remainder, POISON_INUSE, remainder);
+	fault = memchr_inv(pad, POISON_INUSE, remainder);
 	metadata_access_disable();
 	if (!fault)
 		return 1;
@@ -860,9 +862,9 @@ static int slab_pad_check(struct kmem_cache *s, struct page *page)
 		end--;
 
 	slab_err(s, page, "Padding overwritten. 0x%p-0x%p", fault, end - 1);
-	print_section(KERN_ERR, "Padding ", end - remainder, remainder);
+	print_section(KERN_ERR, "Padding ", pad, remainder);
 
-	restore_bytes(s, "slab padding", POISON_INUSE, end - remainder, end);
+	restore_bytes(s, "slab padding", POISON_INUSE, fault, end);
 	return 0;
 }
 
-- 
2.7.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>