Re: [PATCH v10 0/5] Introduce mseal

["Liam R. Howlett" <Liam.Howlett@oracle.com>]

* Theo de Raadt <deraadt@openbsd.org> [240514 22:42]:
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > On Tue, 14 May 2024 at 18:47, Theo de Raadt <deraadt@openbsd.org> wrote:
> > >
> > > Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > >
> > > Regarding mprotect(), POSIX also says:
> > >
> > >     An implementation may permit accesses other than those specified by
> > >     prot; however, no implementation shall permit a write to succeed where
> > >     PROT_WRITE has not been set or shall permit any access where PROT_NONE
> > >     alone has been set.
> > 
> > Why do you quote entirely irrelevant issues?
> > 
> > If the mprotect didn't succeed, then clearly the above is irrelevant.
> 
> Imagine the following region:
> 
> 
>     <--------------------------------------------- len
>     [region PROT_READ] [region PROT_READ + sealed] 
> addr ^
> 
> then perform
>     mprotect(addr, len, PROT_WRITE | PROT_READ);
> 
> This will return -1, with EPERM, when it encounters the sealed region.
> 
> I believe in Linux, since it has not checked for errors as a first
> phase, this changes the first region of memory to PROT_READ |
> PROT_WRITE.  Liam, is that correct?

I really don't want to fight about this - I just want to have reliable
code that is maintainable.  I think the correctness argument is always
going to be unclear because we're all going to interpret the
documentation from our point of view - which is probably how we got here
in the first place.  My opinion of the matter of correctness is,
obviously, the least important.

My problem right now is that we're changing it so that we are not
consistent in when we should check.  I'm not sure how doing both fits
into either model, but it increases the next change going to the 'wrong'
side of the argument (whatever side that happens to be from your view).

If there isn't a technical reason to keep the check before, then we
should treat mseal the same as all other checks.

If we are going to have an up-front check, then it makes sense to keep
the checks that we can (reasonably) do at the same time together.
Linus, you said up front checks is a good thing to aim for.

That said, I don't think the example above will allow the madvise to
succeed at all.  mseal checks the entire region up front while most
other checks occur during the loop across vmas.

Thanks,
Liam