Re: [PATCH v8 0/4] Introduce mseal

[Jeff Xu]

On Fri, Feb 2, 2024 at 11:21 AM Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> wrote:
>
> * Jeff Xu <jeffxu@xxxxxxxxxxxx> [240202 12:24]:
>
> ...
>
> > > Provide code that uses this feature.
>
> Please do this too :)
>
Yes. Will do.


> > >
> > > 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.
>
It will includes mmap/mremap/mprotect/munmap

> >
> > > 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.
>
I can add the flag check.

> 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)
>
The first VMA is not sealed.
That was covered by test_seal_mprotect_two_vma_with_gap.

> What about the other system calls that will be denied on an mseal() VMA?
The other system call's behavior is kept as is, if the memory is not sealed.

> 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?
Yes. The description about do_mprotect_pkey() is correct.

> 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.
>
To make sure this, the test was written with sealing=false, those
tests are passed in the main (before applying my patch) to make sure
the test is correct.

> 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.
>
Will add more in documentation.

> >
> > > 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.
>
For the mapping that is not sealed, all remain unchanged, including
the error handling path.
For the mapping that is sealed, EPREM is returned if the sealing check
fails, and all of VMAs remain unchanged.

> 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.
If you are referring to something like below
[unmapped][map1][unmapped][map2][unmapped][map3][unmapped]
and map2 is sealed.

unmap(start of map1,end of map3) will fail.
mmap/mremap/unmap/mprotect on an address range that includes map2 will
fail with EPERM, with map1/map2/map3 unchanged.

Thanks
-Jeff

>
> Thanks,
> Liam