Re: [PATCH v7 0/4] Introduce mseal()

[Liam R. Howlett]

* Theo de Raadt <deraadt@xxxxxxxxxxx> [240122 17:35]:
> Jeff Xu <jeffxu@xxxxxxxxxxxx> wrote:
> 
> > On Mon, Jan 22, 2024 at 7:49 AM Theo de Raadt <deraadt@xxxxxxxxxxx> wrote:
> > >
> > > Regarding these pieces
> > >
> > > > The PROT_SEAL bit in prot field of mmap(). When present, it marks
> > > > the map sealed since creation.
> > >
> > > OpenBSD won't be doing this.  I had PROT_IMMUTABLE as a draft.  In my
> > > research I found basically zero circumstances when you userland does
> > > that.  The most common circumstance is you create a RW mapping, fill it,
> > > and then change to a more restrictve mapping, and lock it.
> > >
> > > There are a few regions in the addressspace that can be locked while RW.
> > > For instance, the stack.  But the kernel does that, not userland.  I
> > > found regions where the kernel wants to do this to the address space,
> > > but there is no need to export useless functionality to userland.
> > >
> > I have a feeling that most apps that need to use mmap() in their code
> > are likely using RW mappings. Adding sealing to mmap() could stop
> > those mappings from being executable. Of course, those apps would
> > need to change their code. We can't do it for them.
> 
> I don't have a feeling about it.
> 
> I spent a year engineering a complete system which exercises the maximum
> amount of memory you can lock.
> 
> I saw nothing like what you are describing.  I had PROT_IMMUTABLE in my
> drafts, and saw it turning into a dangerous anti-pattern.
> 
> > Also, I believe adding this to mmap() has no downsides, only
> > performance gain, as Pedro Falcato pointed out in [1].
> > 
> > [1] https://lore.kernel.org/lkml/CAKbZUD2A+=bp_sd+Q0Yif7NJqMu8p__eb4yguq0agEcmLH8SDQ@xxxxxxxxxxxxxx/
> 
> Are you joking?  You don't have any code doing that today.  More feelings?

The 'no downside" is to combining two calls together; mmap() & mseal(),
at least that is how I read the linked discussion.

The common case (since there are no users today) of just calling
mmap()/munmap() will have the downside.

There will be a performance impact once you have can_modify_mm() doing
more than just returning true.  Certainly, the impact will be larger
in munmap where multiple VMAs may need to be checked (assuming that's
the plan?).

This will require a new and earlier walk of the vma tree while holding
the mmap_lock.  Since you are checking (potentially multiple) VMAs for
something, I don't think there is a way around holding the lock.

I'm not saying the cost will be large, but it will be a positive
non-zero number.

Thanks,
Liam