Re: [PATCH 4/5] mm/mseal: separate out and simplify VMA gap check

[Liam R. Howlett]

* Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx> [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
> >