On Thu, Jun 16, 2022 at 07:20:09AM -0700, Dave Hansen wrote: > On 6/16/22 01:49, Hyeonggon Yoo wrote: > > On Tue, Jun 14, 2022 at 11:31:48AM -0700, Dave Hansen wrote: > >> On 6/13/22 23:39, Hyeonggon Yoo wrote: > >>> @@ -1514,6 +1515,11 @@ static int __change_page_attr(struct cpa_data *cpa, int primary) > >>> pte_t *kpte, old_pte; > >>> > >>> address = __cpa_addr(cpa, cpa->curpage); > >>> + > >>> + if (WARN((IS_ENABLED(CONFIG_EFI) ? cpa->pgd != efi_mm.pgd : true) > >>> + && address <= TASK_SIZE_MAX, > >>> + KERN_WARNING "CPA: Got a user address")) > >>> + return -EINVAL; > >> > >> I was expecting this to actually go after _PAGE_USER, not necessarily > >> userspace addresses themselves. > > > > userspace ptes may not have _PAGE_USER set. (e.g. swap entry) > > I think it's more accurate to go after user addresses. > > It would, of course, have to be paired with _PAGE_PRESENT checks. This > works both on the way in and out of the set_memory code. It shouldn't > clear other bits a PTE with _PAGE_PRESENT|_PAGE_USER and You mean nothing should not use set_memory code for PTEs with _PAGE_USER|_PAGE_PRESENT but set_memory can still be used to clear _PAGE_USER|_PAGE_PRESENT? Can't we just simply deny any PTE/PMDs with _PAGE_PRESENT|_PAGE_USER? > also shouldn't > *result* in _PAGE_USER|_PAGE_PRESENT PTEs, even if those PTEs are in the > kernel address space. Makes sense. > Filtering on the addresses also makes sense. > > >> What does and should happen with the VDSO, for instance? It's a > >> _PAGE_USER mapping, but it's >TASK_SIZE. > > > > you mean vsyscall? AFAIK address of mapped vDSO image is < TASK_SIZE. > > (or please tell me I'm wrong) > > You're right. That was a silly thinko. > > >> Should set_page_attr() work on it? > > > > vsyscall does not need CPA functionalities. > > So I don't think it (__change_page_attr()) should work on vsyscall. > > Agreed. -- Thanks, Hyeonggon
