| Message ID | 20180717112029.42378-3-kirill.shutemov@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/17/2018 04:20 AM, Kirill A. Shutemov wrote: > Zero page is not encrypted and putting it into encrypted VMA produces > garbage. > > We can map zero page with KeyID-0 into an encrypted VMA, but this would > be violation security boundary between encryption domains. Why? How is it a violation? It only matters if they write secrets. They can't write secrets to the zero page. Is this only because you accidentally inherited ->vm_page_prot on the zero page PTE?
On Wed, Jul 18, 2018 at 10:36:24AM -0700, Dave Hansen wrote: > On 07/17/2018 04:20 AM, Kirill A. Shutemov wrote: > > Zero page is not encrypted and putting it into encrypted VMA produces > > garbage. > > > > We can map zero page with KeyID-0 into an encrypted VMA, but this would > > be violation security boundary between encryption domains. > > Why? How is it a violation? > > It only matters if they write secrets. They can't write secrets to the > zero page. I believe usage of zero page is wrong here. It would indirectly reveal content of supposedly encrypted memory region. I can see argument why it should be okay and I don't have very strong opinion on this. If folks see it's okay to use zero page in encrypted VMAs I can certainly make it work. > Is this only because you accidentally inherited ->vm_page_prot on the > zero page PTE? Yes, in previous patchset I mapped zero page with wrong KeyID. This is one of possible fixes for this.
On 07/19/2018 12:16 AM, Kirill A. Shutemov wrote: > On Wed, Jul 18, 2018 at 10:36:24AM -0700, Dave Hansen wrote: >> On 07/17/2018 04:20 AM, Kirill A. Shutemov wrote: >>> Zero page is not encrypted and putting it into encrypted VMA produces >>> garbage. >>> >>> We can map zero page with KeyID-0 into an encrypted VMA, but this would >>> be violation security boundary between encryption domains. >> Why? How is it a violation? >> >> It only matters if they write secrets. They can't write secrets to the >> zero page. > I believe usage of zero page is wrong here. It would indirectly reveal > content of supposedly encrypted memory region. > > I can see argument why it should be okay and I don't have very strong > opinion on this. I think we should make the zero page work. If folks are security-sensitive, they need to write to guarantee it isn't being shared. That's a pretty low bar. I'm struggling to think of a case where an attacker has access to the encrypted data, the virt->phys mapping, *and* can glean something valuable from the presence of the zero page. Please spend some time and focus on your patch descriptions. Use facts that are backed up and are *precise* or tell the story of how your patch was developed. In this case, citing the "security boundary" is not precise enough without explaining what the boundary is and how it is violated.
On Thu, Jul 19, 2018 at 06:58:14AM -0700, Dave Hansen wrote: > On 07/19/2018 12:16 AM, Kirill A. Shutemov wrote: > > On Wed, Jul 18, 2018 at 10:36:24AM -0700, Dave Hansen wrote: > >> On 07/17/2018 04:20 AM, Kirill A. Shutemov wrote: > >>> Zero page is not encrypted and putting it into encrypted VMA produces > >>> garbage. > >>> > >>> We can map zero page with KeyID-0 into an encrypted VMA, but this would > >>> be violation security boundary between encryption domains. > >> Why? How is it a violation? > >> > >> It only matters if they write secrets. They can't write secrets to the > >> zero page. > > I believe usage of zero page is wrong here. It would indirectly reveal > > content of supposedly encrypted memory region. > > > > I can see argument why it should be okay and I don't have very strong > > opinion on this. > > I think we should make the zero page work. If folks are > security-sensitive, they need to write to guarantee it isn't being > shared. That's a pretty low bar. > > I'm struggling to think of a case where an attacker has access to the > encrypted data, the virt->phys mapping, *and* can glean something > valuable from the presence of the zero page. Okay. > Please spend some time and focus on your patch descriptions. Use facts > that are backed up and are *precise* or tell the story of how your patch > was developed. In this case, citing the "security boundary" is not > precise enough without explaining what the boundary is and how it is > violated. Fair enough. I'll go though all commit messages once again. Sorry.
diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h index 5ab636089c60..2e8658962aae 100644 --- a/arch/s390/include/asm/pgtable.h +++ b/arch/s390/include/asm/pgtable.h @@ -505,7 +505,7 @@ static inline int mm_alloc_pgste(struct mm_struct *mm) * In the case that a guest uses storage keys * faults should no longer be backed by zero pages */ -#define mm_forbids_zeropage mm_has_pgste +#define vma_forbids_zeropage(vma) mm_has_pgste(vma->vm_mm) static inline int mm_uses_skeys(struct mm_struct *mm) { #ifdef CONFIG_PGSTE diff --git a/include/linux/mm.h b/include/linux/mm.h index c8780c5835ad..151d6e6b16e5 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -92,8 +92,8 @@ extern int mmap_rnd_compat_bits __read_mostly; * s390 does this to prevent multiplexing of hardware bits * related to the physical page in case of virtualization. */ -#ifndef mm_forbids_zeropage -#define mm_forbids_zeropage(X) (0) +#ifndef vma_forbids_zeropage +#define vma_forbids_zeropage(vma) vma_keyid(vma) #endif /* diff --git a/mm/huge_memory.c b/mm/huge_memory.c index 1cd7c1a57a14..83f096c7299b 100644 --- a/mm/huge_memory.c +++ b/mm/huge_memory.c @@ -676,8 +676,7 @@ int do_huge_pmd_anonymous_page(struct vm_fault *vmf) return VM_FAULT_OOM; if (unlikely(khugepaged_enter(vma, vma->vm_flags))) return VM_FAULT_OOM; - if (!(vmf->flags & FAULT_FLAG_WRITE) && - !mm_forbids_zeropage(vma->vm_mm) && + if (!(vmf->flags & FAULT_FLAG_WRITE) && !vma_forbids_zeropage(vma) && transparent_hugepage_use_zero_page()) { pgtable_t pgtable; struct page *zero_page; diff --git a/mm/memory.c b/mm/memory.c index 02fbef2bd024..a705637d2ded 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -3139,8 +3139,7 @@ static int do_anonymous_page(struct vm_fault *vmf) return 0; /* Use the zero-page for reads */ - if (!(vmf->flags & FAULT_FLAG_WRITE) && - !mm_forbids_zeropage(vma->vm_mm)) { + if (!(vmf->flags & FAULT_FLAG_WRITE) && !vma_forbids_zeropage(vma)) { entry = pte_mkspecial(pfn_pte(my_zero_pfn(vmf->address), vma->vm_page_prot)); vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
Zero page is not encrypted and putting it into encrypted VMA produces garbage. We can map zero page with KeyID-0 into an encrypted VMA, but this would be violation security boundary between encryption domains. Forbid zero pages in encrypted VMAs. Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> --- arch/s390/include/asm/pgtable.h | 2 +- include/linux/mm.h | 4 ++-- mm/huge_memory.c | 3 +-- mm/memory.c | 3 +-- 4 files changed, 5 insertions(+), 7 deletions(-)