Re: [PATCH v3 2/5] mm/mseal: update madvise() logic

[David Hildenbrand]

> > > The justification for this change is to account for the case where:
> > >
> > > 1. A MAP_PRIVATE R/W file-backed mapping is established.
> > > 2. The mapping is written to, which backs it with anonymous memory.
> > > 3. The mapping is mprotect()'d read-only.
> > > 4. The mapping is mseal()'d.
> >
> > Thinking about this a bit (should have realized this implication earlier)
>
> Well none of us did...
> >> ... assuming we have:
> > 1. A MAP_PRIVATE R/O file-backed mapping.
> > 2. The mapping is mseal()'d.
> >
> > We only really have anon folios in there with things like (a) uprobe (b)
> > debugger access (c) similarly weird FOLL_FORCE stuff.
> >
> > Now, most executables/libraries are mapped that way. If someone would rely
> > on MADV_DONTNEED to zap pages in there (to free up memory), that would get
> > rejected.
>
> Right, yes.
>
> This is odd behaviour to me. But I guess this is what Jeff meant by 'detecting
> this' in android.

It's rather weird usage of MADV_DONTNEED, but maybe, for some R/O 
buffers ...

> The documentation is really not specific enough, we need to fix that. It's
> effectively stating any anon mappings are sealed, which is just not true with
> existing semantics.
>
> However I see:
>
> 	Memory sealing can automatically be applied by the runtime loader to
> 	seal .text and .rodata pages and applications can additionally seal
> 	security critical data at runtime.
>
> So yes, we're going to break MADV_DONTNEED of this mappings.
> > BUT.
> Would you really want to MADV_DONTNEED away uprobes etc.?? That seems... very
> strange and broken behaviour no?
>
> Note that, also, mappings of read-only files have VM_SHARED stripped. So they
> become read-only (With ~VM_MAYWRITE).
>
> To be clear this is where the mode of the file is read-only, not that the
> mapping is read-only alone.
>
> So with this change, we'd disallow discard of this.
>
> It'd be pretty odd to mseal() a read-only file-backed mapping and then try to
> discard, but maybe somebody would weirdly rely upon this?
>
> It's inconsistent, as a person MAP_SHARED mapping a file that is read/write but
> mapped read-only (or r/w of course), can discard fine eve if sealed, but if the
> file happens to be read-only can't.
>
> But we could add a VM_MAYWRITE check also.
>
> OK maybe I"m softening on the anon_vma thing see below.
>
> So we could combine these checks to avoid these issues.
>
>
> > Does something like that rely on MADV_DONTNEED working? Good question.
>
> Kees/Jeff? Can you check if android relies on this?
>
> > Checking for anon_vma in addition, ad mentioned in the other thread, would
> > be a "cheap" check to rule out that there are currently anon vmas in there.
> >
> > Well, not 100% reliable, because MADV_DONTNEED can race with page faults ...
>
> But hang on, it's read-only so we shouldn't get racing faults... right?

You mean, ones that populate anon folios.

Well, there is long-term pinning that can break COW and other weird 
stuff like FOLL_FORCE. Most of the latter probably holds the mmap lock 
in write mode. Probably.

> Hmm maybe I'll soften on this anon_vma idea then. Maybe it is a 'cheap fix' to
> rule out the _usual_ cases.

Yeah, something to evaluate.

--
Cheers,

David / dhildenb