Re: [PATCH v8 0/4] Introduce mseal

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

* Jeff Xu <jeffxu@chromium.org> [240202 12:24]:

...

> > Provide code that uses this feature.

Please do this too :)

> >
> > Provide benchmark results where you apply mseal to 1, 2, 4, 8, 16, and
> > 32 VMAs.
> >
> I will prepare for the benchmark tests.

Thank you, please also include runs of calls that you are modifying for
checking for mseal() as we are adding loops there.

> 
> > Provide code that tests and checks the failure paths.  Failures at the
> > start, middle, and end of the modifications.
> >
> Regarding, "Failures at the start, middle, and end of the modifications."
> 
> With the current implementation, e.g. it checks if the sealing is
> applied before actual modification of VMAs, so partial modifications
> are avoided in mprotect, mremap, munmap.
> 
> There are test cases in the selftests to cover the failure path,
> including the beginning, middle and end of VMAs.
> test_seal_unmapped_start
> test_seal_unmapped_middle
> test_seal_unmapped_end
> test_seal_invalid_input
> test_seal_start_mprotect
> test_seal_end_mprotect
> etc.
> 
> Are those what you are looking for ?

Those are certainly good, but we need more checking in there.  You have
a seal_split test that splits the vma by mseal but you don't check the
flags on the VMAs.

What I'm more concerned about is what happens if you call mseal() on a
range and it can mseal a portion.  Like, what happens to the first vma
in your test_seal_unmapped_middle case?  I see it returns an error, but
is the first VMA mseal()'ed? (no it's not, but test that)

What about the other system calls that will be denied on an mseal() VMA?
Do they still behave the same?  do_mprotect_pkey() will break out of the
loop on the first error it sees - but it has modified some VMAs up to
that point, I believe?  You have changed this to abort before anything
is modified.  This is probably acceptable because it won't affect
existing applications unless they start using mseal(), but that's just
my opinion.

It would be good to state the change in behaviour because it is changing
the fundamental model of changing mprotect/madvise until an issue is
hit.  I think you are covering this by "it blocks X" but it's doing more
than, say, a flag verification.  One could reasonably assume this is
just another flag verification.

> 
> > Document what happens in those failure paths.

I'd like to know how this affects other system calls in the partial
success cases/return error cases.  Some will now return new error codes
and some may change the behaviour.

It may even be okay to allow munmap() to split VMAs at the start/end of
the region and fail to munmap because some VMA in the middle is
mseal()'ed - but maybe not?  I haven't put a whole lot of thought into
it.

Thanks,
Liam