The check_mm_seal() function is doing something general - checking whether
a range contains only VMAs (or rather that it does NOT contain any unmapped
regions).
Generalise this and put the logic in mm/vma.c - introducing
range_contains_unmapped(). Additionally we can simplify the logic, we are
simply checking whether the last vma->vm_end has either a VMA starting
after it or ends before the end parameter.
No functional change intended.
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
mm/mseal.c | 38 +++-----------------------------------
mm/vma.c | 18 ++++++++++++++++++
mm/vma.h | 3 +++
3 files changed, 24 insertions(+), 35 deletions(-)
diff --git a/mm/mseal.c b/mm/mseal.c
index adbcc65e9660..8e4c605af700 100644
--- a/mm/mseal.c
+++ b/mm/mseal.c
@@ -37,34 +37,6 @@ static int mseal_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
return ret;
}
-/*
- * Check for do_mseal:
- * 1> start is part of a valid vma.
- * 2> end is part of a valid vma.
- * 3> No gap (unallocated address) between start and end.
- * 4> map is sealable.
- */
-static int check_mm_seal(unsigned long start, unsigned long end)
-{
- struct vm_area_struct *vma;
- unsigned long nstart = start;
- VMA_ITERATOR(vmi, current->mm, start);
-
- /* going through each vma to check. */
- for_each_vma_range(vmi, vma, end) {
- if (vma->vm_start > nstart)
- /* unallocated memory found. */
- return -ENOMEM;
-
- if (vma->vm_end >= end)
- return 0;
-
- nstart = vma->vm_end;
- }
-
- return -ENOMEM;
-}
-
/*
* Apply sealing.
*/
@@ -184,14 +156,10 @@ int do_mseal(unsigned long start, size_t len_in, unsigned long flags)
if (mmap_write_lock_killable(mm))
return -EINTR;
- /*
- * First pass, this helps to avoid
- * partial sealing in case of error in input address range,
- * e.g. ENOMEM error.
- */
- ret = check_mm_seal(start, end);
- if (ret)
+ if (range_contains_unmapped(mm, start, end)) {
+ ret = -ENOMEM;
goto out;
+ }
/*
* Second pass, this should success, unless there are errors
diff --git a/mm/vma.c b/mm/vma.c
index b3d880652359..b57545568ae6 100644
--- a/mm/vma.c
+++ b/mm/vma.c
@@ -3203,3 +3203,21 @@ int insert_vm_struct(struct mm_struct *mm, struct vm_area_struct *vma)
return 0;
}
+
+/* Does the [start, end) range contain any unmapped memory? */
+bool range_contains_unmapped(struct mm_struct *mm,
+ unsigned long start, unsigned long end)
+{
+ struct vm_area_struct *vma;
+ unsigned long prev_end = start;
+ VMA_ITERATOR(vmi, current->mm, start);
+
+ for_each_vma_range(vmi, vma, end) {
+ if (vma->vm_start > prev_end)
+ return true;
+
+ prev_end = vma->vm_end;
+ }
+
+ return prev_end < end;
+}
diff --git a/mm/vma.h b/mm/vma.h
index d17f560cf53d..bfe9a04e6018 100644
--- a/mm/vma.h
+++ b/mm/vma.h
@@ -598,4 +598,7 @@ int create_init_stack_vma(struct mm_struct *mm, struct vm_area_struct **vmap,
int relocate_vma_down(struct vm_area_struct *vma, unsigned long shift);
#endif
+bool range_contains_unmapped(struct mm_struct *mm,
+ unsigned long start, unsigned long end);
+
#endif /* __MM_VMA_H */
--
2.50.1
* Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [250714 09:08]: > The check_mm_seal() function is doing something general - checking whether > a range contains only VMAs (or rather that it does NOT contain any unmapped > regions). > > Generalise this and put the logic in mm/vma.c - introducing > range_contains_unmapped(). Additionally we can simplify the logic, we are > simply checking whether the last vma->vm_end has either a VMA starting > after it or ends before the end parameter. > > No functional change intended. > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> I do fear that people will find this function and try and use it internally, it may make our jobs of avoiding this being expanded more annoying. Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com> > --- > mm/mseal.c | 38 +++----------------------------------- > mm/vma.c | 18 ++++++++++++++++++ > mm/vma.h | 3 +++ > 3 files changed, 24 insertions(+), 35 deletions(-) > > diff --git a/mm/mseal.c b/mm/mseal.c > index adbcc65e9660..8e4c605af700 100644 > --- a/mm/mseal.c > +++ b/mm/mseal.c > @@ -37,34 +37,6 @@ static int mseal_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma, > return ret; > } > > -/* > - * Check for do_mseal: > - * 1> start is part of a valid vma. > - * 2> end is part of a valid vma. > - * 3> No gap (unallocated address) between start and end. > - * 4> map is sealable. > - */ > -static int check_mm_seal(unsigned long start, unsigned long end) > -{ > - struct vm_area_struct *vma; > - unsigned long nstart = start; > - VMA_ITERATOR(vmi, current->mm, start); > - > - /* going through each vma to check. */ > - for_each_vma_range(vmi, vma, end) { > - if (vma->vm_start > nstart) > - /* unallocated memory found. */ > - return -ENOMEM; > - > - if (vma->vm_end >= end) > - return 0; > - > - nstart = vma->vm_end; > - } > - > - return -ENOMEM; > -} > - > /* > * Apply sealing. > */ > @@ -184,14 +156,10 @@ int do_mseal(unsigned long start, size_t len_in, unsigned long flags) > if (mmap_write_lock_killable(mm)) > return -EINTR; > > - /* > - * First pass, this helps to avoid > - * partial sealing in case of error in input address range, > - * e.g. ENOMEM error. > - */ > - ret = check_mm_seal(start, end); > - if (ret) > + if (range_contains_unmapped(mm, start, end)) { > + ret = -ENOMEM; > goto out; > + } > > /* > * Second pass, this should success, unless there are errors > diff --git a/mm/vma.c b/mm/vma.c > index b3d880652359..b57545568ae6 100644 > --- a/mm/vma.c > +++ b/mm/vma.c > @@ -3203,3 +3203,21 @@ int insert_vm_struct(struct mm_struct *mm, struct vm_area_struct *vma) > > return 0; > } > + > +/* Does the [start, end) range contain any unmapped memory? */ > +bool range_contains_unmapped(struct mm_struct *mm, > + unsigned long start, unsigned long end) > +{ > + struct vm_area_struct *vma; > + unsigned long prev_end = start; > + VMA_ITERATOR(vmi, current->mm, start); > + > + for_each_vma_range(vmi, vma, end) { > + if (vma->vm_start > prev_end) > + return true; > + > + prev_end = vma->vm_end; > + } > + > + return prev_end < end; > +} > diff --git a/mm/vma.h b/mm/vma.h > index d17f560cf53d..bfe9a04e6018 100644 > --- a/mm/vma.h > +++ b/mm/vma.h > @@ -598,4 +598,7 @@ int create_init_stack_vma(struct mm_struct *mm, struct vm_area_struct **vmap, > int relocate_vma_down(struct vm_area_struct *vma, unsigned long shift); > #endif > > +bool range_contains_unmapped(struct mm_struct *mm, > + unsigned long start, unsigned long end); > + > #endif /* __MM_VMA_H */ > -- > 2.50.1
On Mon, Jul 14, 2025 at 11:35:44AM -0400, Liam R. Howlett wrote: > * Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [250714 09:08]: > > The check_mm_seal() function is doing something general - checking whether > > a range contains only VMAs (or rather that it does NOT contain any unmapped > > regions). > > > > Generalise this and put the logic in mm/vma.c - introducing > > range_contains_unmapped(). Additionally we can simplify the logic, we are > > simply checking whether the last vma->vm_end has either a VMA starting > > after it or ends before the end parameter. > > > > No functional change intended. > > > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > > I do fear that people will find this function and try and use it > internally, it may make our jobs of avoiding this being expanded more > annoying. Hmm, surely we should have some ability to dissuade within mm :) Thing is I don't love having this function in mm/mseal.c when it has nothing to do with mseal()'ing. If people want to be weird about gaps they can pretty trivially implement something like this anyway. Probably. Possibly. Maybe? > > Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com> Thanks! > > > --- > > mm/mseal.c | 38 +++----------------------------------- > > mm/vma.c | 18 ++++++++++++++++++ > > mm/vma.h | 3 +++ > > 3 files changed, 24 insertions(+), 35 deletions(-) > > > > diff --git a/mm/mseal.c b/mm/mseal.c > > index adbcc65e9660..8e4c605af700 100644 > > --- a/mm/mseal.c > > +++ b/mm/mseal.c > > @@ -37,34 +37,6 @@ static int mseal_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma, > > return ret; > > } > > > > -/* > > - * Check for do_mseal: > > - * 1> start is part of a valid vma. > > - * 2> end is part of a valid vma. > > - * 3> No gap (unallocated address) between start and end. > > - * 4> map is sealable. > > - */ > > -static int check_mm_seal(unsigned long start, unsigned long end) > > -{ > > - struct vm_area_struct *vma; > > - unsigned long nstart = start; > > - VMA_ITERATOR(vmi, current->mm, start); > > - > > - /* going through each vma to check. */ > > - for_each_vma_range(vmi, vma, end) { > > - if (vma->vm_start > nstart) > > - /* unallocated memory found. */ > > - return -ENOMEM; > > - > > - if (vma->vm_end >= end) > > - return 0; > > - > > - nstart = vma->vm_end; > > - } > > - > > - return -ENOMEM; > > -} > > - > > /* > > * Apply sealing. > > */ > > @@ -184,14 +156,10 @@ int do_mseal(unsigned long start, size_t len_in, unsigned long flags) > > if (mmap_write_lock_killable(mm)) > > return -EINTR; > > > > - /* > > - * First pass, this helps to avoid > > - * partial sealing in case of error in input address range, > > - * e.g. ENOMEM error. > > - */ > > - ret = check_mm_seal(start, end); > > - if (ret) > > + if (range_contains_unmapped(mm, start, end)) { > > + ret = -ENOMEM; > > goto out; > > + } > > > > /* > > * Second pass, this should success, unless there are errors > > diff --git a/mm/vma.c b/mm/vma.c > > index b3d880652359..b57545568ae6 100644 > > --- a/mm/vma.c > > +++ b/mm/vma.c > > @@ -3203,3 +3203,21 @@ int insert_vm_struct(struct mm_struct *mm, struct vm_area_struct *vma) > > > > return 0; > > } > > + > > +/* Does the [start, end) range contain any unmapped memory? */ > > +bool range_contains_unmapped(struct mm_struct *mm, > > + unsigned long start, unsigned long end) > > +{ > > + struct vm_area_struct *vma; > > + unsigned long prev_end = start; > > + VMA_ITERATOR(vmi, current->mm, start); > > + > > + for_each_vma_range(vmi, vma, end) { > > + if (vma->vm_start > prev_end) > > + return true; > > + > > + prev_end = vma->vm_end; > > + } > > + > > + return prev_end < end; > > +} > > diff --git a/mm/vma.h b/mm/vma.h > > index d17f560cf53d..bfe9a04e6018 100644 > > --- a/mm/vma.h > > +++ b/mm/vma.h > > @@ -598,4 +598,7 @@ int create_init_stack_vma(struct mm_struct *mm, struct vm_area_struct **vmap, > > int relocate_vma_down(struct vm_area_struct *vma, unsigned long shift); > > #endif > > > > +bool range_contains_unmapped(struct mm_struct *mm, > > + unsigned long start, unsigned long end); > > + > > #endif /* __MM_VMA_H */ > > -- > > 2.50.1
* Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [250714 11:40]: > On Mon, Jul 14, 2025 at 11:35:44AM -0400, Liam R. Howlett wrote: > > * Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [250714 09:08]: > > > The check_mm_seal() function is doing something general - checking whether > > > a range contains only VMAs (or rather that it does NOT contain any unmapped > > > regions). > > > > > > Generalise this and put the logic in mm/vma.c - introducing > > > range_contains_unmapped(). Additionally we can simplify the logic, we are > > > simply checking whether the last vma->vm_end has either a VMA starting > > > after it or ends before the end parameter. > > > > > > No functional change intended. > > > > > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > > > > I do fear that people will find this function and try and use it > > internally, it may make our jobs of avoiding this being expanded more > > annoying. > > Hmm, surely we should have some ability to dissuade within mm :) > > Thing is I don't love having this function in mm/mseal.c when it has > nothing to do with mseal()'ing. > > If people want to be weird about gaps they can pretty trivially implement > something like this anyway. Probably. Possibly. Maybe? Yes, but the existence of a function legitimizes their thought prior to sending it for review. That is, seeing a function that already does it makes okay to include the option in the planning. > > > > > Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com> > > Thanks! >
On Mon, Jul 14, 2025 at 11:43:57AM -0400, Liam R. Howlett wrote: > Yes, but the existence of a function legitimizes their thought prior to > sending it for review. That is, seeing a function that already does it > makes okay to include the option in the planning. True. OK, based on what you're saying and what Pedro's saying, next respin/fixpatch I"ll put this in mseal.c as a static function, even if it makes me cringe a bit to do it. And then Pedro can remove it in his series :) For the record I don't agree with this check being performed... it seems silly to me.
On Mon, Jul 14, 2025 at 02:00:39PM +0100, Lorenzo Stoakes wrote: > The check_mm_seal() function is doing something general - checking whether > a range contains only VMAs (or rather that it does NOT contain any unmapped > regions). > > Generalise this and put the logic in mm/vma.c - introducing > range_contains_unmapped(). Additionally we can simplify the logic, we are > simply checking whether the last vma->vm_end has either a VMA starting > after it or ends before the end parameter. > I don't like this. Unless you have any other user for this in mind, we'll proliferate this awful behavior (and add this into core vma code). I have some patches locally to fully remove this upfront check, and AFAIK we're somewhat in agreement that we can simply nuke this check (for various reasons, including that we *still* don't have a man page for the syscall). I can send them for proper discussion after your series lands. -- Pedro
On Mon, Jul 14, 2025 at 04:17:23PM +0100, Pedro Falcato wrote: > On Mon, Jul 14, 2025 at 02:00:39PM +0100, Lorenzo Stoakes wrote: > > The check_mm_seal() function is doing something general - checking whether > > a range contains only VMAs (or rather that it does NOT contain any unmapped > > regions). > > > > Generalise this and put the logic in mm/vma.c - introducing > > range_contains_unmapped(). Additionally we can simplify the logic, we are > > simply checking whether the last vma->vm_end has either a VMA starting > > after it or ends before the end parameter. > > > > I don't like this. Unless you have any other user for this in mind, > we'll proliferate this awful behavior (and add this into core vma code). I'm not sure how putting it in an internal-only mm file perpetuates anything. I'm naming the function by what it does, and putting it where it belongs in the VMA logic, and additionally making the function less horrible. Let's not please get stuck on the isues with mseal implementation which will catch-22 us into not being able to refactor. We can do the refactoring first and it's fine to just yank this if it's not used. I'm not having a function like this sat in mm/mseal.c when it has absolutely nothing to do with mseal specifically though. > > I have some patches locally to fully remove this upfront check, and AFAIK > we're somewhat in agreement that we can simply nuke this check (for > various reasons, including that we *still* don't have a man page for the > syscall). I can send them for proper discussion after your series lands. Yes I agree this check is odd, I don't really see why on earth we're concerned with whether there are gaps or not, you'd surely want to just seal whatever VMAs exist in the range? But this belongs as something separate (a _functional_ change), let's get the code sane first. And ack on manpage, that's a horrible oversight that needs addressing! > > -- > Pedro
On 14.07.25 17:23, Lorenzo Stoakes wrote: > On Mon, Jul 14, 2025 at 04:17:23PM +0100, Pedro Falcato wrote: >> On Mon, Jul 14, 2025 at 02:00:39PM +0100, Lorenzo Stoakes wrote: >>> The check_mm_seal() function is doing something general - checking whether >>> a range contains only VMAs (or rather that it does NOT contain any unmapped >>> regions). >>> >>> Generalise this and put the logic in mm/vma.c - introducing >>> range_contains_unmapped(). Additionally we can simplify the logic, we are >>> simply checking whether the last vma->vm_end has either a VMA starting >>> after it or ends before the end parameter. >>> >> >> I don't like this. Unless you have any other user for this in mind, >> we'll proliferate this awful behavior (and add this into core vma code). > > I'm not sure how putting it in an internal-only mm file perpetuates > anything. > > I'm naming the function by what it does, and putting it where it belongs in > the VMA logic, and additionally making the function less horrible. > > Let's not please get stuck on the isues with mseal implementation which > will catch-22 us into not being able to refactor. > > We can do the refactoring first and it's fine to just yank this if it's not > used. > > I'm not having a function like this sat in mm/mseal.c when it has > absolutely nothing to do with mseal specifically though. > >> >> I have some patches locally to fully remove this upfront check, and AFAIK >> we're somewhat in agreement that we can simply nuke this check (for >> various reasons, including that we *still* don't have a man page for the >> syscall). I can send them for proper discussion after your series lands. > > Yes I agree this check is odd, I don't really see why on earth we're > concerned with whether there are gaps or not, you'd surely want to just > seal whatever VMAs exist in the range? Probably because GAPs cannot be sealed. So user space could assume that in fact, nothing in that area can change after a successful mseal, while it can ... Not sure, though ... -- Cheers, David / dhildenb
On Mon, Jul 14, 2025 at 05:25:59PM +0200, David Hildenbrand wrote: > On 14.07.25 17:23, Lorenzo Stoakes wrote: > > On Mon, Jul 14, 2025 at 04:17:23PM +0100, Pedro Falcato wrote: > > > On Mon, Jul 14, 2025 at 02:00:39PM +0100, Lorenzo Stoakes wrote: > > > > The check_mm_seal() function is doing something general - checking whether > > > > a range contains only VMAs (or rather that it does NOT contain any unmapped > > > > regions). > > > > > > > > Generalise this and put the logic in mm/vma.c - introducing > > > > range_contains_unmapped(). Additionally we can simplify the logic, we are > > > > simply checking whether the last vma->vm_end has either a VMA starting > > > > after it or ends before the end parameter. > > > > > > > > > > I don't like this. Unless you have any other user for this in mind, > > > we'll proliferate this awful behavior (and add this into core vma code). > > > > I'm not sure how putting it in an internal-only mm file perpetuates > > anything. > > > > I'm naming the function by what it does, and putting it where it belongs in > > the VMA logic, and additionally making the function less horrible. > > > > Let's not please get stuck on the isues with mseal implementation which > > will catch-22 us into not being able to refactor. > > > > We can do the refactoring first and it's fine to just yank this if it's not > > used. > > > > I'm not having a function like this sat in mm/mseal.c when it has > > absolutely nothing to do with mseal specifically though. > > > > > > > > I have some patches locally to fully remove this upfront check, and AFAIK > > > we're somewhat in agreement that we can simply nuke this check (for > > > various reasons, including that we *still* don't have a man page for the > > > syscall). I can send them for proper discussion after your series lands. > > > > Yes I agree this check is odd, I don't really see why on earth we're > > concerned with whether there are gaps or not, you'd surely want to just > > seal whatever VMAs exist in the range? > > Probably because GAPs cannot be sealed. So user space could assume that in > fact, nothing in that area can change after a successful mseal, while it can > ... > > Not sure, though ... Yeah maybe a sekuriteh thing where you want to be sure the range is in fact _all_ sealed. I'm not sure that really makes much sense in practice honestly though, because if another thread can fiddle with things, then surely you've already 'lost'. if you expected to touch a VMA where in fact aa gap exists your software is _already_ broken because you're going to segfault. So it just seems overly theoretical to me. I think we should error out if there's _no_ VMAs at all, but otherwise succeed. The semantics of 'all VMAs which exist in the range are sealed' would still be maintained. > > -- > Cheers, > > David / dhildenb >
* Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [250714 11:32]: > On Mon, Jul 14, 2025 at 05:25:59PM +0200, David Hildenbrand wrote: > > On 14.07.25 17:23, Lorenzo Stoakes wrote: > > > On Mon, Jul 14, 2025 at 04:17:23PM +0100, Pedro Falcato wrote: > > > > On Mon, Jul 14, 2025 at 02:00:39PM +0100, Lorenzo Stoakes wrote: > > > > > The check_mm_seal() function is doing something general - checking whether > > > > > a range contains only VMAs (or rather that it does NOT contain any unmapped > > > > > regions). > > > > > > > > > > Generalise this and put the logic in mm/vma.c - introducing > > > > > range_contains_unmapped(). Additionally we can simplify the logic, we are > > > > > simply checking whether the last vma->vm_end has either a VMA starting > > > > > after it or ends before the end parameter. > > > > > > > > > > > > > I don't like this. Unless you have any other user for this in mind, > > > > we'll proliferate this awful behavior (and add this into core vma code). > > > > > > I'm not sure how putting it in an internal-only mm file perpetuates > > > anything. > > > > > > I'm naming the function by what it does, and putting it where it belongs in > > > the VMA logic, and additionally making the function less horrible. > > > > > > Let's not please get stuck on the isues with mseal implementation which > > > will catch-22 us into not being able to refactor. > > > > > > We can do the refactoring first and it's fine to just yank this if it's not > > > used. > > > > > > I'm not having a function like this sat in mm/mseal.c when it has > > > absolutely nothing to do with mseal specifically though. > > > > > > > > > > > I have some patches locally to fully remove this upfront check, and AFAIK > > > > we're somewhat in agreement that we can simply nuke this check (for > > > > various reasons, including that we *still* don't have a man page for the > > > > syscall). I can send them for proper discussion after your series lands. > > > > > > Yes I agree this check is odd, I don't really see why on earth we're > > > concerned with whether there are gaps or not, you'd surely want to just > > > seal whatever VMAs exist in the range? > > > > Probably because GAPs cannot be sealed. So user space could assume that in > > fact, nothing in that area can change after a successful mseal, while it can > > ... > > > > Not sure, though ... > > Yeah maybe a sekuriteh thing where you want to be sure the range is in fact > _all_ sealed. > > I'm not sure that really makes much sense in practice honestly though, because > if another thread can fiddle with things, then surely you've already 'lost'. > > if you expected to touch a VMA where in fact aa gap exists your software is > _already_ broken because you're going to segfault. Since this is applying a seal to a range that already exists, we are in a race condition anyways. If a gap exists it might be better to fail and exit as it is insecure, but really, if someone has created a gap then they could have mapped whatever they want.. > > So it just seems overly theoretical to me. I don't see a theoretical means to justify doing this, so I must be missing something. > > I think we should error out if there's _no_ VMAs at all, but otherwise succeed. > > The semantics of 'all VMAs which exist in the range are sealed' would still be > maintained. > > > > > -- > > Cheers, > > > > David / dhildenb > >
On 14.07.25 15:00, Lorenzo Stoakes wrote: > The check_mm_seal() function is doing something general - checking whether > a range contains only VMAs (or rather that it does NOT contain any unmapped > regions). > > Generalise this and put the logic in mm/vma.c - introducing > range_contains_unmapped(). Additionally we can simplify the logic, we are > simply checking whether the last vma->vm_end has either a VMA starting > after it or ends before the end parameter. > > No functional change intended. > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > --- Acked-by: David Hildenbrand <david@redhat.com> -- Cheers, David / dhildenb
© 2016 - 2025 Red Hat, Inc.