| Message ID | 20220614063933.13030-2-42.hyeyoo@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | CPA improvements | expand |
On Tue, 2022-06-14 at 15:39 +0900, Hyeonggon Yoo wrote: > Currently CPA is not used for user mappings (only pgd of init_mm > or and efi_mm is used). For simplicity, always fail when user address > is passed. > > Note that efi_mm uses 1:1 mapping so its address should not be > considered as user address. > > Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> > --- > arch/x86/mm/pat/set_memory.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/arch/x86/mm/pat/set_memory.c > b/arch/x86/mm/pat/set_memory.c > index 1abd5438f126..67cf969fed0d 100644 > --- a/arch/x86/mm/pat/set_memory.c > +++ b/arch/x86/mm/pat/set_memory.c > @@ -20,6 +20,7 @@ > #include <linux/kernel.h> > #include <linux/cc_platform.h> > #include <linux/set_memory.h> > +#include <linux/efi.h> > > #include <asm/e820/api.h> > #include <asm/processor.h> > @@ -1514,6 +1515,11 @@ static int __change_page_attr(struct cpa_data > *cpa, int primary) I guess having it here instead of __change_page_attr_set_clr() will result in the direct map alias addresses getting checked as well. Since these are determined inside of CPA, I'm not sure if it's needed as much. > pte_t *kpte, old_pte; > > address = __cpa_addr(cpa, cpa->curpage); > + > + if (WARN((IS_ENABLED(CONFIG_EFI) ? cpa->pgd != efi_mm.pgd : > true) Could it be? (!IS_ENABLED(CONFIG_EFI) || cpa->pgd != efi_mm.pgd) > + && address <= TASK_SIZE_MAX, > + KERN_WARNING "CPA: Got a user address")) > + return -EINVAL; > repeat: > kpte = _lookup_address_cpa(cpa, address, &level); > if (!kpte)
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. What does and should happen with the VDSO, for instance? It's a _PAGE_USER mapping, but it's >TASK_SIZE. Should set_page_attr() work on it?
On Tue, Jun 14, 2022 at 05:52:31PM +0000, Edgecombe, Rick P wrote: > On Tue, 2022-06-14 at 15:39 +0900, Hyeonggon Yoo wrote: > > Currently CPA is not used for user mappings (only pgd of init_mm > > or and efi_mm is used). For simplicity, always fail when user address > > is passed. > > > > Note that efi_mm uses 1:1 mapping so its address should not be > > considered as user address. > > > > Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> > > --- > > arch/x86/mm/pat/set_memory.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/arch/x86/mm/pat/set_memory.c > > b/arch/x86/mm/pat/set_memory.c > > index 1abd5438f126..67cf969fed0d 100644 > > --- a/arch/x86/mm/pat/set_memory.c > > +++ b/arch/x86/mm/pat/set_memory.c > > @@ -20,6 +20,7 @@ > > #include <linux/kernel.h> > > #include <linux/cc_platform.h> > > #include <linux/set_memory.h> > > +#include <linux/efi.h> > > > > #include <asm/e820/api.h> > > #include <asm/processor.h> > > @@ -1514,6 +1515,11 @@ static int __change_page_attr(struct cpa_data > > *cpa, int primary) > > I guess having it here instead of __change_page_attr_set_clr() will > result in the direct map alias addresses getting checked as well. Since > these are determined inside of CPA, I'm not sure if it's needed as > much. It does not check alias address when it failed. I put it in __change_page_attr() with CPA_ARRAY in mind. Because it may not be a single continuous area. > > > pte_t *kpte, old_pte; > > > > address = __cpa_addr(cpa, cpa->curpage); > > + > > + if (WARN((IS_ENABLED(CONFIG_EFI) ? cpa->pgd != efi_mm.pgd : > > true) > > Could it be? > > (!IS_ENABLED(CONFIG_EFI) || cpa->pgd != efi_mm.pgd) Looks better, will update in v2. Thanks! Hyeonggon > > > + && address <= TASK_SIZE_MAX, > > + KERN_WARNING "CPA: Got a user address")) > > + return -EINVAL; > > repeat: > > kpte = _lookup_address_cpa(cpa, address, &level); > > if (!kpte)
On Tue, Jun 14, 2022 at 03:39:32PM +0900, Hyeonggon Yoo wrote: > + > + 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; This looks rather unreadable. Why not something like: static bool cpa_addr_valid(struct cpa_data *cpa, unsigned long address) { if (address > TASK_SIZE_MAX) return true; if (IS_ENABLED(CONFIG_EFI) && cpa->pgd == efi_mm.pgd) return true; return false; } .... if (WARN_ON_ONCE(!cpa_addr_valid(cpa, address))) return -EINVAL;
On Wed, 2022-06-15 at 12:26 +0900, Hyeonggon Yoo wrote: > On Tue, Jun 14, 2022 at 05:52:31PM +0000, Edgecombe, Rick P wrote: > > On Tue, 2022-06-14 at 15:39 +0900, Hyeonggon Yoo wrote: > > > Currently CPA is not used for user mappings (only pgd of init_mm > > > or and efi_mm is used). For simplicity, always fail when user > > > address > > > is passed. > > > > > > Note that efi_mm uses 1:1 mapping so its address should not be > > > considered as user address. > > > > > > Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> > > > --- > > > arch/x86/mm/pat/set_memory.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/arch/x86/mm/pat/set_memory.c > > > b/arch/x86/mm/pat/set_memory.c > > > index 1abd5438f126..67cf969fed0d 100644 > > > --- a/arch/x86/mm/pat/set_memory.c > > > +++ b/arch/x86/mm/pat/set_memory.c > > > @@ -20,6 +20,7 @@ > > > #include <linux/kernel.h> > > > #include <linux/cc_platform.h> > > > #include <linux/set_memory.h> > > > +#include <linux/efi.h> > > > > > > #include <asm/e820/api.h> > > > #include <asm/processor.h> > > > @@ -1514,6 +1515,11 @@ static int __change_page_attr(struct > > > cpa_data > > > *cpa, int primary) > > > > I guess having it here instead of __change_page_attr_set_clr() will > > result in the direct map alias addresses getting checked as well. > > Since > > these are determined inside of CPA, I'm not sure if it's needed as > > much. > > It does not check alias address when it failed. > I put it in __change_page_attr() with CPA_ARRAY in mind. > Because it may not be a single continuous area. Makes sense. > > > > > > pte_t *kpte, old_pte; > > > > > > address = __cpa_addr(cpa, cpa->curpage); > > > + > > > + if (WARN((IS_ENABLED(CONFIG_EFI) ? cpa->pgd != efi_mm.pgd : > > > true) > > > > Could it be? > > > > (!IS_ENABLED(CONFIG_EFI) || cpa->pgd != efi_mm.pgd) > > Looks better, will update in v2. Christoph's seems better to me.
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. > 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) > 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. I think renaming fault_in_kernel_space() and using it would be more robust than simply using the expression (address <= TASK_SIZE_MAX).
On Wed, Jun 15, 2022 at 06:11:03AM -0700, Christoph Hellwig wrote: > On Tue, Jun 14, 2022 at 03:39:32PM +0900, Hyeonggon Yoo wrote: > > + > > + 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; > > This looks rather unreadable. > > Why not something like: > > static bool cpa_addr_valid(struct cpa_data *cpa, unsigned long address) > { > if (address > TASK_SIZE_MAX) > return true; > if (IS_ENABLED(CONFIG_EFI) && cpa->pgd == efi_mm.pgd) > return true; > return false; > } > > .... > > if (WARN_ON_ONCE(!cpa_addr_valid(cpa, address))) > return -EINVAL; > Thank your for feedback. I'll care more about readability in v2.
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 also shouldn't *result* in _PAGE_USER|_PAGE_PRESENT PTEs, even if those PTEs are in the kernel address space. 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.
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.
On 6/20/22 01:08, Hyeonggon Yoo wrote: >> 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? I don't see a reason why it should be able to clear _PAGE_USER|_PAGE_PRESENT entries either.
diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c index 1abd5438f126..67cf969fed0d 100644 --- a/arch/x86/mm/pat/set_memory.c +++ b/arch/x86/mm/pat/set_memory.c @@ -20,6 +20,7 @@ #include <linux/kernel.h> #include <linux/cc_platform.h> #include <linux/set_memory.h> +#include <linux/efi.h> #include <asm/e820/api.h> #include <asm/processor.h> @@ -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; repeat: kpte = _lookup_address_cpa(cpa, address, &level); if (!kpte)
Currently CPA is not used for user mappings (only pgd of init_mm or and efi_mm is used). For simplicity, always fail when user address is passed. Note that efi_mm uses 1:1 mapping so its address should not be considered as user address. Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> --- arch/x86/mm/pat/set_memory.c | 6 ++++++ 1 file changed, 6 insertions(+)