[v2,6/8] mm: prevent get_user_pages() from overflowing page refcount

Commit Message

Ajay Kaher Oct. 9, 2019, 12:44 a.m. UTC

From: Linus Torvalds <[e-mail]>

commit 8fde12ca79aff9b5ba951fce1a2641901b8d8e64 upstream.

If the page refcount wraps around past zero, it will be freed while
there are still four billion references to it.  One of the possible
avenues for an attacker to try to make this happen is by doing direct IO
on a page multiple times.  This patch makes get_user_pages() refuse to
take a new page reference if there are already more than two billion
references to the page.

Reported-by: Jann Horn <[e-mail]>
Acked-by: Matthew Wilcox <[e-mail]>
Cc: [e-mail]
Signed-off-by: Linus Torvalds <[e-mail]>
[ 4.4.y backport notes:
  Ajay: Added local variable 'err' with-in follow_hugetlb_page()
        from 2be7cfed995e, to resolve compilation error
  Srivatsa: Replaced call to get_page_foll() with try_get_page_foll() ]
Signed-off-by: Srivatsa S. Bhat (VMware) <[e-mail]>
Signed-off-by: Ajay Kaher <[e-mail]>
---
 mm/gup.c     | 43 ++++++++++++++++++++++++++++++++-----------
 mm/hugetlb.c | 16 +++++++++++++++-
 2 files changed, 47 insertions(+), 12 deletions(-)

Comments

Vlastimil Babka Oct. 9, 2019, 1:13 p.m. UTC | #1

On 10/9/19 2:44 AM, Ajay Kaher wrote:
> From: Linus Torvalds <[e-mail]>
> 
> commit 8fde12ca79aff9b5ba951fce1a2641901b8d8e64 upstream.
> 
> If the page refcount wraps around past zero, it will be freed while
> there are still four billion references to it.  One of the possible
> avenues for an attacker to try to make this happen is by doing direct IO
> on a page multiple times.  This patch makes get_user_pages() refuse to
> take a new page reference if there are already more than two billion
> references to the page.
> 
> Reported-by: Jann Horn <[e-mail]>
> Acked-by: Matthew Wilcox <[e-mail]>
> Cc: [e-mail]
> Signed-off-by: Linus Torvalds <[e-mail]>
> [ 4.4.y backport notes:
>   Ajay: Added local variable 'err' with-in follow_hugetlb_page()
>         from 2be7cfed995e, to resolve compilation error
>   Srivatsa: Replaced call to get_page_foll() with try_get_page_foll() ]
> Signed-off-by: Srivatsa S. Bhat (VMware) <[e-mail]>
> Signed-off-by: Ajay Kaher <[e-mail]>
> ---
>  mm/gup.c     | 43 ++++++++++++++++++++++++++++++++-----------
>  mm/hugetlb.c | 16 +++++++++++++++-
>  2 files changed, 47 insertions(+), 12 deletions(-)

This seems to have the same issue as the 4.9 stable version [1], in not
touching the arch-specific gup.c variants.

[1]
https://lore.kernel.org/lkml/[e-mail]/

> diff --git a/mm/gup.c b/mm/gup.c
> index fae4d1e..171b460 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -126,8 +126,12 @@ retry:
>  		}
>  	}
>  
> -	if (flags & FOLL_GET)
> -		get_page_foll(page);
> +	if (flags & FOLL_GET) {
> +		if (unlikely(!try_get_page_foll(page))) {
> +			page = ERR_PTR(-ENOMEM);
> +			goto out;
> +		}
> +	}
>  	if (flags & FOLL_TOUCH) {
>  		if ((flags & FOLL_WRITE) &&
>  		    !pte_dirty(pte) && !PageDirty(page))
> @@ -289,7 +293,10 @@ static int get_gate_page(struct mm_struct *mm, unsigned long address,
>  			goto unmap;
>  		*page = pte_page(*pte);
>  	}
> -	get_page(*page);
> +	if (unlikely(!try_get_page(*page))) {
> +		ret = -ENOMEM;
> +		goto unmap;
> +	}
>  out:
>  	ret = 0;
>  unmap:
> @@ -1053,6 +1060,20 @@ struct page *get_dump_page(unsigned long addr)
>   */
>  #ifdef CONFIG_HAVE_GENERIC_RCU_GUP
>  
> +/*
> + * Return the compund head page with ref appropriately incremented,
> + * or NULL if that failed.
> + */
> +static inline struct page *try_get_compound_head(struct page *page, int refs)
> +{
> +	struct page *head = compound_head(page);
> +	if (WARN_ON_ONCE(atomic_read(&head->_count) < 0))
> +		return NULL;
> +	if (unlikely(!page_cache_add_speculative(head, refs)))
> +		return NULL;
> +	return head;
> +}
> +
>  #ifdef __HAVE_ARCH_PTE_SPECIAL
>  static int gup_pte_range(pmd_t pmd, unsigned long addr, unsigned long end,
>  			 int write, struct page **pages, int *nr)
> @@ -1082,9 +1103,9 @@ static int gup_pte_range(pmd_t pmd, unsigned long addr, unsigned long end,
>  
>  		VM_BUG_ON(!pfn_valid(pte_pfn(pte)));
>  		page = pte_page(pte);
> -		head = compound_head(page);
>  
> -		if (!page_cache_get_speculative(head))
> +		head = try_get_compound_head(page, 1);
> +		if (!head)
>  			goto pte_unmap;
>  
>  		if (unlikely(pte_val(pte) != pte_val(*ptep))) {
> @@ -1141,8 +1162,8 @@ static int gup_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr,
>  		refs++;
>  	} while (addr += PAGE_SIZE, addr != end);
>  
> -	head = compound_head(pmd_page(orig));
> -	if (!page_cache_add_speculative(head, refs)) {
> +	head = try_get_compound_head(pmd_page(orig), refs);
> +	if (!head) {
>  		*nr -= refs;
>  		return 0;
>  	}
> @@ -1187,8 +1208,8 @@ static int gup_huge_pud(pud_t orig, pud_t *pudp, unsigned long addr,
>  		refs++;
>  	} while (addr += PAGE_SIZE, addr != end);
>  
> -	head = compound_head(pud_page(orig));
> -	if (!page_cache_add_speculative(head, refs)) {
> +	head = try_get_compound_head(pud_page(orig), refs);
> +	if (!head) {
>  		*nr -= refs;
>  		return 0;
>  	}
> @@ -1229,8 +1250,8 @@ static int gup_huge_pgd(pgd_t orig, pgd_t *pgdp, unsigned long addr,
>  		refs++;
>  	} while (addr += PAGE_SIZE, addr != end);
>  
> -	head = compound_head(pgd_page(orig));
> -	if (!page_cache_add_speculative(head, refs)) {
> +	head = try_get_compound_head(pgd_page(orig), refs);
> +	if (!head) {
>  		*nr -= refs;
>  		return 0;
>  	}
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index fd932e7..3a1501e 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -3886,6 +3886,7 @@ long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma,
>  	unsigned long vaddr = *position;
>  	unsigned long remainder = *nr_pages;
>  	struct hstate *h = hstate_vma(vma);
> +	int err = -EFAULT;
>  
>  	while (vaddr < vma->vm_end && remainder) {
>  		pte_t *pte;
> @@ -3957,6 +3958,19 @@ long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma,
>  
>  		pfn_offset = (vaddr & ~huge_page_mask(h)) >> PAGE_SHIFT;
>  		page = pte_page(huge_ptep_get(pte));
> +
> +		/*
> +		 * Instead of doing 'try_get_page_foll()' below in the same_page
> +		 * loop, just check the count once here.
> +		 */
> +		if (unlikely(page_count(page) <= 0)) {
> +			if (pages) {
> +				spin_unlock(ptl);
> +				remainder = 0;
> +				err = -ENOMEM;
> +				break;
> +			}
> +		}
>  same_page:
>  		if (pages) {
>  			pages[i] = mem_map_offset(page, pfn_offset);
> @@ -3983,7 +3997,7 @@ same_page:
>  	*nr_pages = remainder;
>  	*position = vaddr;
>  
> -	return i ? i : -EFAULT;
> +	return i ? i : err;
>  }
>  
>  unsigned long hugetlb_change_protection(struct vm_area_struct *vma,
>

Ajay Kaher Oct. 17, 2019, 4:28 p.m. UTC | #2

On 09/10/19, 6:43 PM, "Vlastimil Babka" <[e-mail]> wrote:

>> Reported-by: Jann Horn <[e-mail]>
>> Acked-by: Matthew Wilcox <[e-mail]>
>> Cc: [e-mail]
>> Signed-off-by: Linus Torvalds <[e-mail]>
>> [ 4.4.y backport notes:
>>   Ajay: Added local variable 'err' with-in follow_hugetlb_page()
>>         from 2be7cfed995e, to resolve compilation error
>>   Srivatsa: Replaced call to get_page_foll() with try_get_page_foll() ]
>> Signed-off-by: Srivatsa S. Bhat (VMware) <[e-mail]>
>> Signed-off-by: Ajay Kaher <[e-mail]>
>> ---
>>  mm/gup.c     | 43 ++++++++++++++++++++++++++++++++-----------
>>  mm/hugetlb.c | 16 +++++++++++++++-
>>  2 files changed, 47 insertions(+), 12 deletions(-)
>    
> This seems to have the same issue as the 4.9 stable version [1], in not
> touching the arch-specific gup.c variants.
>    
> [1]
> https://lore.kernel.org/lkml/[e-mail]/

Thanks Vlastimil for highlighting this here.

Yes, arch-specific gup.c variants also need to handle not only for 4.4.y,
however it should be handled till 4.19.y. I believe it's better to start
from 4.19.y and then backport those changes till 4.4.y.

Affected areas of gup.c (where page->count have been used) are:
#1: get_page() used in these files and this is safe as
       it's defined in mm.h (here it's already taken care of)
#2: get_head_page_multiple() has following:
               VM_BUG_ON_PAGE(page_count(page) == 0, page);
           Need to change this to:
               VM_BUG_ON_PAGE(page_ref_zero_or_close_to_overflow(page), page);
#3: Some of the files have used page_cache_get_speculative(),
       page_cache_add_speculative() with combination of compound_head(),
       this scenario needs to be handled as it was handled here:
           https://lore.kernel.org/stable/[e-mail]/

Please share with me any suggestions or patches if you have already  
worked on this.

Could we handle arch-specific gup.c in different patch sets and 
let these patches to merge to 4.4.y?

- Ajay

Ajay Kaher Oct. 25, 2019, 6:18 a.m. UTC | #3

On 17/10/19, 9:58 PM, "Ajay Kaher" <[e-mail]> wrote:
    
> > This seems to have the same issue as the 4.9 stable version [1], in not
> > touching the arch-specific gup.c variants.
> >    
> > [1]
> > https://lore.kernel.org/lkml/[e-mail]/
>    
> Thanks Vlastimil for highlighting this here.
> 
> Yes, arch-specific gup.c variants also need to handle not only for 4.4.y,
> however it should be handled till 4.19.y. I believe it's better to start
> from 4.19.y and then backport those changes till 4.4.y.
>    
> Affected areas of gup.c (where page->count have been used) are:
> #1: get_page() used in these files and this is safe as
>        it's defined in mm.h (here it's already taken care of)
> #2: get_head_page_multiple() has following:
>               VM_BUG_ON_PAGE(page_count(page) == 0, page);
>          Need to change this to:
>               VM_BUG_ON_PAGE(page_ref_zero_or_close_to_overflow(page), page);
> #3: Some of the files have used page_cache_get_speculative(),
>        page_cache_add_speculative() with combination of compound_head(),
>        this scenario needs to be handled as it was handled here:
>            https://lore.kernel.org/stable/[e-mail]/
>    
> Please share with me any suggestions or patches if you have already  
> worked on this.
>    
> Could we handle arch-specific gup.c in different patch sets and 
> let these patches to merge to 4.4.y?
  
Vlastimil, please suggest if it's fine to merge these patches to 4.4.y
and handle arch-specific gup.c in different patch sets starts from 4.19.y,
then backport all the way to 4.4.y. 

Greg, any suggestion from your side.

>    - Ajay

Vlastimil Babka Nov. 6, 2019, 8:55 a.m. UTC | #4

On 10/25/19 8:18 AM, Ajay Kaher wrote:
> On 17/10/19, 9:58 PM, "Ajay Kaher" <[e-mail]> wrote:
>     
>> > This seems to have the same issue as the 4.9 stable version [1], in not
>> > touching the arch-specific gup.c variants.
>> >    
>> > [1]
>> > https://lore.kernel.org/lkml/[e-mail]/
>>    
>> Thanks Vlastimil for highlighting this here.
>> 
>> Yes, arch-specific gup.c variants also need to handle not only for 4.4.y,
>> however it should be handled till 4.19.y. I believe it's better to start
>> from 4.19.y and then backport those changes till 4.4.y.
>>    
>> Affected areas of gup.c (where page->count have been used) are:
>> #1: get_page() used in these files and this is safe as
>>        it's defined in mm.h (here it's already taken care of)
>> #2: get_head_page_multiple() has following:
>>               VM_BUG_ON_PAGE(page_count(page) == 0, page);
>>          Need to change this to:
>>               VM_BUG_ON_PAGE(page_ref_zero_or_close_to_overflow(page), page);
>> #3: Some of the files have used page_cache_get_speculative(),
>>        page_cache_add_speculative() with combination of compound_head(),
>>        this scenario needs to be handled as it was handled here:
>>            https://lore.kernel.org/stable/[e-mail]/
>>    
>> Please share with me any suggestions or patches if you have already  
>> worked on this.
>>    
>> Could we handle arch-specific gup.c in different patch sets and 
>> let these patches to merge to 4.4.y?
>   
> Vlastimil, please suggest if it's fine to merge these patches to 4.4.y

I'm not sure if it makes much sense to merge them without the arch-specific gup
support, when we're aware that it's missing.

> and handle arch-specific gup.c in different patch sets starts from 4.19.y,

Actually arch-specific gup.c were removed in 4.13, so it's enough to start from
4.9.y, which I'm going to finally look into.

> then backport all the way to 4.4.y. 
> 
> Greg, any suggestion from your side.
> 
>>    - Ajay
>     
>     
>     
>

Ajay Kaher Nov. 11, 2019, 5 a.m. UTC | #5

On 06/11/19, 2:25 PM, "Vlastimil Babka" <[e-mail]> wrote:

> On 10/25/19 8:18 AM, Ajay Kaher wrote:
>> On 17/10/19, 9:58 PM, "Ajay Kaher" <[e-mail]> wrote:
>>     
>>>    
>>> Could we handle arch-specific gup.c in different patch sets and 
>>> let these patches to merge to 4.4.y?
>>   
>> Vlastimil, please suggest if it's fine to merge these patches to 4.4.y
>
> I'm not sure if it makes much sense to merge them without the arch-specific gup
> support, when we're aware that it's missing.
>
>> and handle arch-specific gup.c in different patch sets starts from 4.19.y,
>
> Actually arch-specific gup.c were removed in 4.13, so it's enough to start from
> 4.9.y, which I'm going to finally look into.

Yes x86 gup.c is removed. s390 gup.c is present till 4.19,
so if you are making changes in this file for 4.4.y and 4.9.y, 
then same should be done for 4.14.y and v4.19.y.

- Ajay

Greg KH Nov. 21, 2019, 8:38 p.m. UTC | #6

On Mon, Nov 11, 2019 at 05:00:29AM +0000, Ajay Kaher wrote:
> 
> On 06/11/19, 2:25 PM, "Vlastimil Babka" <[e-mail]> wrote:
> 
> > On 10/25/19 8:18 AM, Ajay Kaher wrote:
> >> On 17/10/19, 9:58 PM, "Ajay Kaher" <[e-mail]> wrote:
> >>     
> >>>    
> >>> Could we handle arch-specific gup.c in different patch sets and 
> >>> let these patches to merge to 4.4.y?
> >>   
> >> Vlastimil, please suggest if it's fine to merge these patches to 4.4.y
> >
> > I'm not sure if it makes much sense to merge them without the arch-specific gup
> > support, when we're aware that it's missing.
> >
> >> and handle arch-specific gup.c in different patch sets starts from 4.19.y,
> >
> > Actually arch-specific gup.c were removed in 4.13, so it's enough to start from
> > 4.9.y, which I'm going to finally look into.
> 
> Yes x86 gup.c is removed. s390 gup.c is present till 4.19,
> so if you are making changes in this file for 4.4.y and 4.9.y, 
> then same should be done for 4.14.y and v4.19.y.

Ok, I have no idea what to do here.  I have two different series from
both of you, yet they are different.

Can you both come up with a series you agree on, and send it to me, with
both of your acks so that I know this is what should be applied?  I've
deleted both of your current series from my todo mbox.

thanks,

greg k-h

Vlastimil Babka Nov. 29, 2019, 9:08 a.m. UTC | #7

On 11/21/19 9:38 PM, [e-mail] wrote:
> On Mon, Nov 11, 2019 at 05:00:29AM +0000, Ajay Kaher wrote:
>>
>> On 06/11/19, 2:25 PM, "Vlastimil Babka" <[e-mail]> wrote:
>>
>>> On 10/25/19 8:18 AM, Ajay Kaher wrote:
>>>> On 17/10/19, 9:58 PM, "Ajay Kaher" <[e-mail]> wrote:
>>>>     
>>>>>    
>>>>> Could we handle arch-specific gup.c in different patch sets and 
>>>>> let these patches to merge to 4.4.y?
>>>>   
>>>> Vlastimil, please suggest if it's fine to merge these patches to 4.4.y
>>>
>>> I'm not sure if it makes much sense to merge them without the arch-specific gup
>>> support, when we're aware that it's missing.
>>>
>>>> and handle arch-specific gup.c in different patch sets starts from 4.19.y,
>>>
>>> Actually arch-specific gup.c were removed in 4.13, so it's enough to start from
>>> 4.9.y, which I'm going to finally look into.
>>
>> Yes x86 gup.c is removed. s390 gup.c is present till 4.19,
>> so if you are making changes in this file for 4.4.y and 4.9.y, 
>> then same should be done for 4.14.y and v4.19.y.
> 
> Ok, I have no idea what to do here.  I have two different series from
> both of you, yet they are different.
> 
> Can you both come up with a series you agree on, and send it to me, with
> both of your acks so that I know this is what should be applied?  I've
> deleted both of your current series from my todo mbox.

I started by fixing up [1] 4.9, 4.14 and 4.19 which already got the
backport, but without arch-specific gup.c variants. I'll wait for Ajay's
feedback for the 4.4 series.

[1] https://lore.kernel.org/linux-mm/[e-mail]/

> thanks,
> 
> greg k-h
>

Patch

diff --git a/mm/gup.c b/mm/gup.c
index fae4d1e..171b460 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -126,8 +126,12 @@  retry:
 		}
 	}
 
-	if (flags & FOLL_GET)
-		get_page_foll(page);
+	if (flags & FOLL_GET) {
+		if (unlikely(!try_get_page_foll(page))) {
+			page = ERR_PTR(-ENOMEM);
+			goto out;
+		}
+	}
 	if (flags & FOLL_TOUCH) {
 		if ((flags & FOLL_WRITE) &&
 		    !pte_dirty(pte) && !PageDirty(page))
@@ -289,7 +293,10 @@  static int get_gate_page(struct mm_struct *mm, unsigned long address,
 			goto unmap;
 		*page = pte_page(*pte);
 	}
-	get_page(*page);
+	if (unlikely(!try_get_page(*page))) {
+		ret = -ENOMEM;
+		goto unmap;
+	}
 out:
 	ret = 0;
 unmap:
@@ -1053,6 +1060,20 @@  struct page *get_dump_page(unsigned long addr)
  */
 #ifdef CONFIG_HAVE_GENERIC_RCU_GUP
 
+/*
+ * Return the compund head page with ref appropriately incremented,
+ * or NULL if that failed.
+ */
+static inline struct page *try_get_compound_head(struct page *page, int refs)
+{
+	struct page *head = compound_head(page);
+	if (WARN_ON_ONCE(atomic_read(&head->_count) < 0))
+		return NULL;
+	if (unlikely(!page_cache_add_speculative(head, refs)))
+		return NULL;
+	return head;
+}
+
 #ifdef __HAVE_ARCH_PTE_SPECIAL
 static int gup_pte_range(pmd_t pmd, unsigned long addr, unsigned long end,
 			 int write, struct page **pages, int *nr)
@@ -1082,9 +1103,9 @@  static int gup_pte_range(pmd_t pmd, unsigned long addr, unsigned long end,
 
 		VM_BUG_ON(!pfn_valid(pte_pfn(pte)));
 		page = pte_page(pte);
-		head = compound_head(page);
 
-		if (!page_cache_get_speculative(head))
+		head = try_get_compound_head(page, 1);
+		if (!head)
 			goto pte_unmap;
 
 		if (unlikely(pte_val(pte) != pte_val(*ptep))) {
@@ -1141,8 +1162,8 @@  static int gup_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr,
 		refs++;
 	} while (addr += PAGE_SIZE, addr != end);
 
-	head = compound_head(pmd_page(orig));
-	if (!page_cache_add_speculative(head, refs)) {
+	head = try_get_compound_head(pmd_page(orig), refs);
+	if (!head) {
 		*nr -= refs;
 		return 0;
 	}
@@ -1187,8 +1208,8 @@  static int gup_huge_pud(pud_t orig, pud_t *pudp, unsigned long addr,
 		refs++;
 	} while (addr += PAGE_SIZE, addr != end);
 
-	head = compound_head(pud_page(orig));
-	if (!page_cache_add_speculative(head, refs)) {
+	head = try_get_compound_head(pud_page(orig), refs);
+	if (!head) {
 		*nr -= refs;
 		return 0;
 	}
@@ -1229,8 +1250,8 @@  static int gup_huge_pgd(pgd_t orig, pgd_t *pgdp, unsigned long addr,
 		refs++;
 	} while (addr += PAGE_SIZE, addr != end);
 
-	head = compound_head(pgd_page(orig));
-	if (!page_cache_add_speculative(head, refs)) {
+	head = try_get_compound_head(pgd_page(orig), refs);
+	if (!head) {
 		*nr -= refs;
 		return 0;
 	}
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index fd932e7..3a1501e 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -3886,6 +3886,7 @@  long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma,
 	unsigned long vaddr = *position;
 	unsigned long remainder = *nr_pages;
 	struct hstate *h = hstate_vma(vma);
+	int err = -EFAULT;
 
 	while (vaddr < vma->vm_end && remainder) {
 		pte_t *pte;
@@ -3957,6 +3958,19 @@  long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma,
 
 		pfn_offset = (vaddr & ~huge_page_mask(h)) >> PAGE_SHIFT;
 		page = pte_page(huge_ptep_get(pte));
+
+		/*
+		 * Instead of doing 'try_get_page_foll()' below in the same_page
+		 * loop, just check the count once here.
+		 */
+		if (unlikely(page_count(page) <= 0)) {
+			if (pages) {
+				spin_unlock(ptl);
+				remainder = 0;
+				err = -ENOMEM;
+				break;
+			}
+		}
 same_page:
 		if (pages) {
 			pages[i] = mem_map_offset(page, pfn_offset);
@@ -3983,7 +3997,7 @@  same_page:
 	*nr_pages = remainder;
 	*position = vaddr;
 
-	return i ? i : -EFAULT;
+	return i ? i : err;
 }
 
 unsigned long hugetlb_change_protection(struct vm_area_struct *vma,