Re: [PATCH v5 18/39] mm: Handle faultless write upgrades for shstk

From: David Hildenbrand
Date: Wed Jan 25 2023 - 04:28:04 EST

On 24.01.23 19:14, Edgecombe, Rick P wrote:
On Tue, 2023-01-24 at 17:24 +0100, David Hildenbrand wrote:
On 23.01.23 21:47, Edgecombe, Rick P wrote:
On Mon, 2023-01-23 at 10:50 +0100, David Hildenbrand wrote:
On 19.01.23 22:22, Rick Edgecombe wrote:
The x86 Control-flow Enforcement Technology (CET) feature
includes
a new
type of memory called shadow stack. This shadow stack memory
has
some
unusual properties, which requires some core mm changes to
function
properly.

Since shadow stack memory can be changed from userspace, is
both
VM_SHADOW_STACK and VM_WRITE. But it should not be made
conventionally
writable (i.e. pte_mkwrite()). So some code that calls
pte_mkwrite() needs
to be adjusted.

One such case is when memory is made writable without an actual
write
fault. This happens in some mprotect operations, and also
prot_numa
faults.
In both cases code checks whether it should be made
(conventionally)
writable by calling vma_wants_manual_pte_write_upgrade().

One way to fix this would be have code actually check if memory
is
also
VM_SHADOW_STACK and in that case call pte_mkwrite_shstk(). But
since
most memory won't be shadow stack, just have simpler logic and
skip
this
optimization by changing vma_wants_manual_pte_write_upgrade()
to
not
return true for VM_SHADOW_STACK_MEMORY. This will simply handle
all
cases of this type.

Cc: David Hildenbrand <david@xxxxxxxxxx>
Tested-by: Pengfei Xu <pengfei.xu@xxxxxxxxx>
Tested-by: John Allen <john.allen@xxxxxxx>
Signed-off-by: Yu-cheng Yu <yu-cheng.yu@xxxxxxxxx>
Reviewed-by: Kirill A. Shutemov <
kirill.shutemov@xxxxxxxxxxxxxxx>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx>
---

Instead of having these x86-shadow stack details all over the MM
space,
was the option explored to handle this more in arch specific
code?

IIUC, one way to get it working would be

1) Have a SW "shadowstack" PTE flag.
2) Have an "SW-dirty" PTE flag, to store "dirty=1" when
"write=0".

I don't think that idea came up. So vma->vm_page_prot would have
the SW
shadow stack flag for VM_SHADOW_STACK, and pte_mkwrite() could do
Write=0,Dirty=1 part. It seems like it should work.


Right, if we include it in vma->vm_page_prot, we'd immediately let
mk_pte() just handle that.

Otherwise, we'd have to refactor e.g., mk_pte() to consume a vma
instead
of the vma->vm_page_prot. Let's see if we can avoid that for now.


pte_mkwrite(), pte_write(), pte_dirty ... can then make decisions
based
on the "shadowstack" PTE flag and hide all these details from
core-
mm.

When mapping a shadowstack page (new page, migration, swapin,
...),
which can be obtained by looking at the VMA flags, the first
thing
you'd
do is set the "shadowstack" PTE flag.

I guess the downside is that it uses an extra software bit. But the
other positive is that it's less error prone, so that someone
writing
core-mm code won't introduce a change that makes shadow stack VMAs
Write=1 if they don't know to also check for VM_SHADOW_STACK.

Right. And I think this mimics the what I would have expected HW to
provide: a dedicated HW bit, not somehow mangling this into semantics
of
existing bits.

Yea.


Roughly speaking: if we abstract it that way and get all of the "how
to
set it writable now?" out of core-MM, it not only is cleaner and
less
error prone, it might even allow other architectures that implement
something comparable (e.g., using a dedicated HW bit) to actually
reuse
some of that work. Otherwise most of that "shstk" is really just x86
specific ...

I guess the only cases we have to special case would be page pinning
code where pte_write() would indicate that the PTE is writable (well,
it
is, just not by "ordinary CPU instruction" context directly): but you
do
that already, so ... :)

Sorry for stumbling over that this late, I only started looking into
this when you CCed me on that one patch.

Sorry for not calling more attention to it earlier. Appreciate your
comments.

Previously versions of this series had changed some of these
pte_mkwrite() calls to maybe_mkwrite(), which of course takes a vma.
This way an x86 implementation could use the VM_SHADOW_STACK vma flag
to decide between pte_mkwrite() and pte_mkwrite_shstk(). The feedback
was that in some of these code paths "maybe" isn't really an option, it
*needs* to make it writable. Even though the logic was the same, the
name of the function made it look wrong.

But another option could be to change pte_mkwrite() to take a vma. This
would save using another software bit on x86, but instead requires a
small change to each arch's pte_mkwrite().

I played with that idea shortly as well, but discarded it. I was not able to convince myself that it wouldn't be required to pass in the VMA as well for things like pte_dirty(), pte_mkdirty(), pte_write(), ... which would end up fairly ugly (or even impossible in thing slike GUP-fast).

For example, I wonder how we'd be handling stuff like do_numa_page() cleanly correctly, where we use pte_modify() + pte_mkwrite(), and either call might set the PTE writable and maintain dirty bit ...

Having that said, maybe it could work with only a single saved-dirty bit and passing in the VMA for pte_mkwrite() only.

pte_wrprotect() would detect "writable=0,dirty=1" and move the dirty bit to the soft-dirty bit instead, resulting in "writable=0,dirty=0,saved-dirty=1",

pte_dirty() would return dirty==1||saved-dirty==1.

pte_mkdirty() would set either set dirty=1 or saved-dirty=1, depending on the writable bit.

pte_mkclean() would clean both bits.

pte_write() would detect "writable == 1 || (writable==0 && dirty==1)"

pte_mkwrite() would act according to the VMA, and in addition, merge the saved-dirty bit into the dirty bit.

pte_modify() and mk_pte() .... would require more thought ...


Further, ptep_modify_prot_commit() might have to be adjusted to properly flush in all relevant cases IIRC.


x86's pte_mkwrite() would then be pretty close to maybe_mkwrite(), but
maybe it could additionally warn if the vma is not writable. It also
seems more aligned with your changes to stop taking hints from PTE bits
and just look at the VMA? (I'm thinking about the dropping of the dirty
check in GUP and dropping pte_saved_write())

The soft-shstk bit wouldn't be a hint, it would be logically changing the "type" of the PTE such that any other PTE functions can do the right thing without having to consume the VMA.


--
Thanks,

David / dhildenb