Messages in this thread | | | Date | Wed, 25 Jan 2023 10:27:04 +0100 | From | David Hildenbrand <> | Subject | Re: [PATCH v5 18/39] mm: Handle faultless write upgrades for shstk |
| |
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@redhat.com> >>>>> Tested-by: Pengfei Xu <pengfei.xu@intel.com> >>>>> Tested-by: John Allen <john.allen@amd.com> >>>>> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> >>>>> Reviewed-by: Kirill A. Shutemov < >>>>> kirill.shutemov@linux.intel.com> >>>>> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> >>>>> --- >>>> >>>> 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
| |