[v3] Avoid MAP_FIXED gap exposure

[PATCH v3 14/16] mm/mmap: Use PHYS_PFN in mmap_region()

Posted by Liam R. Howlett 1 year, 7 months ago

From: "Liam R. Howlett" <Liam.Howlett@Oracle.com>

Instead of shifting the length by PAGE_SIZE, use PHYS_PFN.  Also use the
existing local variable everywhere instead of some of the time.

Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com>
---
 mm/mmap.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index 0c334eeae8cd..b14da6bd257f 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2935,7 +2935,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
 	struct mm_struct *mm = current->mm;
 	struct vm_area_struct *vma = NULL;
 	struct vm_area_struct *next, *prev, *merge;
-	pgoff_t pglen = len >> PAGE_SHIFT;
+	pgoff_t pglen = PHYS_PFN(len);
 	unsigned long charged = 0;
 	struct vma_munmap_struct vms;
 	struct ma_state mas_detach;
@@ -2955,7 +2955,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
 	 * MAP_FIXED may remove pages of mappings that intersects with requested
 	 * mapping. Account for the pages it would unmap.
 	 */
-	if (!may_expand_vm(mm, vm_flags, (len >> PAGE_SHIFT) - nr_pages))
+	if (!may_expand_vm(mm, vm_flags, pglen - nr_pages))
 		return -ENOMEM;
 
 	if (unlikely(!can_modify_mm(mm, addr, end)))
@@ -2990,7 +2990,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
 	 * Private writable mapping: check memory availability
 	 */
 	if (accountable_mapping(file, vm_flags)) {
-		charged = len >> PAGE_SHIFT;
+		charged = pglen;
 		charged -= nr_accounted;
 		if (security_vm_enough_memory_mm(mm, charged))
 			goto abort_munmap;
@@ -3149,14 +3149,14 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
 	if (vms.nr_pages)
 		vms_complete_munmap_vmas(&vms, &mas_detach);
 
-	vm_stat_account(mm, vm_flags, len >> PAGE_SHIFT);
+	vm_stat_account(mm, vm_flags, pglen);
 	if (vm_flags & VM_LOCKED) {
 		if ((vm_flags & VM_SPECIAL) || vma_is_dax(vma) ||
 					is_vm_hugetlb_page(vma) ||
 					vma == get_gate_vma(current->mm))
 			vm_flags_clear(vma, VM_LOCKED_MASK);
 		else
-			mm->locked_vm += (len >> PAGE_SHIFT);
+			mm->locked_vm += pglen;
 	}
 
 	if (file)
-- 
2.43.0

Re: [PATCH v3 14/16] mm/mmap: Use PHYS_PFN in mmap_region()

Posted by Lorenzo Stoakes 1 year, 7 months ago

On Thu, Jul 04, 2024 at 02:27:16PM GMT, Liam R. Howlett wrote:
> From: "Liam R. Howlett" <Liam.Howlett@Oracle.com>
>
> Instead of shifting the length by PAGE_SIZE, use PHYS_PFN.  Also use the
> existing local variable everywhere instead of some of the time.
>
> Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com>
> ---
>  mm/mmap.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 0c334eeae8cd..b14da6bd257f 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -2935,7 +2935,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
>  	struct mm_struct *mm = current->mm;
>  	struct vm_area_struct *vma = NULL;
>  	struct vm_area_struct *next, *prev, *merge;
> -	pgoff_t pglen = len >> PAGE_SHIFT;
> +	pgoff_t pglen = PHYS_PFN(len);
>  	unsigned long charged = 0;
>  	struct vma_munmap_struct vms;
>  	struct ma_state mas_detach;
> @@ -2955,7 +2955,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
>  	 * MAP_FIXED may remove pages of mappings that intersects with requested
>  	 * mapping. Account for the pages it would unmap.
>  	 */
> -	if (!may_expand_vm(mm, vm_flags, (len >> PAGE_SHIFT) - nr_pages))
> +	if (!may_expand_vm(mm, vm_flags, pglen - nr_pages))
>  		return -ENOMEM;
>
>  	if (unlikely(!can_modify_mm(mm, addr, end)))
> @@ -2990,7 +2990,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
>  	 * Private writable mapping: check memory availability
>  	 */
>  	if (accountable_mapping(file, vm_flags)) {
> -		charged = len >> PAGE_SHIFT;
> +		charged = pglen;
>  		charged -= nr_accounted;
>  		if (security_vm_enough_memory_mm(mm, charged))
>  			goto abort_munmap;
> @@ -3149,14 +3149,14 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
>  	if (vms.nr_pages)
>  		vms_complete_munmap_vmas(&vms, &mas_detach);
>
> -	vm_stat_account(mm, vm_flags, len >> PAGE_SHIFT);
> +	vm_stat_account(mm, vm_flags, pglen);
>  	if (vm_flags & VM_LOCKED) {
>  		if ((vm_flags & VM_SPECIAL) || vma_is_dax(vma) ||
>  					is_vm_hugetlb_page(vma) ||
>  					vma == get_gate_vma(current->mm))
>  			vm_flags_clear(vma, VM_LOCKED_MASK);
>  		else
> -			mm->locked_vm += (len >> PAGE_SHIFT);
> +			mm->locked_vm += pglen;
>  	}
>
>  	if (file)
> --
> 2.43.0
>

Maybe I should literally look ahead before making comments :)) thanks for
reading my mind and doing what I asked though! ;)

However I don't think you've fixed the duplication of PHYS_PFN(vm_end -
vm_start) in count_vma_pages_range() - still worth doing I think.

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

Re: [PATCH v3 14/16] mm/mmap: Use PHYS_PFN in mmap_region()

Posted by Suren Baghdasaryan 1 year, 7 months ago

On Mon, Jul 8, 2024 at 5:21 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Thu, Jul 04, 2024 at 02:27:16PM GMT, Liam R. Howlett wrote:
> > From: "Liam R. Howlett" <Liam.Howlett@Oracle.com>
> >
> > Instead of shifting the length by PAGE_SIZE, use PHYS_PFN.  Also use the
> > existing local variable everywhere instead of some of the time.
> >
> > Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com>
> > ---
> >  mm/mmap.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 0c334eeae8cd..b14da6bd257f 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -2935,7 +2935,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> >       struct mm_struct *mm = current->mm;
> >       struct vm_area_struct *vma = NULL;
> >       struct vm_area_struct *next, *prev, *merge;
> > -     pgoff_t pglen = len >> PAGE_SHIFT;
> > +     pgoff_t pglen = PHYS_PFN(len);
> >       unsigned long charged = 0;
> >       struct vma_munmap_struct vms;
> >       struct ma_state mas_detach;
> > @@ -2955,7 +2955,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> >        * MAP_FIXED may remove pages of mappings that intersects with requested
> >        * mapping. Account for the pages it would unmap.
> >        */
> > -     if (!may_expand_vm(mm, vm_flags, (len >> PAGE_SHIFT) - nr_pages))
> > +     if (!may_expand_vm(mm, vm_flags, pglen - nr_pages))
> >               return -ENOMEM;
> >
> >       if (unlikely(!can_modify_mm(mm, addr, end)))
> > @@ -2990,7 +2990,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> >        * Private writable mapping: check memory availability
> >        */
> >       if (accountable_mapping(file, vm_flags)) {
> > -             charged = len >> PAGE_SHIFT;
> > +             charged = pglen;
> >               charged -= nr_accounted;
> >               if (security_vm_enough_memory_mm(mm, charged))
> >                       goto abort_munmap;
> > @@ -3149,14 +3149,14 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> >       if (vms.nr_pages)
> >               vms_complete_munmap_vmas(&vms, &mas_detach);
> >
> > -     vm_stat_account(mm, vm_flags, len >> PAGE_SHIFT);
> > +     vm_stat_account(mm, vm_flags, pglen);
> >       if (vm_flags & VM_LOCKED) {
> >               if ((vm_flags & VM_SPECIAL) || vma_is_dax(vma) ||
> >                                       is_vm_hugetlb_page(vma) ||
> >                                       vma == get_gate_vma(current->mm))
> >                       vm_flags_clear(vma, VM_LOCKED_MASK);
> >               else
> > -                     mm->locked_vm += (len >> PAGE_SHIFT);
> > +                     mm->locked_vm += pglen;
> >       }
> >
> >       if (file)
> > --
> > 2.43.0
> >
>
> Maybe I should literally look ahead before making comments :)) thanks for
> reading my mind and doing what I asked though! ;)
>
> However I don't think you've fixed the duplication of PHYS_PFN(vm_end -
> vm_start) in count_vma_pages_range() - still worth doing I think.
>
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

Reviewed-by: Suren Baghdasaryan <surenb@google.com>

Re: [PATCH v3 14/16] mm/mmap: Use PHYS_PFN in mmap_region()

Posted by Liam R. Howlett 1 year, 7 months ago

* Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [240708 08:21]:
> On Thu, Jul 04, 2024 at 02:27:16PM GMT, Liam R. Howlett wrote:
> > From: "Liam R. Howlett" <Liam.Howlett@Oracle.com>
> >
> > Instead of shifting the length by PAGE_SIZE, use PHYS_PFN.  Also use the
> > existing local variable everywhere instead of some of the time.
> >
> > Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com>
> > ---
> >  mm/mmap.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 0c334eeae8cd..b14da6bd257f 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -2935,7 +2935,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> >  	struct mm_struct *mm = current->mm;
> >  	struct vm_area_struct *vma = NULL;
> >  	struct vm_area_struct *next, *prev, *merge;
> > -	pgoff_t pglen = len >> PAGE_SHIFT;
> > +	pgoff_t pglen = PHYS_PFN(len);
> >  	unsigned long charged = 0;
> >  	struct vma_munmap_struct vms;
> >  	struct ma_state mas_detach;
> > @@ -2955,7 +2955,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> >  	 * MAP_FIXED may remove pages of mappings that intersects with requested
> >  	 * mapping. Account for the pages it would unmap.
> >  	 */
> > -	if (!may_expand_vm(mm, vm_flags, (len >> PAGE_SHIFT) - nr_pages))
> > +	if (!may_expand_vm(mm, vm_flags, pglen - nr_pages))
> >  		return -ENOMEM;
> >
> >  	if (unlikely(!can_modify_mm(mm, addr, end)))
> > @@ -2990,7 +2990,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> >  	 * Private writable mapping: check memory availability
> >  	 */
> >  	if (accountable_mapping(file, vm_flags)) {
> > -		charged = len >> PAGE_SHIFT;
> > +		charged = pglen;
> >  		charged -= nr_accounted;
> >  		if (security_vm_enough_memory_mm(mm, charged))
> >  			goto abort_munmap;
> > @@ -3149,14 +3149,14 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> >  	if (vms.nr_pages)
> >  		vms_complete_munmap_vmas(&vms, &mas_detach);
> >
> > -	vm_stat_account(mm, vm_flags, len >> PAGE_SHIFT);
> > +	vm_stat_account(mm, vm_flags, pglen);
> >  	if (vm_flags & VM_LOCKED) {
> >  		if ((vm_flags & VM_SPECIAL) || vma_is_dax(vma) ||
> >  					is_vm_hugetlb_page(vma) ||
> >  					vma == get_gate_vma(current->mm))
> >  			vm_flags_clear(vma, VM_LOCKED_MASK);
> >  		else
> > -			mm->locked_vm += (len >> PAGE_SHIFT);
> > +			mm->locked_vm += pglen;
> >  	}
> >
> >  	if (file)
> > --
> > 2.43.0
> >
> 
> Maybe I should literally look ahead before making comments :)) thanks for
> reading my mind and doing what I asked though! ;)
> 
> However I don't think you've fixed the duplication of PHYS_PFN(vm_end -
> vm_start) in count_vma_pages_range() - still worth doing I think.

I drop that function in the last patch so probably not worth doing.
This is just a few patches before the axe drops.

> 
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

Re: [PATCH v3 14/16] mm/mmap: Use PHYS_PFN in mmap_region()

Posted by Lorenzo Stoakes 1 year, 7 months ago

On Tue, Jul 09, 2024 at 02:35:16PM GMT, Liam R. Howlett wrote:
[snip]

> >
> > Maybe I should literally look ahead before making comments :)) thanks for
> > reading my mind and doing what I asked though! ;)
> >
> > However I don't think you've fixed the duplication of PHYS_PFN(vm_end -
> > vm_start) in count_vma_pages_range() - still worth doing I think.
>
> I drop that function in the last patch so probably not worth doing.
> This is just a few patches before the axe drops.
>

Actually that's a fair point - I think its fine to do without this nit with
that context!

This is the peril of reviewing forwards through the series and being
surprised later when things are addressed in subsequent patches (or become,
ultimately, irrelevant!).

> >
> > Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>