head 1.2; access; symbols pkgsrc-2020Q3:1.1.0.2; locks; strict; comment @# @; 1.2 date 2020.11.06.21.45.49; author bouyer; state dead; branches; next 1.1; commitid WDPVMlrHGXeceSuC; 1.1 date 2020.10.21.09.04.10; author bouyer; state Exp; branches 1.1.2.1; next ; commitid z0QsOseMMzGbyKsC; 1.1.2.1 date 2020.10.21.09.04.10; author bsiegert; state dead; branches; next 1.1.2.2; commitid 4LJlEfpW0QY3ZUsC; 1.1.2.2 date 2020.10.22.16.29.05; author bsiegert; state Exp; branches; next ; commitid 4LJlEfpW0QY3ZUsC; desc @@ 1.2 log @Update xenkernel413 and xentools413 to 4.13.2. This includes fixes for XSA up to XSA347, and an improved fix for XSA 286. @ text @$NetBSD: patch-XSA347,v 1.1 2020/10/21 09:04:10 bouyer Exp $ From: Jan Beulich Subject: AMD/IOMMU: convert amd_iommu_pte from struct to union This is to add a "raw" counterpart to the bitfield equivalent. Take the opportunity and - convert fields to bool / unsigned int, - drop the naming of the reserved field, - shorten the names of the ignored ones. This is part of XSA-347. Signed-off-by: Jan Beulich Reviewed-by: Andrew Cooper Reviewed-by: Paul Durrant --- xen/drivers/passthrough/amd/iommu_map.c.orig +++ xen/drivers/passthrough/amd/iommu_map.c @@@@ -38,7 +38,7 @@@@ static unsigned int pfn_to_pde_idx(unsig static unsigned int clear_iommu_pte_present(unsigned long l1_mfn, unsigned long dfn) { - struct amd_iommu_pte *table, *pte; + union amd_iommu_pte *table, *pte; unsigned int flush_flags; table = map_domain_page(_mfn(l1_mfn)); @@@@ -52,7 +52,7 @@@@ static unsigned int clear_iommu_pte_pres return flush_flags; } -static unsigned int set_iommu_pde_present(struct amd_iommu_pte *pte, +static unsigned int set_iommu_pde_present(union amd_iommu_pte *pte, unsigned long next_mfn, unsigned int next_level, bool iw, bool ir) @@@@ -87,7 +87,7 @@@@ static unsigned int set_iommu_pte_presen int pde_level, bool iw, bool ir) { - struct amd_iommu_pte *table, *pde; + union amd_iommu_pte *table, *pde; unsigned int flush_flags; table = map_domain_page(_mfn(pt_mfn)); @@@@ -178,7 +178,7 @@@@ void iommu_dte_set_guest_cr3(struct amd_ static int iommu_pde_from_dfn(struct domain *d, unsigned long dfn, unsigned long pt_mfn[], bool map) { - struct amd_iommu_pte *pde, *next_table_vaddr; + union amd_iommu_pte *pde, *next_table_vaddr; unsigned long next_table_mfn; unsigned int level; struct page_info *table; @@@@ -458,7 +458,7 @@@@ int __init amd_iommu_quarantine_init(str unsigned long end_gfn = 1ul << (DEFAULT_DOMAIN_ADDRESS_WIDTH - PAGE_SHIFT); unsigned int level = amd_iommu_get_paging_mode(end_gfn); - struct amd_iommu_pte *table; + union amd_iommu_pte *table; if ( hd->arch.root_table ) { @@@@ -489,7 +489,7 @@@@ int __init amd_iommu_quarantine_init(str for ( i = 0; i < PTE_PER_TABLE_SIZE; i++ ) { - struct amd_iommu_pte *pde = &table[i]; + union amd_iommu_pte *pde = &table[i]; /* * PDEs are essentially a subset of PTEs, so this function --- xen/drivers/passthrough/amd/pci_amd_iommu.c.orig +++ xen/drivers/passthrough/amd/pci_amd_iommu.c @@@@ -390,7 +390,7 @@@@ static void deallocate_next_page_table(s static void deallocate_page_table(struct page_info *pg) { - struct amd_iommu_pte *table_vaddr; + union amd_iommu_pte *table_vaddr; unsigned int index, level = PFN_ORDER(pg); PFN_ORDER(pg) = 0; @@@@ -405,7 +405,7 @@@@ static void deallocate_page_table(struct for ( index = 0; index < PTE_PER_TABLE_SIZE; index++ ) { - struct amd_iommu_pte *pde = &table_vaddr[index]; + union amd_iommu_pte *pde = &table_vaddr[index]; if ( pde->mfn && pde->next_level && pde->pr ) { @@@@ -557,7 +557,7 @@@@ static void amd_dump_p2m_table_level(str paddr_t gpa, int indent) { paddr_t address; - struct amd_iommu_pte *table_vaddr; + const union amd_iommu_pte *table_vaddr; int index; if ( level < 1 ) @@@@ -573,7 +573,7 @@@@ static void amd_dump_p2m_table_level(str for ( index = 0; index < PTE_PER_TABLE_SIZE; index++ ) { - struct amd_iommu_pte *pde = &table_vaddr[index]; + const union amd_iommu_pte *pde = &table_vaddr[index]; if ( !(index % 2) ) process_pending_softirqs(); --- xen/include/asm-x86/hvm/svm/amd-iommu-defs.h.orig +++ xen/include/asm-x86/hvm/svm/amd-iommu-defs.h @@@@ -465,20 +465,23 @@@@ union amd_iommu_x2apic_control { #define IOMMU_PAGE_TABLE_U32_PER_ENTRY (IOMMU_PAGE_TABLE_ENTRY_SIZE / 4) #define IOMMU_PAGE_TABLE_ALIGNMENT 4096 -struct amd_iommu_pte { - uint64_t pr:1; - uint64_t ignored0:4; - uint64_t a:1; - uint64_t d:1; - uint64_t ignored1:2; - uint64_t next_level:3; - uint64_t mfn:40; - uint64_t reserved:7; - uint64_t u:1; - uint64_t fc:1; - uint64_t ir:1; - uint64_t iw:1; - uint64_t ignored2:1; +union amd_iommu_pte { + uint64_t raw; + struct { + bool pr:1; + unsigned int ign0:4; + bool a:1; + bool d:1; + unsigned int ign1:2; + unsigned int next_level:3; + uint64_t mfn:40; + unsigned int :7; + bool u:1; + bool fc:1; + bool ir:1; + bool iw:1; + unsigned int ign2:1; + }; }; /* Paging modes */ From: Jan Beulich Subject: AMD/IOMMU: update live PTEs atomically Updating a live PTE bitfield by bitfield risks the compiler re-ordering the individual updates as well as splitting individual updates into multiple memory writes. Construct the new entry fully in a local variable, do the check to determine the flushing needs on the thus established new entry, and then write the new entry by a single insn. Similarly using memset() to clear a PTE is unsafe, as the order of writes the function does is, at least in principle, undefined. This is part of XSA-347. Signed-off-by: Jan Beulich Reviewed-by: Paul Durrant --- xen/drivers/passthrough/amd/iommu_map.c.orig +++ xen/drivers/passthrough/amd/iommu_map.c @@@@ -45,7 +45,7 @@@@ static unsigned int clear_iommu_pte_pres pte = &table[pfn_to_pde_idx(dfn, 1)]; flush_flags = pte->pr ? IOMMU_FLUSHF_modified : 0; - memset(pte, 0, sizeof(*pte)); + write_atomic(&pte->raw, 0); unmap_domain_page(table); @@@@ -57,26 +57,30 @@@@ static unsigned int set_iommu_pde_presen unsigned int next_level, bool iw, bool ir) { + union amd_iommu_pte new = {}, old; unsigned int flush_flags = IOMMU_FLUSHF_added; - if ( pte->pr && - (pte->mfn != next_mfn || - pte->iw != iw || - pte->ir != ir || - pte->next_level != next_level) ) - flush_flags |= IOMMU_FLUSHF_modified; - /* * FC bit should be enabled in PTE, this helps to solve potential * issues with ATS devices */ - pte->fc = !next_level; + new.fc = !next_level; + + new.mfn = next_mfn; + new.iw = iw; + new.ir = ir; + new.next_level = next_level; + new.pr = true; + + old.raw = read_atomic(&pte->raw); + old.ign0 = 0; + old.ign1 = 0; + old.ign2 = 0; + + if ( old.pr && old.raw != new.raw ) + flush_flags |= IOMMU_FLUSHF_modified; - pte->mfn = next_mfn; - pte->iw = iw; - pte->ir = ir; - pte->next_level = next_level; - pte->pr = 1; + write_atomic(&pte->raw, new.raw); return flush_flags; } From: Jan Beulich Subject: AMD/IOMMU: ensure suitable ordering of DTE modifications DMA and interrupt translation should be enabled only after other applicable DTE fields have been written. Similarly when disabling translation or when moving a device between domains, translation should first be disabled, before other entry fields get modified. Note however that the "moving" aspect doesn't apply to the interrupt remapping side, as domain specifics are maintained in the IRTEs here, not the DTE. We also never disable interrupt remapping once it got enabled for a device (the respective argument passed is always the immutable iommu_intremap). This is part of XSA-347. Signed-off-by: Jan Beulich Reviewed-by: Paul Durrant --- xen/drivers/passthrough/amd/iommu_map.c.orig +++ xen/drivers/passthrough/amd/iommu_map.c @@@@ -107,11 +107,18 @@@@ void amd_iommu_set_root_page_table(struc uint64_t root_ptr, uint16_t domain_id, uint8_t paging_mode, bool valid) { + if ( valid || dte->v ) + { + dte->tv = false; + dte->v = true; + smp_wmb(); + } dte->domain_id = domain_id; dte->pt_root = paddr_to_pfn(root_ptr); dte->iw = true; dte->ir = true; dte->paging_mode = paging_mode; + smp_wmb(); dte->tv = true; dte->v = valid; } @@@@ -134,6 +141,7 @@@@ void amd_iommu_set_intremap_table( } dte->ig = false; /* unmapped interrupts result in i/o page faults */ + smp_wmb(); dte->iv = valid; } --- xen/drivers/passthrough/amd/pci_amd_iommu.c.orig +++ xen/drivers/passthrough/amd/pci_amd_iommu.c @@@@ -120,7 +120,10 @@@@ static void amd_iommu_setup_domain_devic /* Undo what amd_iommu_disable_domain_device() may have done. */ ivrs_dev = &get_ivrs_mappings(iommu->seg)[req_id]; if ( dte->it_root ) + { dte->int_ctl = IOMMU_DEV_TABLE_INT_CONTROL_TRANSLATED; + smp_wmb(); + } dte->iv = iommu_intremap; dte->ex = ivrs_dev->dte_allow_exclusion; dte->sys_mgt = MASK_EXTR(ivrs_dev->device_flags, ACPI_IVHD_SYSTEM_MGMT); @ 1.1 log @Add upstream security patches for XSA286, XSA345, XSA346, XSA347. Bump PKGREVISION. @ text @d1 1 a1 1 $NetBSD: $ @ 1.1.2.1 log @file patch-XSA347 was added on branch pkgsrc-2020Q3 on 2020-10-22 16:29:05 +0000 @ text @d1 282 @ 1.1.2.2 log @Pullup ticket #6355 - requested by bouyer sysutils/xenkernel411: security fix sysutils/xenkernel413: security fix Revisions pulled up: - sysutils/xenkernel411/Makefile 1.17 - sysutils/xenkernel411/distinfo 1.15 - sysutils/xenkernel411/patches/patch-XSA286 1.1 - sysutils/xenkernel411/patches/patch-XSA345 1.1 - sysutils/xenkernel411/patches/patch-XSA346 1.1 - sysutils/xenkernel411/patches/patch-XSA347 1.1 - sysutils/xenkernel413/Makefile 1.6 - sysutils/xenkernel413/distinfo 1.4 - sysutils/xenkernel413/patches/patch-XSA286 1.1 - sysutils/xenkernel413/patches/patch-XSA345 1.1 - sysutils/xenkernel413/patches/patch-XSA346 1.1 - sysutils/xenkernel413/patches/patch-XSA347 1.1 --- Module Name: pkgsrc Committed By: bouyer Date: Wed Oct 21 09:03:05 UTC 2020 Modified Files: pkgsrc/sysutils/xenkernel411: Makefile distinfo Added Files: pkgsrc/sysutils/xenkernel411/patches: patch-XSA286 patch-XSA345 patch-XSA346 patch-XSA347 Log Message: Add upstream security patches for XSA286, XSA345, XSA346, XSA347. Bump PKGREVISION. --- Module Name: pkgsrc Committed By: bouyer Date: Wed Oct 21 09:04:10 UTC 2020 Modified Files: pkgsrc/sysutils/xenkernel413: Makefile distinfo Added Files: pkgsrc/sysutils/xenkernel413/patches: patch-XSA286 patch-XSA345 patch-XSA346 patch-XSA347 Log Message: Add upstream security patches for XSA286, XSA345, XSA346, XSA347. Bump PKGREVISION. @ text @a0 282 $NetBSD: patch-XSA347,v 1.1 2020/10/21 09:04:10 bouyer Exp $ From: Jan Beulich Subject: AMD/IOMMU: convert amd_iommu_pte from struct to union This is to add a "raw" counterpart to the bitfield equivalent. Take the opportunity and - convert fields to bool / unsigned int, - drop the naming of the reserved field, - shorten the names of the ignored ones. This is part of XSA-347. Signed-off-by: Jan Beulich Reviewed-by: Andrew Cooper Reviewed-by: Paul Durrant --- xen/drivers/passthrough/amd/iommu_map.c.orig +++ xen/drivers/passthrough/amd/iommu_map.c @@@@ -38,7 +38,7 @@@@ static unsigned int pfn_to_pde_idx(unsig static unsigned int clear_iommu_pte_present(unsigned long l1_mfn, unsigned long dfn) { - struct amd_iommu_pte *table, *pte; + union amd_iommu_pte *table, *pte; unsigned int flush_flags; table = map_domain_page(_mfn(l1_mfn)); @@@@ -52,7 +52,7 @@@@ static unsigned int clear_iommu_pte_pres return flush_flags; } -static unsigned int set_iommu_pde_present(struct amd_iommu_pte *pte, +static unsigned int set_iommu_pde_present(union amd_iommu_pte *pte, unsigned long next_mfn, unsigned int next_level, bool iw, bool ir) @@@@ -87,7 +87,7 @@@@ static unsigned int set_iommu_pte_presen int pde_level, bool iw, bool ir) { - struct amd_iommu_pte *table, *pde; + union amd_iommu_pte *table, *pde; unsigned int flush_flags; table = map_domain_page(_mfn(pt_mfn)); @@@@ -178,7 +178,7 @@@@ void iommu_dte_set_guest_cr3(struct amd_ static int iommu_pde_from_dfn(struct domain *d, unsigned long dfn, unsigned long pt_mfn[], bool map) { - struct amd_iommu_pte *pde, *next_table_vaddr; + union amd_iommu_pte *pde, *next_table_vaddr; unsigned long next_table_mfn; unsigned int level; struct page_info *table; @@@@ -458,7 +458,7 @@@@ int __init amd_iommu_quarantine_init(str unsigned long end_gfn = 1ul << (DEFAULT_DOMAIN_ADDRESS_WIDTH - PAGE_SHIFT); unsigned int level = amd_iommu_get_paging_mode(end_gfn); - struct amd_iommu_pte *table; + union amd_iommu_pte *table; if ( hd->arch.root_table ) { @@@@ -489,7 +489,7 @@@@ int __init amd_iommu_quarantine_init(str for ( i = 0; i < PTE_PER_TABLE_SIZE; i++ ) { - struct amd_iommu_pte *pde = &table[i]; + union amd_iommu_pte *pde = &table[i]; /* * PDEs are essentially a subset of PTEs, so this function --- xen/drivers/passthrough/amd/pci_amd_iommu.c.orig +++ xen/drivers/passthrough/amd/pci_amd_iommu.c @@@@ -390,7 +390,7 @@@@ static void deallocate_next_page_table(s static void deallocate_page_table(struct page_info *pg) { - struct amd_iommu_pte *table_vaddr; + union amd_iommu_pte *table_vaddr; unsigned int index, level = PFN_ORDER(pg); PFN_ORDER(pg) = 0; @@@@ -405,7 +405,7 @@@@ static void deallocate_page_table(struct for ( index = 0; index < PTE_PER_TABLE_SIZE; index++ ) { - struct amd_iommu_pte *pde = &table_vaddr[index]; + union amd_iommu_pte *pde = &table_vaddr[index]; if ( pde->mfn && pde->next_level && pde->pr ) { @@@@ -557,7 +557,7 @@@@ static void amd_dump_p2m_table_level(str paddr_t gpa, int indent) { paddr_t address; - struct amd_iommu_pte *table_vaddr; + const union amd_iommu_pte *table_vaddr; int index; if ( level < 1 ) @@@@ -573,7 +573,7 @@@@ static void amd_dump_p2m_table_level(str for ( index = 0; index < PTE_PER_TABLE_SIZE; index++ ) { - struct amd_iommu_pte *pde = &table_vaddr[index]; + const union amd_iommu_pte *pde = &table_vaddr[index]; if ( !(index % 2) ) process_pending_softirqs(); --- xen/include/asm-x86/hvm/svm/amd-iommu-defs.h.orig +++ xen/include/asm-x86/hvm/svm/amd-iommu-defs.h @@@@ -465,20 +465,23 @@@@ union amd_iommu_x2apic_control { #define IOMMU_PAGE_TABLE_U32_PER_ENTRY (IOMMU_PAGE_TABLE_ENTRY_SIZE / 4) #define IOMMU_PAGE_TABLE_ALIGNMENT 4096 -struct amd_iommu_pte { - uint64_t pr:1; - uint64_t ignored0:4; - uint64_t a:1; - uint64_t d:1; - uint64_t ignored1:2; - uint64_t next_level:3; - uint64_t mfn:40; - uint64_t reserved:7; - uint64_t u:1; - uint64_t fc:1; - uint64_t ir:1; - uint64_t iw:1; - uint64_t ignored2:1; +union amd_iommu_pte { + uint64_t raw; + struct { + bool pr:1; + unsigned int ign0:4; + bool a:1; + bool d:1; + unsigned int ign1:2; + unsigned int next_level:3; + uint64_t mfn:40; + unsigned int :7; + bool u:1; + bool fc:1; + bool ir:1; + bool iw:1; + unsigned int ign2:1; + }; }; /* Paging modes */ From: Jan Beulich Subject: AMD/IOMMU: update live PTEs atomically Updating a live PTE bitfield by bitfield risks the compiler re-ordering the individual updates as well as splitting individual updates into multiple memory writes. Construct the new entry fully in a local variable, do the check to determine the flushing needs on the thus established new entry, and then write the new entry by a single insn. Similarly using memset() to clear a PTE is unsafe, as the order of writes the function does is, at least in principle, undefined. This is part of XSA-347. Signed-off-by: Jan Beulich Reviewed-by: Paul Durrant --- xen/drivers/passthrough/amd/iommu_map.c.orig +++ xen/drivers/passthrough/amd/iommu_map.c @@@@ -45,7 +45,7 @@@@ static unsigned int clear_iommu_pte_pres pte = &table[pfn_to_pde_idx(dfn, 1)]; flush_flags = pte->pr ? IOMMU_FLUSHF_modified : 0; - memset(pte, 0, sizeof(*pte)); + write_atomic(&pte->raw, 0); unmap_domain_page(table); @@@@ -57,26 +57,30 @@@@ static unsigned int set_iommu_pde_presen unsigned int next_level, bool iw, bool ir) { + union amd_iommu_pte new = {}, old; unsigned int flush_flags = IOMMU_FLUSHF_added; - if ( pte->pr && - (pte->mfn != next_mfn || - pte->iw != iw || - pte->ir != ir || - pte->next_level != next_level) ) - flush_flags |= IOMMU_FLUSHF_modified; - /* * FC bit should be enabled in PTE, this helps to solve potential * issues with ATS devices */ - pte->fc = !next_level; + new.fc = !next_level; + + new.mfn = next_mfn; + new.iw = iw; + new.ir = ir; + new.next_level = next_level; + new.pr = true; + + old.raw = read_atomic(&pte->raw); + old.ign0 = 0; + old.ign1 = 0; + old.ign2 = 0; + + if ( old.pr && old.raw != new.raw ) + flush_flags |= IOMMU_FLUSHF_modified; - pte->mfn = next_mfn; - pte->iw = iw; - pte->ir = ir; - pte->next_level = next_level; - pte->pr = 1; + write_atomic(&pte->raw, new.raw); return flush_flags; } From: Jan Beulich Subject: AMD/IOMMU: ensure suitable ordering of DTE modifications DMA and interrupt translation should be enabled only after other applicable DTE fields have been written. Similarly when disabling translation or when moving a device between domains, translation should first be disabled, before other entry fields get modified. Note however that the "moving" aspect doesn't apply to the interrupt remapping side, as domain specifics are maintained in the IRTEs here, not the DTE. We also never disable interrupt remapping once it got enabled for a device (the respective argument passed is always the immutable iommu_intremap). This is part of XSA-347. Signed-off-by: Jan Beulich Reviewed-by: Paul Durrant --- xen/drivers/passthrough/amd/iommu_map.c.orig +++ xen/drivers/passthrough/amd/iommu_map.c @@@@ -107,11 +107,18 @@@@ void amd_iommu_set_root_page_table(struc uint64_t root_ptr, uint16_t domain_id, uint8_t paging_mode, bool valid) { + if ( valid || dte->v ) + { + dte->tv = false; + dte->v = true; + smp_wmb(); + } dte->domain_id = domain_id; dte->pt_root = paddr_to_pfn(root_ptr); dte->iw = true; dte->ir = true; dte->paging_mode = paging_mode; + smp_wmb(); dte->tv = true; dte->v = valid; } @@@@ -134,6 +141,7 @@@@ void amd_iommu_set_intremap_table( } dte->ig = false; /* unmapped interrupts result in i/o page faults */ + smp_wmb(); dte->iv = valid; } --- xen/drivers/passthrough/amd/pci_amd_iommu.c.orig +++ xen/drivers/passthrough/amd/pci_amd_iommu.c @@@@ -120,7 +120,10 @@@@ static void amd_iommu_setup_domain_devic /* Undo what amd_iommu_disable_domain_device() may have done. */ ivrs_dev = &get_ivrs_mappings(iommu->seg)[req_id]; if ( dte->it_root ) + { dte->int_ctl = IOMMU_DEV_TABLE_INT_CONTROL_TRANSLATED; + smp_wmb(); + } dte->iv = iommu_intremap; dte->ex = ivrs_dev->dte_allow_exclusion; dte->sys_mgt = MASK_EXTR(ivrs_dev->device_flags, ACPI_IVHD_SYSTEM_MGMT); @