Re: [PATCH v8 0/4] Introduce mseal

[Jeff Xu]

On Fri, Feb 2, 2024 at 10:52 AM Pedro Falcato <pedro.falcato@xxxxxxxxx> wrote:
>
> On Fri, Feb 2, 2024 at 5:59 PM Jeff Xu <jeffxu@xxxxxxxxxxxx> wrote:
> >
> > On Thu, Feb 1, 2024 at 9:00 PM Theo de Raadt <deraadt@xxxxxxxxxxx> wrote:
> > >
> > > Jeff Xu <jeffxu@xxxxxxxxxxxx> wrote:
> > >
> > > > Even without free.
> > > > I personally do not like the heap getting sealed like that.
> > > >
> > > > Component A.
> > > > p=malloc(4096);
> > > > writing something to p.
> > > >
> > > > Compohave nent B:
> > > > mprotect(p,4096, RO)
> > > > mseal(p,4096)
> > > >
> > > > This will split the heap VMA, and prevent the heap from shrinking, if
> > > > this is in a frequent code path, then it might hurt the process's
> > > > memory usage.
> > > >
> > > > The existing code is more likely to use malloc than mmap(), so it is
> > > > easier for dev to seal a piece of data belonging to another component.
> > > > I hope this pattern is not wide-spreading.
> > > >
> > > > The ideal way will be just changing the library A to use mmap.
> > >
> > > I think you are lacking some test programs to see how it actually
> > > behaves; the effect is worse than you think, and the impact is immediately
> > > visible to the programmer, and the lesson is clear:
> > >
> > >         you can only seal objects which you gaurantee never get recycled.
> > >
> > >         Pushing a sealed object back into reuse is a disasterous bug.
> > >
> > >         Noone should call this interface, unless they understand that.
> > >
> > > I'll say again, you don't have a test program for various allocators to
> > > understand how it behaves.  The failure modes described in your docuemnts
> > > are not correct.
> > >
> > I understand what you mean: I will add that part to the document:
> > Try to recycle a sealed memory is disastrous, e.g.
> > p=malloc(4096);
> > mprotect(p,4096,RO)
> > mseal(p,4096)
> > free(p);
> >
> > My point is:
> > I think sealing an object from the heap is a bad pattern in general,
> > even dev doesn't free it. That was one of the reasons for the sealable
> > flag, I hope saying this doesn't be perceived as looking for excuses.
>
> The point you're missing is that adding MAP_SEALABLE reduces
> composability. With MAP_SEALABLE, everything that mmaps some part of
> the address space that may ever be sealed will need to be modified to
> know about MAP_SEALABLE.
>
> Say you did the same thing for mprotect. MAP_PROTECT would control the
> mprotectability of the map. You'd stop:
>
> p = malloc(4096);
> mprotect(p, 4096, PROT_READ);
> free(p);
>
> ! But you'd need to change every spot that mmap()'s something to know
> about and use MAP_PROTECT: all "producers" of mmap memory would need
> to know about the consumers doing mprotect(). So now either all mmap()
> callers mindlessly add MAP_PROTECT out of fear the consumers do
> mprotect (and you gain nothing from MAP_PROTECT), or the mmap()
> callers need to know the consumers call mprotect(), and thus you
> introduce a huge layering violation (and you actually lose from having
> MAP_PROTECT).
>
> Hopefully you can map the above to MAP_SEALABLE. Or to any other m*()
> operation. For example, if chrome runs on an older glibc that does not
> know about MAP_SEALABLE, it will not be able to mseal() its own shared
> libraries' .text (even if, yes, that should ideally be left to ld.so).
>
I think I have heard enough complaints about MAP_SEALABLE from Linux
developers and Linus in the last two days to convince myself that it
is a bad idea :)

For the last time, I was trying to limit the scope of mseal() limited
to two known cases. And MAP_SEALABLE is a reversible decision, a
system ctrl can turn it off, or we can obsolete it in future. (this
was mentioned in the document of V8).

I will rest my case. Obviously from the feedback,  it is loud and
clear that we want to be able to seal all the memory.

> IMO, UNIX API design has historically mostly been "play stupid games,
> win stupid prizes", which is e.g: why things like close(STDOUT_FILENO)
> work. If you close stdout (and don't dup/reopen something to stdout)
> and printf(), things will break, and you get to keep both pieces.
> There's no O_CLOSEABLE, just as there's no O_DUPABLE.
>
> --
> Pedro