[PATCH 0/4] Build checks for "tmp" memory access after POST; resume fixes.
Memory allocated with malloc_tmp() (and its variants) is only valid during the POST phase. It can be tricky to find all users of "tmp" memory and failures from getting this wrong can be subtle. So, this series adds a mechanism to try and catch these cases. The idea is to flag global variables that point to "tmp" memory as only being reachable from "init" code. The build can then verify it. No change of this nature would be complete without uncovering existing errors. The S3 resume code was accessing invalid memory when it called the shadow ram functions and the smm init code. The first two patches fix this up. The remaining two patches add the build time check. -Kevin Kevin O'Connor (4): shadow: Don't use PCIDevices list in make_bios_readonly(). smm: Don't use PCIDevices list in smm_setup(). Add VARVERIFY32INIT attribute for variables only available during "init". Use VARVERIFY32INIT on global variables that point to "tmp" memory. src/boot.c | 4 ++-- src/paravirt.c | 1 + src/pci.c | 2 +- src/pmm.c | 6 +++-- src/romfile.c | 2 +- src/shadow.c | 34 +++++++++++----------------- src/smm.c | 66 +++++++++++++++++++++++++++++------------------------- src/types.h | 4 ++++ src/util.h | 1 + tools/layoutrom.py | 18 +++++++-------- 10 files changed, 71 insertions(+), 67 deletions(-) -- 1.7.11.7
The make_bios_readonly() call is invoked from resume. The PCIDevices list is only valid during POST. Cache the necessary PCI BDF ids so that PCIDevices isn't needed. Signed-off-by: Kevin O'Connor <kevin@koconnor.net> --- src/shadow.c | 34 +++++++++++++--------------------- 1 file changed, 13 insertions(+), 21 deletions(-) diff --git a/src/shadow.c b/src/shadow.c index e971fe9..967eb8e 100644 --- a/src/shadow.c +++ b/src/shadow.c @@ -97,23 +97,7 @@ make_bios_readonly_intel(u16 bdf, u32 pam0) pci_config_writeb(bdf, pam0, 0x10); } -static void i440fx_bios_make_readonly(struct pci_device *pci, void *arg) -{ - make_bios_readonly_intel(pci->bdf, I440FX_PAM0); -} - -void mch_bios_make_readonly(struct pci_device *pci, void *arg) -{ - make_bios_readonly_intel(pci->bdf, Q35_HOST_BRIDGE_PAM0); -} - -static const struct pci_device_id dram_controller_make_readonly_tbl[] = { - PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82441, - i440fx_bios_make_readonly), - PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_Q35_MCH, - mch_bios_make_readonly), - PCI_DEVICE_END -}; +static int ShadowBDF = -1; // Make the 0xc0000-0x100000 area read/writable. void @@ -133,11 +117,13 @@ make_bios_writable(void) if (vendor == PCI_VENDOR_ID_INTEL && device == PCI_DEVICE_ID_INTEL_82441) { make_bios_writable_intel(bdf, I440FX_PAM0); + ShadowBDF = bdf; return; } if (vendor == PCI_VENDOR_ID_INTEL && device == PCI_DEVICE_ID_INTEL_Q35_MCH) { make_bios_writable_intel(bdf, Q35_HOST_BRIDGE_PAM0); + ShadowBDF = bdf; return; } } @@ -150,12 +136,18 @@ make_bios_readonly(void) { if (!CONFIG_QEMU || runningOnXen()) return; - dprintf(3, "locking shadow ram\n"); - struct pci_device *pci = pci_find_init_device( - dram_controller_make_readonly_tbl, NULL); - if (!pci) + + if (ShadowBDF < 0) { dprintf(1, "Unable to lock ram - bridge not found\n"); + return; + } + + u16 device = pci_config_readw(ShadowBDF, PCI_DEVICE_ID); + if (device == PCI_DEVICE_ID_INTEL_82441) + make_bios_readonly_intel(ShadowBDF, I440FX_PAM0); + else + make_bios_readonly_intel(ShadowBDF, Q35_HOST_BRIDGE_PAM0); } void -- 1.7.11.7
The smm_setup() call is invoked from resume. The PCIDevices list is only valid during POST. Cache the necessary PCI BDF ids so that PCIDevices isn't needed. Signed-off-by: Kevin O'Connor <kevin@koconnor.net> --- src/paravirt.c | 1 + src/smm.c | 66 +++++++++++++++++++++++++++++++--------------------------- src/util.h | 1 + 3 files changed, 37 insertions(+), 31 deletions(-) diff --git a/src/paravirt.c b/src/paravirt.c index f061039..4a26c6e 100644 --- a/src/paravirt.c +++ b/src/paravirt.c @@ -106,6 +106,7 @@ qemu_platform_setup(void) // Initialize pci pci_setup(); + smm_device_setup(); smm_setup(); // Initialize mtrr and smp diff --git a/src/smm.c b/src/smm.c index 490a626..a424a29 100644 --- a/src/smm.c +++ b/src/smm.c @@ -6,6 +6,7 @@ // This file may be distributed under the terms of the GNU LGPLv3 license. #include "pci.h" // pci_config_writel +#include "pci_regs.h" // PCI_DEVICE_ID #include "util.h" // wbinvd #include "config.h" // CONFIG_* #include "ioport.h" // outb @@ -111,46 +112,30 @@ smm_relocate_and_restore(void) #define PIIX_APMC_EN (1 << 25) // This code is hardcoded for PIIX4 Power Management device. -static void piix4_apmc_smm_setup(struct pci_device *pci, void *arg) +static void piix4_apmc_smm_setup(int isabdf, int i440_bdf) { - struct pci_device *i440_pci = pci_find_device(PCI_VENDOR_ID_INTEL - , PCI_DEVICE_ID_INTEL_82441); - if (!i440_pci) - return; - /* check if SMM init is already done */ - u32 value = pci_config_readl(pci->bdf, PIIX_DEVACTB); + u32 value = pci_config_readl(isabdf, PIIX_DEVACTB); if (value & PIIX_APMC_EN) return; /* enable the SMM memory window */ - pci_config_writeb(i440_pci->bdf, I440FX_SMRAM, 0x02 | 0x48); + pci_config_writeb(i440_bdf, I440FX_SMRAM, 0x02 | 0x48); smm_save_and_copy(); /* enable SMI generation when writing to the APMC register */ - pci_config_writel(pci->bdf, PIIX_DEVACTB, value | PIIX_APMC_EN); + pci_config_writel(isabdf, PIIX_DEVACTB, value | PIIX_APMC_EN); smm_relocate_and_restore(); /* close the SMM memory window and enable normal SMM */ - pci_config_writeb(i440_pci->bdf, I440FX_SMRAM, 0x02 | 0x08); + pci_config_writeb(i440_bdf, I440FX_SMRAM, 0x02 | 0x08); } /* PCI_VENDOR_ID_INTEL && PCI_DEVICE_ID_INTEL_ICH9_LPC */ -void ich9_lpc_apmc_smm_setup(struct pci_device *dev, void *arg) +void ich9_lpc_apmc_smm_setup(int isabdf, int mch_bdf) { - struct pci_device *mch_dev; - int mch_bdf; - - // This code is hardcoded for Q35 Power Management device. - mch_dev = pci_find_device(PCI_VENDOR_ID_INTEL, - PCI_DEVICE_ID_INTEL_Q35_MCH); - mch_bdf = mch_dev->bdf; - - if (mch_bdf < 0) - return; - /* check if SMM init is already done */ u32 value = inl(PORT_ACPI_PM_BASE + ICH9_PMIO_SMI_EN); if (value & ICH9_PMIO_SMI_EN_APMC_EN) @@ -171,21 +156,40 @@ void ich9_lpc_apmc_smm_setup(struct pci_device *dev, void *arg) pci_config_writeb(mch_bdf, Q35_HOST_BRIDGE_SMRAM, 0x02 | 0x08); } -static const struct pci_device_id smm_init_tbl[] = { - PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371AB_3, - piix4_apmc_smm_setup), - PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH9_LPC, - ich9_lpc_apmc_smm_setup), +static int SMMISADeviceBDF = -1, SMMPMDeviceBDF = -1; + +void +smm_device_setup(void) +{ + if (!CONFIG_USE_SMM) + return; - PCI_DEVICE_END, -}; + struct pci_device *isapci, *pmpci; + isapci = pci_find_device(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371AB_3); + pmpci = pci_find_device(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82441); + if (isapci && pmpci) { + SMMISADeviceBDF = isapci->bdf; + SMMPMDeviceBDF = pmpci->bdf; + return; + } + isapci = pci_find_device(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH9_LPC); + pmpci = pci_find_device(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_Q35_MCH); + if (isapci && pmpci) { + SMMISADeviceBDF = isapci->bdf; + SMMPMDeviceBDF = pmpci->bdf; + } +} void smm_setup(void) { - if (!CONFIG_USE_SMM) + if (!CONFIG_USE_SMM || SMMISADeviceBDF < 0) return; dprintf(3, "init smm\n"); - pci_find_init_device(smm_init_tbl, NULL); + u16 device = pci_config_readw(SMMISADeviceBDF, PCI_DEVICE_ID); + if (device == PCI_DEVICE_ID_INTEL_82371AB_3) + piix4_apmc_smm_setup(SMMISADeviceBDF, SMMPMDeviceBDF); + else + ich9_lpc_apmc_smm_setup(SMMISADeviceBDF, SMMPMDeviceBDF); } diff --git a/src/util.h b/src/util.h index 306a8bf..af029fc 100644 --- a/src/util.h +++ b/src/util.h @@ -314,6 +314,7 @@ extern const u8 pci_irqs[4]; void pci_setup(void); // smm.c +void smm_device_setup(void); void smm_setup(void); // smp.c -- 1.7.11.7
Add a build check to verify certain variables are only reachable via the 32bit "init" code. This can be used as a mechanism to enforce certain data (and code that accesses that data) as only available during POST. Signed-off-by: Kevin O'Connor <kevin@koconnor.net> --- src/types.h | 4 ++++ tools/layoutrom.py | 18 +++++++++--------- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/types.h b/src/types.h index 03104a2..84582ac 100644 --- a/src/types.h +++ b/src/types.h @@ -59,6 +59,8 @@ extern void __force_link_error__only_in_16bit(void) __noreturn; # define VARLOW __section(".discard.varlow." UNIQSEC) __VISIBLE __weak // Designate a variable as visible and located in the f-segment. # define VARFSEG __section(".discard.varfseg." UNIQSEC) __VISIBLE __weak +// Verify a variable is only accessable via 32bit "init" functions +# define VARVERIFY32INIT __section(".discard.varinit." UNIQSEC) // Designate top-level assembler as 16bit only. # define ASM16(code) __ASM(code) // Designate top-level assembler as 32bit flat only. @@ -77,6 +79,7 @@ extern void __force_link_error__only_in_16bit(void) __noreturn; # define VAR32SEG __section(".data32seg." UNIQSEC) # define VARLOW __section(".discard.varlow." UNIQSEC) __VISIBLE __weak # define VARFSEG __section(".discard.varfseg." UNIQSEC) __VISIBLE __weak +# define VARVERIFY32INIT __section(".discard.varinit." UNIQSEC) # define ASM16(code) # define ASM32FLAT(code) # define ASSERT16() __force_link_error__only_in_16bit() @@ -92,6 +95,7 @@ extern void __force_link_error__only_in_16bit(void) __noreturn; # define VAR32SEG __section(".discard.var32seg." UNIQSEC) # define VARLOW __section(".data.varlow." UNIQSEC) __VISIBLE __weak # define VARFSEG __section(".data.varfseg." UNIQSEC) __VISIBLE +# define VARVERIFY32INIT __section(".data.varinit." UNIQSEC) # define ASM16(code) # define ASM32FLAT(code) __ASM(code) # define ASSERT16() __force_link_error__only_in_16bit() diff --git a/tools/layoutrom.py b/tools/layoutrom.py index e8b32a7..20b9716 100755 --- a/tools/layoutrom.py +++ b/tools/layoutrom.py @@ -432,16 +432,20 @@ PHDRS # Detection of init code ###################################################################### -def markRuntime(section, sections): +def markRuntime(section, sections, chain=[]): if (section is None or not section.keep or section.category is not None or '.init.' in section.name or section.fileid != '32flat'): return + if '.data.varinit.' in section.name: + print "ERROR: %s is VARVERIFY32INIT but used from %s" % ( + section.name, chain) + sys.exit(1) section.category = '32flat' # Recursively mark all sections this section points to for reloc in section.relocs: - markRuntime(reloc.symbol.section, sections) + markRuntime(reloc.symbol.section, sections, chain + [section.name]) -def findInit(sections): +def findInit(sections, genreloc): # Recursively find and mark all "runtime" sections. for section in sections: if ('.data.varlow.' in section.name or '.data.varfseg.' in section.name @@ -450,7 +454,7 @@ def findInit(sections): for section in sections: if section.category is not None: continue - if section.fileid == '32flat': + if section.fileid == '32flat' and genreloc: section.category = '32init' else: section.category = section.fileid @@ -635,11 +639,7 @@ def main(): # Separate 32bit flat into runtime and init parts genreloc = '_reloc_abs_start' in info32flat[1] - if genreloc: - findInit(sections) - else: - for section in sections: - section.category = section.fileid + findInit(sections, genreloc) # Note "low memory" and "fseg memory" parts for section in getSectionsPrefix(sections, '.data.varlow.'): -- 1.7.11.7
Enable the recently added build check on global variables that are (or contain) pointers to memory allocated by malloc_tmp(). This helps detect cases where temporary memory is accessed after POST. Signed-off-by: Kevin O'Connor <kevin@koconnor.net> --- src/boot.c | 4 ++-- src/pci.c | 2 +- src/pmm.c | 6 ++++-- src/romfile.c | 2 +- 4 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/boot.c b/src/boot.c index 3370c2d..c308602 100644 --- a/src/boot.c +++ b/src/boot.c @@ -21,7 +21,7 @@ * Boot priority ordering ****************************************************************/ -static char **Bootorder; +static char **Bootorder VARVERIFY32INIT; static int BootorderCount; static void @@ -293,7 +293,7 @@ struct bootentry_s { const char *description; struct bootentry_s *next; }; -static struct bootentry_s *BootList; +static struct bootentry_s *BootList VARVERIFY32INIT; #define IPL_TYPE_FLOPPY 0x01 #define IPL_TYPE_HARDDISK 0x02 diff --git a/src/pci.c b/src/pci.c index 4f6a6fa..a92fb9b 100644 --- a/src/pci.c +++ b/src/pci.c @@ -85,7 +85,7 @@ pci_next(int bdf, int bus) } } -struct pci_device *PCIDevices; +struct pci_device *PCIDevices VARVERIFY32INIT; int MaxPCIBus VARFSEG; // Check if PCI is available at all diff --git a/src/pmm.c b/src/pmm.c index f02cf62..c4156ba 100644 --- a/src/pmm.c +++ b/src/pmm.c @@ -29,9 +29,11 @@ struct zone_s { struct allocinfo_s *info; }; -struct zone_s ZoneLow, ZoneHigh, ZoneFSeg, ZoneTmpLow, ZoneTmpHigh; +struct zone_s ZoneLow VARVERIFY32INIT, ZoneHigh VARVERIFY32INIT; +struct zone_s ZoneFSeg VARVERIFY32INIT; +struct zone_s ZoneTmpLow VARVERIFY32INIT, ZoneTmpHigh VARVERIFY32INIT; -static struct zone_s *Zones[] = { +static struct zone_s *Zones[] VARVERIFY32INIT = { &ZoneTmpLow, &ZoneLow, &ZoneFSeg, &ZoneTmpHigh, &ZoneHigh }; diff --git a/src/romfile.c b/src/romfile.c index 93ee3d6..ea71d1f 100644 --- a/src/romfile.c +++ b/src/romfile.c @@ -7,7 +7,7 @@ #include "config.h" // CONFIG_* #include "util.h" // dprintf -static struct romfile_s *RomfileRoot; +static struct romfile_s *RomfileRoot VARVERIFY32INIT; void romfile_add(struct romfile_s *file) -- 1.7.11.7
participants (1)
-
Kevin O'Connor