| Message ID | e67b7f6c682bddbea2fe8b2d87b8441e4d2ea6e6.1729628198.git.lorenzo.stoakes@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fix error handling in mmap_region() and refactor | expand |
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 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>
* 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, 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 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;
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
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; }
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(-) -- 2.47.0