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>
---
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)
+ *
+ * This is only applicable if the user has set the Memory-Deny-Write-Execute
+ * (MDWE) protection mask for the current process.
+ *
+ * @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)
{
+ /* If MDWE is disabled, we have nothing to deny. */
if (!test_bit(MMF_HAS_MDWE, ¤t->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* 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>
> ---
> 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).
> + *
> + * This is only applicable if the user has set the Memory-Deny-Write-Execute
> + * (MDWE) protection mask for the current process.
> + *
> + * @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.
> {
> + /* If MDWE is disabled, we have nothing to deny. */
> if (!test_bit(MMF_HAS_MDWE, ¤t->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
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, ¤t->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
On Wed, 23 Oct 2024 at 07:30, Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
>
> Nit: somehow "new" is special in my editor. Also, I'm too lazy to fix
> my editor so I'll live with it.
I think your editor might be in some "edit C++" mode, where "new" is a keyword.
So it might be as simple as telling your editor that it's just C.
Or your editor just doesn't know or care about the difference.
Linus
On 10/22/24 22:40, Lorenzo Stoakes wrote:
> 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: Vlastimil Babka <vbabka@suse.cz>
On Tue, Oct 22, 2024 at 10:41 PM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
> 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: Jann Horn <jannh@google.com>
[..]
> -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)
> {
> + /* If MDWE is disabled, we have nothing to deny. */
> if (!test_bit(MMF_HAS_MDWE, ¤t->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. */
nit: maybe clarify this as "nor existing VMAs newly becoming
executable" or something like that
> + if (!(old & VM_EXEC))
> return true;
>
> return false;
On Tue, Oct 22, 2024 at 11:15:18PM +0200, Jann Horn wrote:
> On Tue, Oct 22, 2024 at 10:41 PM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> > 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: Jann Horn <jannh@google.com>
Thanks!
>
> [..]
> > -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)
> > {
> > + /* If MDWE is disabled, we have nothing to deny. */
> > if (!test_bit(MMF_HAS_MDWE, ¤t->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. */
>
> nit: maybe clarify this as "nor existing VMAs newly becoming
> executable" or something like that
Ack
>
>
> > + if (!(old & VM_EXEC))
> > return true;
> >
> > return false;
© 2016 - 2024 Red Hat, Inc.