[PATCH] mm: rmap: fix use-after-free in __put_anon_vma

[PATCH] mm: rmap: fix use-after-free in __put_anon_vma

[Andrey Ryabinin]

While working address sanitizer for kernel I've discovered use-after-free
bug in __put_anon_vma.
For the last anon_vma, anon_vma->root freed before child anon_vma.
Later in anon_vma_free(anon_vma) we are referencing to already freed anon_vma->root
to check rwsem.
This patch puts freeing of child anon_vma before freeing of anon_vma->root.

Cc: stable@vger.kernel.org # v3.0+
Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>
---
 mm/rmap.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/mm/rmap.c b/mm/rmap.c
index 9c3e773..161bffc7 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1564,10 +1564,11 @@ void __put_anon_vma(struct anon_vma *anon_vma)
 {
 	struct anon_vma *root = anon_vma->root;
 
-	if (root != anon_vma && atomic_dec_and_test(&root->refcount))
+	if (root != anon_vma && atomic_dec_and_test(&root->refcount)) {
+		anon_vma_free(anon_vma);
 		anon_vma_free(root);
-
-	anon_vma_free(anon_vma);
+	} else
+		anon_vma_free(anon_vma);
 }
 
 static struct anon_vma *rmap_walk_anon_lock(struct page *page,
-- 
1.8.5.5

--
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: rmap: fix use-after-free in __put_anon_vma

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 1439 bytes --]

On Fri, Jun 06, 2014 at 03:30:55PM +0400, Andrey Ryabinin wrote:
> While working address sanitizer for kernel I've discovered use-after-free
> bug in __put_anon_vma.
> For the last anon_vma, anon_vma->root freed before child anon_vma.
> Later in anon_vma_free(anon_vma) we are referencing to already freed anon_vma->root
> to check rwsem.
> This patch puts freeing of child anon_vma before freeing of anon_vma->root.

Yes, I think that is right indeed.

Very hard to hit, but valid since not all callers hold rcu_read_lock().

> 
> Cc: stable@vger.kernel.org # v3.0+
> Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>
> ---
>  mm/rmap.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 9c3e773..161bffc7 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1564,10 +1564,11 @@ void __put_anon_vma(struct anon_vma *anon_vma)
>  {
>  	struct anon_vma *root = anon_vma->root;
>  
> -	if (root != anon_vma && atomic_dec_and_test(&root->refcount))
> +	if (root != anon_vma && atomic_dec_and_test(&root->refcount)) {
> +		anon_vma_free(anon_vma);
>  		anon_vma_free(root);
> -
> -	anon_vma_free(anon_vma);
> +	} else
> +		anon_vma_free(anon_vma);
>  }

Why not simply move the freeing of anon_vma before the root, like:

	anon_vma_free(anon_vma);
	if (root != anon_vma && atomic_dec_and_test(&root->refcount))
		anon_vma_free(root);

?

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] mm: rmap: fix use-after-free in __put_anon_vma

[Andrey Ryabinin]

On 06/06/14 15:56, Peter Zijlstra wrote:
> On Fri, Jun 06, 2014 at 03:30:55PM +0400, Andrey Ryabinin wrote:
>> While working address sanitizer for kernel I've discovered use-after-free
>> bug in __put_anon_vma.
>> For the last anon_vma, anon_vma->root freed before child anon_vma.
>> Later in anon_vma_free(anon_vma) we are referencing to already freed anon_vma->root
>> to check rwsem.
>> This patch puts freeing of child anon_vma before freeing of anon_vma->root.
> 
> Yes, I think that is right indeed.
> 
> Very hard to hit, but valid since not all callers hold rcu_read_lock().
> 
>>
>> Cc: stable@vger.kernel.org # v3.0+
>> Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>
>> ---
>>  mm/rmap.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/rmap.c b/mm/rmap.c
>> index 9c3e773..161bffc7 100644
>> --- a/mm/rmap.c
>> +++ b/mm/rmap.c
>> @@ -1564,10 +1564,11 @@ void __put_anon_vma(struct anon_vma *anon_vma)
>>  {
>>  	struct anon_vma *root = anon_vma->root;
>>  
>> -	if (root != anon_vma && atomic_dec_and_test(&root->refcount))
>> +	if (root != anon_vma && atomic_dec_and_test(&root->refcount)) {
>> +		anon_vma_free(anon_vma);
>>  		anon_vma_free(root);
>> -
>> -	anon_vma_free(anon_vma);
>> +	} else
>> +		anon_vma_free(anon_vma);
>>  }
> 
> Why not simply move the freeing of anon_vma before the root, like:
> 
> 	anon_vma_free(anon_vma);
> 	if (root != anon_vma && atomic_dec_and_test(&root->refcount))
> 		anon_vma_free(root);
> 
> ?
> 

IMO It looks more logical to decrement root's refcounter before freeing child vma.
In fact I wasn't completely sure that it is safe to do so. But after some digging,
now it looks safe to me.







--
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 v2] mm: rmap: fix use-after-free in __put_anon_vma

[Andrey Ryabinin]

While working address sanitizer for kernel I've discovered use-after-free
bug in __put_anon_vma.
For the last anon_vma, anon_vma->root freed before child anon_vma.
Later in anon_vma_free(anon_vma) we are referencing to already freed anon_vma->root
to check rwsem.
This patch puts freeing of child anon_vma before freeing of anon_vma->root.

Cc: <stable@vger.kernel.org> # v3.0+
Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>
---

Changes since v1:
 - just made it more simple following Peter's suggestion

 mm/rmap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/rmap.c b/mm/rmap.c
index 9c3e773..cb5f70a 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1564,10 +1564,10 @@ void __put_anon_vma(struct anon_vma *anon_vma)
 {
 	struct anon_vma *root = anon_vma->root;
 
+	anon_vma_free(anon_vma);
+
 	if (root != anon_vma && atomic_dec_and_test(&root->refcount))
 		anon_vma_free(root);
-
-	anon_vma_free(anon_vma);
 }
 
 static struct anon_vma *rmap_walk_anon_lock(struct page *page,
-- 
1.8.5.5

--
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 v2] mm: rmap: fix use-after-free in __put_anon_vma

[Peter Zijlstra]

On Fri, Jun 06, 2014 at 07:09:30PM +0400, Andrey Ryabinin wrote:
> While working address sanitizer for kernel I've discovered use-after-free
> bug in __put_anon_vma.
> For the last anon_vma, anon_vma->root freed before child anon_vma.
> Later in anon_vma_free(anon_vma) we are referencing to already freed anon_vma->root
> to check rwsem.
> This patch puts freeing of child anon_vma before freeing of anon_vma->root.
> 
> Cc: <stable@vger.kernel.org> # v3.0+

Acked-by: Peter Zijlstra <peterz@infradead.org>

> Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>
> ---
> 
> Changes since v1:
>  - just made it more simple following Peter's suggestion
> 
>  mm/rmap.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 9c3e773..cb5f70a 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1564,10 +1564,10 @@ void __put_anon_vma(struct anon_vma *anon_vma)
>  {
>  	struct anon_vma *root = anon_vma->root;
>  
> +	anon_vma_free(anon_vma);
> +
>  	if (root != anon_vma && atomic_dec_and_test(&root->refcount))
>  		anon_vma_free(root);
> -
> -	anon_vma_free(anon_vma);
>  }
>  
>  static struct anon_vma *rmap_walk_anon_lock(struct page *page,
> -- 
> 1.8.5.5
> 

--
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 v2] mm: rmap: fix use-after-free in __put_anon_vma

[David Rientjes]

On Fri, 6 Jun 2014, Andrey Ryabinin wrote:

> While working address sanitizer for kernel I've discovered use-after-free
> bug in __put_anon_vma.
> For the last anon_vma, anon_vma->root freed before child anon_vma.
> Later in anon_vma_free(anon_vma) we are referencing to already freed anon_vma->root
> to check rwsem.
> This patch puts freeing of child anon_vma before freeing of anon_vma->root.
> 
> Cc: <stable@vger.kernel.org> # v3.0+
> Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>

Acked-by: David Rientjes <rientjes@google.com>

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