linux-hardening - Re: [PATCH v3 5/5] mm/mseal: rework mseal apply logic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [day] [month] [year] [list]
Message-ID: <CABi2SkXQsao-S+uy63k6Zc=y4W-pik6XZSzw+05f_Ws_frqZmQ@mail.gmail.com>
Date: Thu, 24 Jul 2025 11:41:30 -0700
From: Jeff Xu <jeffxu@...omium.org>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, "Liam R . Howlett" <Liam.Howlett@...cle.com>, 
	David Hildenbrand <david@...hat.com>, Vlastimil Babka <vbabka@...e.cz>, Jann Horn <jannh@...gle.com>, 
	Pedro Falcato <pfalcato@...e.de>, linux-mm@...ck.org, linux-kernel@...r.kernel.org, 
	Kees Cook <kees@...nel.org>, linux-hardening@...r.kernel.org
Subject: Re: [PATCH v3 5/5] mm/mseal: rework mseal apply logic

Hi Lorenzo,

On Wed, Jul 16, 2025 at 10:38 AM Lorenzo Stoakes
<lorenzo.stoakes@...cle.com> wrote:
>
> The logic can be simplified - firstly by renaming the inconsistently named
> apply_mm_seal() to mseal_apply().
>
> We then wrap mseal_fixup() into the main loop as the logic is simple enough
> to not require it, equally it isn't a hugely pleasant pattern in mprotect()
> etc.  so it's not something we want to perpetuate.
>
> We eliminate the need for invoking vma_iter_end() on each loop by directly
> determining if the VMA was merged - the only thing we need concern
> ourselves with is whether the start/end of the (gapless) range are offset
> into VMAs.
>
> This refactoring also avoids the rather horrid 'pass pointer to prev
> around' pattern used in mprotect() et al.
>
> No functional change intended.
>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> Reviewed-by: Pedro Falcato <pfalcato@...e.de>
> Reviewed-by: Liam R. Howlett <Liam.Howlett@...cle.com>
> Acked-by: David Hildenbrand <david@...hat.com>
Acked-by: Jeff Xu <jeffxu@...omium.org>

Thanks and regards,
-Jeff
> ---
>  mm/mseal.c | 67 ++++++++++++++++--------------------------------------
>  1 file changed, 20 insertions(+), 47 deletions(-)
>
> diff --git a/mm/mseal.c b/mm/mseal.c
> index 61c07b1369cb..0ab12e09792a 100644
> --- a/mm/mseal.c
> +++ b/mm/mseal.c
> @@ -15,28 +15,6 @@
>  #include <linux/sched.h>
>  #include "internal.h"
>
> -static int mseal_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
> -               struct vm_area_struct **prev, unsigned long start,
> -               unsigned long end, vm_flags_t newflags)
> -{
> -       int ret = 0;
> -       vm_flags_t oldflags = vma->vm_flags;
> -
> -       if (newflags == oldflags)
> -               goto out;
> -
> -       vma = vma_modify_flags(vmi, *prev, vma, start, end, newflags);
> -       if (IS_ERR(vma)) {
> -               ret = PTR_ERR(vma);
> -               goto out;
> -       }
> -
> -       vm_flags_set(vma, VM_SEALED);
> -out:
> -       *prev = vma;
> -       return ret;
> -}
> -
>  /* Does the [start, end) range contain any unmapped memory? */
>  static bool range_contains_unmapped(struct mm_struct *mm,
>                 unsigned long start, unsigned long end)
> @@ -55,38 +33,33 @@ static bool range_contains_unmapped(struct mm_struct *mm,
>         return prev_end < end;
>  }
>
> -/*
> - * Apply sealing.
> - */
> -static int apply_mm_seal(unsigned long start, unsigned long end)
> +static int mseal_apply(struct mm_struct *mm,
> +               unsigned long start, unsigned long end)
>  {
> -       unsigned long nstart;
>         struct vm_area_struct *vma, *prev;
> -       VMA_ITERATOR(vmi, current->mm, start);
> +       unsigned long curr_start = start;
> +       VMA_ITERATOR(vmi, mm, start);
>
> +       /* We know there are no gaps so this will be non-NULL. */
>         vma = vma_iter_load(&vmi);
> -       /*
> -        * Note: check_mm_seal should already checked ENOMEM case.
> -        * so vma should not be null, same for the other ENOMEM cases.
> -        */
>         prev = vma_prev(&vmi);
>         if (start > vma->vm_start)
>                 prev = vma;
>
> -       nstart = start;
>         for_each_vma_range(vmi, vma, end) {
> -               int error;
> -               unsigned long tmp;
> -               vm_flags_t newflags;
> -
> -               newflags = vma->vm_flags | VM_SEALED;
> -               tmp = vma->vm_end;
> -               if (tmp > end)
> -                       tmp = end;
> -               error = mseal_fixup(&vmi, vma, &prev, nstart, tmp, newflags);
> -               if (error)
> -                       return error;
> -               nstart = vma_iter_end(&vmi);
> +               unsigned long curr_end = MIN(vma->vm_end, end);
> +
> +               if (!(vma->vm_flags & VM_SEALED)) {
> +                       vma = vma_modify_flags(&vmi, prev, vma,
> +                                       curr_start, curr_end,
> +                                       vma->vm_flags | VM_SEALED);
> +                       if (IS_ERR(vma))
> +                               return PTR_ERR(vma);
> +                       vm_flags_set(vma, VM_SEALED);
> +               }
> +
> +               prev = vma;
> +               curr_start = curr_end;
>         }
>
>         return 0;
> @@ -185,10 +158,10 @@ int do_mseal(unsigned long start, size_t len_in, unsigned long flags)
>          * reaching the max supported VMAs, however, those cases shall
>          * be rare.
>          */
> -       ret = apply_mm_seal(start, end);
> +       ret = mseal_apply(mm, start, end);
>
>  out:
> -       mmap_write_unlock(current->mm);
> +       mmap_write_unlock(mm);
>         return ret;
>  }
>
> --
> 2.50.1
>