On Fri, Jun 29, 2018 at 4:47 PM, Daniel Borkmann <dan...@iogearbox.net> wrote: > On 06/29/2018 08:42 PM, Kees Cook wrote: >> On Thu, Jun 28, 2018 at 2:34 PM, Daniel Borkmann <dan...@iogearbox.net> >> wrote: >>> Kees suggested that if set_memory_*() can fail, we should annotate it with >>> __must_check, and all callers need to deal with it gracefully given those >>> set_memory_*() markings aren't "advisory", but they're expected to actually >>> do what they say. This might be an option worth to move forward in future >>> but would at the same time require that set_memory_*() calls from supporting >>> archs are guaranteed to be "atomic" in that they provide rollback if part >>> of the range fails, once that happened, the transition from RW -> RO could >>> be made more robust that way, while subsequent RO -> RW transition /must/ >>> continue guaranteeing to always succeed the undo part. >> >> Does this mean we can have BPF filters that aren't read-only then? >> What's the situation where set_memory_ro() fails? (Can it be induced >> by the user?) > > My understanding is that the cpa_process_alias() would attempt to also change > attributes of physmap ranges, and it found that a large page had to be split > for this but failed in doing so thus attributes couldn't be updated there due > to page alloc error. Attempting to change the primary mapping which would be > directly the addr passed to set_memory_ro() was however set to read-only > despite error. While for reproduction I had a toggle on the alloc_pages() in > split_large_page() to have it fail, I only could trigger it occasionally; I > used the selftest suite in a loop to stress test and it hit about or twice > over hours.
Okay, so it's pretty rare; that's good! :P It really seems like this should be a situation that never fails, but if we ARE going to allow failures, then I think we need to propagate them up to callers. That means modules could fail to load in these cases, etc, etc. Since this is a fundamental protection, we need to either never fail to set things RO or we need to disallow operation continuing in the face of something NOT being RO. -Kees -- Kees Cook Pixel Security
