Re: [PATCH hotfix 6.12 3/8] mm: refactor map_deny_write_exec()

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Wed, Oct 23, 2024 at 10:30:46AM -0400, Liam R. Howlett wrote:
> * Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [241022 16:41]:
> > Refactor the map_deny_write_exec() to not unnecessarily require a VMA
> > parameter but rather to accept VMA flags parameters, which allows us to use
> > this function early in mmap_region() in a subsequent commit.
> >
> > While we're here, we refactor the function to be more readable and add some
> > additional documentation.
> >
> > Reported-by: Jann Horn <jannh@google.com>
> > Fixes: deb0f6562884 ("mm/mmap: undo ->mmap() when arch_validate_flags() fails")
> > Cc: stable <stable@kernel.org>
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>
> Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>

Thanks!

>
> > ---
> >  include/linux/mman.h | 21 ++++++++++++++++++---
> >  mm/mmap.c            |  2 +-
> >  mm/mprotect.c        |  2 +-
> >  3 files changed, 20 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/linux/mman.h b/include/linux/mman.h
> > index bcb201ab7a41..d71f83b821ef 100644
> > --- a/include/linux/mman.h
> > +++ b/include/linux/mman.h
> > @@ -188,16 +188,31 @@ static inline bool arch_memory_deny_write_exec_supported(void)
> >   *
> >   *	d)	mmap(PROT_READ | PROT_EXEC)
> >   *		mmap(PROT_READ | PROT_EXEC | PROT_BTI)
>
> nit: the arguments are not in the comments (which is a pre-existing
> issue).

I add them! ----
               |
>              |
> > + *        |
> > + * This is only applicable if the user has set the Memory-Deny-Write-Execute
> > + * (MDWE) protection mask for the current process.
> > + *   v----|---------------------------------------------------------v
> > + * @old specifies the VMA flags the VMA originally possessed, and @new the ones
> > + * we propose to set.
> > + *
> > + * Return: false if proposed change is OK, true if not ok and should be denied.
> >   */
> > -static inline bool map_deny_write_exec(struct vm_area_struct *vma,  unsigned long vm_flags)
> > +static inline bool map_deny_write_exec(unsigned long old, unsigned long new)
>
> Nit: somehow "new" is special in my editor.  Also, I'm too lazy to fix
> my editor so I'll live with it.

Time for a "new" editor! ;)

>
> >  {
> > +	/* If MDWE is disabled, we have nothing to deny. */
> >  	if (!test_bit(MMF_HAS_MDWE, &current->mm->flags))
> >  		return false;
> >
> > -	if ((vm_flags & VM_EXEC) && (vm_flags & VM_WRITE))
> > +	/* If the new VMA is not executable, we have nothing to deny. */
> > +	if (!(new & VM_EXEC))
> > +		return false;
> > +
> > +	/* Under MDWE we absolutely do not accept writably executable... */
> > +	if (new & VM_WRITE)
> >  		return true;
> >
> > -	if (!(vma->vm_flags & VM_EXEC) && (vm_flags & VM_EXEC))
> > +	/* ...nor newly executable VMAs. */
> > +	if (!(old & VM_EXEC))
> >  		return true;
> >
> >  	return false;
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index d55c58e99a54..66edf0ebba94 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -1508,7 +1508,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> >  		vma_set_anonymous(vma);
> >  	}
> >
> > -	if (map_deny_write_exec(vma, vma->vm_flags)) {
> > +	if (map_deny_write_exec(vma->vm_flags, vma->vm_flags)) {
> >  		error = -EACCES;
> >  		goto close_and_free_vma;
> >  	}
> > diff --git a/mm/mprotect.c b/mm/mprotect.c
> > index 0c5d6d06107d..6f450af3252e 100644
> > --- a/mm/mprotect.c
> > +++ b/mm/mprotect.c
> > @@ -810,7 +810,7 @@ static int do_mprotect_pkey(unsigned long start, size_t len,
> >  			break;
> >  		}
> >
> > -		if (map_deny_write_exec(vma, newflags)) {
> > +		if (map_deny_write_exec(vma->vm_flags, newflags)) {
> >  			error = -EACCES;
> >  			break;
> >  		}
> > --
> > 2.47.0