diff mbox series

[08/65] xen: Annotate fnptr targets from acpi_table_parse()

Message ID 20211126123446.32324-9-andrew.cooper3@citrix.com (mailing list archive)
State New, archived
Headers show
Series x86: Support for CET Indirect Branch Tracking | expand

Commit Message

Andrew Cooper Nov. 26, 2021, 12:33 p.m. UTC 
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Wei Liu <wl@xen.org>
CC: Julien Grall <julien@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/acpi/boot.c                 | 24 ++++++++++++------------
 xen/arch/x86/hvm/dom0_build.c            | 16 ++++++++--------
 xen/arch/x86/srat.c                      |  4 ++--
 xen/arch/x86/tboot.c                     |  2 +-
 xen/arch/x86/x86_64/acpi_mmcfg.c         |  2 +-
 xen/arch/x86/x86_64/mmconfig.h           |  2 +-
 xen/drivers/acpi/apei/hest.c             |  4 ++--
 xen/drivers/acpi/numa.c                  | 10 +++++-----
 xen/drivers/passthrough/amd/iommu_acpi.c |  9 +++++----
 xen/drivers/passthrough/pci.c            |  3 ++-
 xen/drivers/passthrough/vtd/dmar.c       |  2 +-
 xen/include/asm-x86/tboot.h              |  2 +-
 xen/include/xen/acpi.h                   |  2 +-
 13 files changed, 42 insertions(+), 40 deletions(-)

Comments

Jan Beulich Dec. 6, 2021, 8:36 a.m. UTC | #1 
On 26.11.2021 13:33, Andrew Cooper wrote:
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c

Elsewhere in this file we have

        rc = map ?   map_mmio_regions(d, _gfn(pfn), nr_pages, _mfn(pfn))
                 : unmap_mmio_regions(d, _gfn(pfn), nr_pages, _mfn(pfn));

which has been in this shape only as of commit e3b418ac4919
("x86/pvh-dom0: Remove unnecessary function pointer call from
modify_identity_mmio()"). Aren't we relying on the compiler not
transforming this back into the earlier

        rc = (map ? map_mmio_regions : unmap_mmio_regions)
             (d, _gfn(pfn), nr_pages, _mfn(pfn));

? And aren't we further relying on the compiler not transforming direct
calls into indirect ones for other reasons (I recall Microsoft's compiler
being pretty aggressive about this when the same function was called
more than once in close succession, it at least certain past versions)?
Is the widened effect of the annotation intended to also guarantee that
indirect calls will not be produced by the compiler for any reason when
the annotation is absent on a targeted function's declaration?

I've made an attempt at auditing our code for further similar constructs,
and I couldn't spot any. But the pattern isn't easy to grep for without
producing a very large result set, so there's still the chance that I may
have overlooked something.

Jan
Andrew Cooper Dec. 10, 2021, 2:44 p.m. UTC | #2 
On 06/12/2021 08:36, Jan Beulich wrote:
> On 26.11.2021 13:33, Andrew Cooper wrote:
>> --- a/xen/arch/x86/hvm/dom0_build.c
>> +++ b/xen/arch/x86/hvm/dom0_build.c
> Elsewhere in this file we have
>
>         rc = map ?   map_mmio_regions(d, _gfn(pfn), nr_pages, _mfn(pfn))
>                  : unmap_mmio_regions(d, _gfn(pfn), nr_pages, _mfn(pfn));
>
> which has been in this shape only as of commit e3b418ac4919
> ("x86/pvh-dom0: Remove unnecessary function pointer call from
> modify_identity_mmio()"). Aren't we relying on the compiler not
> transforming this back into the earlier
>
>         rc = (map ? map_mmio_regions : unmap_mmio_regions)
>              (d, _gfn(pfn), nr_pages, _mfn(pfn));
>
> ?

That old code was especially dumb even before retpoline.  See also the
damage caused by c/s 245a320ce2.

Yes, we are relying on the compiler not to do transformations behind our
backs, but it won't of its own accord.

>  And aren't we further relying on the compiler not transforming direct
> calls into indirect ones for other reasons (I recall Microsoft's compiler
> being pretty aggressive about this when the same function was called
> more than once in close succession, it at least certain past versions)?

That sounds like a broken compiler.

There are legal cases where a direct call has to turn into an indirect
one, and that's when we need to traverse more than disp32 distance.

But without going to a larger mcmodel, we'd get linker errors before
that becomes a problem, because R_X86_64_PLT32 relocations can't be
retrofitted into an indirect call at link time.

> Is the widened effect of the annotation intended to also guarantee that
> indirect calls will not be produced by the compiler for any reason when
> the annotation is absent on a targeted function's declaration?

That would be one for the clang and gcc developers.

I don't see a plausible problem here.

~Andrew
Jan Beulich Dec. 13, 2021, 7:46 a.m. UTC | #3 
On 10.12.2021 15:44, Andrew Cooper wrote:
> On 06/12/2021 08:36, Jan Beulich wrote:
>> On 26.11.2021 13:33, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/hvm/dom0_build.c
>>> +++ b/xen/arch/x86/hvm/dom0_build.c
>> Elsewhere in this file we have
>>
>>         rc = map ?   map_mmio_regions(d, _gfn(pfn), nr_pages, _mfn(pfn))
>>                  : unmap_mmio_regions(d, _gfn(pfn), nr_pages, _mfn(pfn));
>>
>> which has been in this shape only as of commit e3b418ac4919
>> ("x86/pvh-dom0: Remove unnecessary function pointer call from
>> modify_identity_mmio()"). Aren't we relying on the compiler not
>> transforming this back into the earlier
>>
>>         rc = (map ? map_mmio_regions : unmap_mmio_regions)
>>              (d, _gfn(pfn), nr_pages, _mfn(pfn));
>>
>> ?
> 
> That old code was especially dumb even before retpoline.  See also the
> damage caused by c/s 245a320ce2.

I must be lacking context here - what damage did that one cause again?
Or which subsequent fix of that damage am I overlooking when going
through the further commits on top of that one?

> Yes, we are relying on the compiler not to do transformations behind our
> backs, but it won't of its own accord.
> 
>>  And aren't we further relying on the compiler not transforming direct
>> calls into indirect ones for other reasons (I recall Microsoft's compiler
>> being pretty aggressive about this when the same function was called
>> more than once in close succession, it at least certain past versions)?
> 
> That sounds like a broken compiler.
> 
> There are legal cases where a direct call has to turn into an indirect
> one, and that's when we need to traverse more than disp32 distance.

Right, but that's certainly not happing anywhere in (relevant) practice
withing a single compiled binary.

> But without going to a larger mcmodel, we'd get linker errors before
> that becomes a problem, because R_X86_64_PLT32 relocations can't be
> retrofitted into an indirect call at link time.

I guess I don't see a connection to a PLT reloc: There wouldn't be any
if the compiler chose to make an indirect call out of a direct one. It
would be simple PC-relative relocations (generally coming from a RIP-
relative LEA) instead.

Jan

>> Is the widened effect of the annotation intended to also guarantee that
>> indirect calls will not be produced by the compiler for any reason when
>> the annotation is absent on a targeted function's declaration?
> 
> That would be one for the clang and gcc developers.
> 
> I don't see a plausible problem here.
> 
> ~Andrew
>
diff mbox series

Patch

diff --git a/xen/arch/x86/acpi/boot.c b/xen/arch/x86/acpi/boot.c
index 24702d041c9c..43953b385533 100644
--- a/xen/arch/x86/acpi/boot.c
+++ b/xen/arch/x86/acpi/boot.c
@@ -60,7 +60,7 @@  static u64 acpi_lapic_addr __initdata = APIC_DEFAULT_PHYS_BASE;
                               Boot-time Configuration
    -------------------------------------------------------------------------- */
 
-static int __init acpi_parse_madt(struct acpi_table_header *table)
+static int __init cf_check acpi_parse_madt(struct acpi_table_header *table)
 {
 	struct acpi_table_madt *madt =
 		container_of(table, struct acpi_table_madt, header);
@@ -77,7 +77,7 @@  static int __init acpi_parse_madt(struct acpi_table_header *table)
 	return 0;
 }
 
-static int __init
+static int __init cf_check
 acpi_parse_x2apic(struct acpi_subtable_header *header, const unsigned long end)
 {
 	struct acpi_madt_local_x2apic *processor =
@@ -133,7 +133,7 @@  acpi_parse_x2apic(struct acpi_subtable_header *header, const unsigned long end)
 	return 0;
 }
 
-static int __init
+static int __init cf_check
 acpi_parse_lapic(struct acpi_subtable_header * header, const unsigned long end)
 {
 	struct acpi_madt_local_apic *processor =
@@ -171,7 +171,7 @@  acpi_parse_lapic(struct acpi_subtable_header * header, const unsigned long end)
 	return 0;
 }
 
-static int __init
+static int __init cf_check
 acpi_parse_lapic_addr_ovr(struct acpi_subtable_header * header,
 			  const unsigned long end)
 {
@@ -187,7 +187,7 @@  acpi_parse_lapic_addr_ovr(struct acpi_subtable_header * header,
 	return 0;
 }
 
-static int __init
+static int __init cf_check
 acpi_parse_x2apic_nmi(struct acpi_subtable_header *header,
 		      const unsigned long end)
 {
@@ -206,7 +206,7 @@  acpi_parse_x2apic_nmi(struct acpi_subtable_header *header,
 	return 0;
 }
 
-static int __init
+static int __init cf_check
 acpi_parse_lapic_nmi(struct acpi_subtable_header * header, const unsigned long end)
 {
 	struct acpi_madt_local_apic_nmi *lapic_nmi =
@@ -223,7 +223,7 @@  acpi_parse_lapic_nmi(struct acpi_subtable_header * header, const unsigned long e
 	return 0;
 }
 
-static int __init
+static int __init cf_check
 acpi_parse_ioapic(struct acpi_subtable_header * header, const unsigned long end)
 {
 	struct acpi_madt_io_apic *ioapic =
@@ -240,7 +240,7 @@  acpi_parse_ioapic(struct acpi_subtable_header * header, const unsigned long end)
 	return 0;
 }
 
-static int __init
+static int __init cf_check
 acpi_parse_int_src_ovr(struct acpi_subtable_header * header,
 		       const unsigned long end)
 {
@@ -267,7 +267,7 @@  acpi_parse_int_src_ovr(struct acpi_subtable_header * header,
 	return 0;
 }
 
-static int __init
+static int __init cf_check
 acpi_parse_nmi_src(struct acpi_subtable_header * header, const unsigned long end)
 {
 	struct acpi_madt_nmi_source *nmi_src =
@@ -283,7 +283,7 @@  acpi_parse_nmi_src(struct acpi_subtable_header * header, const unsigned long end
 	return 0;
 }
 
-static int __init acpi_parse_hpet(struct acpi_table_header *table)
+static int __init cf_check acpi_parse_hpet(struct acpi_table_header *table)
 {
 	const struct acpi_table_hpet *hpet_tbl =
 		container_of(table, const struct acpi_table_hpet, header);
@@ -319,7 +319,7 @@  static int __init acpi_parse_hpet(struct acpi_table_header *table)
 	return 0;
 }
 
-static int __init acpi_invalidate_bgrt(struct acpi_table_header *table)
+static int __init cf_check acpi_invalidate_bgrt(struct acpi_table_header *table)
 {
 	struct acpi_table_bgrt *bgrt_tbl =
 		container_of(table, struct acpi_table_bgrt, header);
@@ -472,7 +472,7 @@  acpi_fadt_parse_sleep_info(const struct acpi_table_fadt *fadt)
 	       acpi_sinfo.wakeup_vector, acpi_sinfo.vector_width);
 }
 
-static int __init acpi_parse_fadt(struct acpi_table_header *table)
+static int __init cf_check acpi_parse_fadt(struct acpi_table_header *table)
 {
 	const struct acpi_table_fadt *fadt =
 		container_of(table, const struct acpi_table_fadt, header);
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index 43e1bf12488a..a3c47de3c7e5 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -737,15 +737,15 @@  static int __init pvh_setup_cpus(struct domain *d, paddr_t entry,
     return 0;
 }
 
-static int __init acpi_count_intr_ovr(struct acpi_subtable_header *header,
-                                     const unsigned long end)
+static int __init cf_check acpi_count_intr_ovr(
+    struct acpi_subtable_header *header, const unsigned long end)
 {
     acpi_intr_overrides++;
     return 0;
 }
 
-static int __init acpi_set_intr_ovr(struct acpi_subtable_header *header,
-                                    const unsigned long end)
+static int __init cf_check acpi_set_intr_ovr(
+    struct acpi_subtable_header *header, const unsigned long end)
 {
     const struct acpi_madt_interrupt_override *intr =
         container_of(header, struct acpi_madt_interrupt_override, header);
@@ -756,15 +756,15 @@  static int __init acpi_set_intr_ovr(struct acpi_subtable_header *header,
     return 0;
 }
 
-static int __init acpi_count_nmi_src(struct acpi_subtable_header *header,
-                                     const unsigned long end)
+static int __init cf_check acpi_count_nmi_src(
+    struct acpi_subtable_header *header, const unsigned long end)
 {
     acpi_nmi_sources++;
     return 0;
 }
 
-static int __init acpi_set_nmi_src(struct acpi_subtable_header *header,
-                                   const unsigned long end)
+static int __init cf_check acpi_set_nmi_src(
+    struct acpi_subtable_header *header, const unsigned long end)
 {
     const struct acpi_madt_nmi_source *src =
         container_of(header, struct acpi_madt_nmi_source, header);
diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
index 6b77b9820195..cfe24c7e781c 100644
--- a/xen/arch/x86/srat.c
+++ b/xen/arch/x86/srat.c
@@ -407,8 +407,8 @@  void __init acpi_numa_arch_fixup(void) {}
 
 static uint64_t __initdata srat_region_mask;
 
-static int __init srat_parse_region(struct acpi_subtable_header *header,
-				    const unsigned long end)
+static int __init cf_check srat_parse_region(
+    struct acpi_subtable_header *header, const unsigned long end)
 {
 	struct acpi_srat_mem_affinity *ma;
 
diff --git a/xen/arch/x86/tboot.c b/xen/arch/x86/tboot.c
index 529367ed8167..fe1abfdf08ff 100644
--- a/xen/arch/x86/tboot.c
+++ b/xen/arch/x86/tboot.c
@@ -450,7 +450,7 @@  int __init tboot_protect_mem_regions(void)
     return 1;
 }
 
-int __init tboot_parse_dmar_table(acpi_table_handler dmar_handler)
+int __init cf_check tboot_parse_dmar_table(acpi_table_handler dmar_handler)
 {
     int rc;
     uint64_t size;
diff --git a/xen/arch/x86/x86_64/acpi_mmcfg.c b/xen/arch/x86/x86_64/acpi_mmcfg.c
index 0db8f57abbed..2159c68189e4 100644
--- a/xen/arch/x86/x86_64/acpi_mmcfg.c
+++ b/xen/arch/x86/x86_64/acpi_mmcfg.c
@@ -68,7 +68,7 @@  static int __init acpi_mcfg_check_entry(struct acpi_table_mcfg *mcfg,
     return -EINVAL;
 }
 
-int __init acpi_parse_mcfg(struct acpi_table_header *header)
+int __init cf_check acpi_parse_mcfg(struct acpi_table_header *header)
 {
     struct acpi_table_mcfg *mcfg;
     unsigned long i;
diff --git a/xen/arch/x86/x86_64/mmconfig.h b/xen/arch/x86/x86_64/mmconfig.h
index 4d3b9fcbdd3c..433046be663a 100644
--- a/xen/arch/x86/x86_64/mmconfig.h
+++ b/xen/arch/x86/x86_64/mmconfig.h
@@ -76,7 +76,7 @@  static inline void mmio_config_writel(void __iomem *pos, u32 val)
 
 /* function prototypes */
 struct acpi_table_header;
-int acpi_parse_mcfg(struct acpi_table_header *header);
+int cf_check acpi_parse_mcfg(struct acpi_table_header *header);
 int pci_mmcfg_reserved(uint64_t address, unsigned int segment,
                        unsigned int start_bus, unsigned int end_bus,
                        unsigned int flags);
diff --git a/xen/drivers/acpi/apei/hest.c b/xen/drivers/acpi/apei/hest.c
index c5f3aaab7c4e..5881275d2f37 100644
--- a/xen/drivers/acpi/apei/hest.c
+++ b/xen/drivers/acpi/apei/hest.c
@@ -128,8 +128,8 @@  int apei_hest_parse(apei_hest_func_t func, void *data)
  * Check if firmware advertises firmware first mode. We need FF bit to be set
  * along with a set of MC banks which work in FF mode.
  */
-static int __init hest_parse_cmc(const struct acpi_hest_header *hest_hdr,
-				 void *data)
+static int __init cf_check hest_parse_cmc(
+	const struct acpi_hest_header *hest_hdr, void *data)
 {
 #ifdef CONFIG_X86_MCE
 	unsigned int i;
diff --git a/xen/drivers/acpi/numa.c b/xen/drivers/acpi/numa.c
index 85f891757c21..bc6e888234e4 100644
--- a/xen/drivers/acpi/numa.c
+++ b/xen/drivers/acpi/numa.c
@@ -112,14 +112,14 @@  void __init acpi_table_print_srat_entry(struct acpi_subtable_header * header)
 	}
 }
 
-static int __init acpi_parse_slit(struct acpi_table_header *table)
+static int __init cf_check acpi_parse_slit(struct acpi_table_header *table)
 {
 	acpi_numa_slit_init((struct acpi_table_slit *)table);
 
 	return 0;
 }
 
-static int __init
+static int __init cf_check
 acpi_parse_x2apic_affinity(struct acpi_subtable_header *header,
 			   const unsigned long end)
 {
@@ -138,7 +138,7 @@  acpi_parse_x2apic_affinity(struct acpi_subtable_header *header,
 	return 0;
 }
 
-static int __init
+static int __init cf_check
 acpi_parse_processor_affinity(struct acpi_subtable_header *header,
 			      const unsigned long end)
 {
@@ -156,7 +156,7 @@  acpi_parse_processor_affinity(struct acpi_subtable_header *header,
 	return 0;
 }
 
-static int __init
+static int __init cf_check
 acpi_parse_memory_affinity(struct acpi_subtable_header *header,
 			   const unsigned long end)
 {
@@ -174,7 +174,7 @@  acpi_parse_memory_affinity(struct acpi_subtable_header *header,
 	return 0;
 }
 
-int __init acpi_parse_srat(struct acpi_table_header *table)
+int __init cf_check acpi_parse_srat(struct acpi_table_header *table)
 {
 	if (!table)
 		return -EINVAL;
diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c b/xen/drivers/passthrough/amd/iommu_acpi.c
index 5ea227732821..3a7931458944 100644
--- a/xen/drivers/passthrough/amd/iommu_acpi.c
+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
@@ -1078,7 +1078,7 @@  static inline bool_t is_ivmd_block(u8 type)
             type == ACPI_IVRS_TYPE_MEMORY_IOMMU);
 }
 
-static int __init parse_ivrs_table(struct acpi_table_header *table)
+static int __init cf_check parse_ivrs_table(struct acpi_table_header *table)
 {
     const struct acpi_ivrs_header *ivrs_block;
     unsigned long length;
@@ -1170,7 +1170,7 @@  static int __init parse_ivrs_table(struct acpi_table_header *table)
     return error;
 }
 
-static int __init detect_iommu_acpi(struct acpi_table_header *table)
+static int __init cf_check detect_iommu_acpi(struct acpi_table_header *table)
 {
     const struct acpi_ivrs_header *ivrs_block;
     unsigned long length = sizeof(struct acpi_table_ivrs);
@@ -1264,7 +1264,8 @@  static int __init get_last_bdf_ivhd(
     return last_bdf;
 }
 
-static int __init get_last_bdf_acpi(struct acpi_table_header *table)
+static int __init cf_check cf_check get_last_bdf_acpi(
+    struct acpi_table_header *table)
 {
     const struct acpi_ivrs_header *ivrs_block;
     unsigned long length = sizeof(struct acpi_table_ivrs);
@@ -1306,7 +1307,7 @@  int __init amd_iommu_update_ivrs_mapping_acpi(void)
     return acpi_table_parse(ACPI_SIG_IVRS, parse_ivrs_table);
 }
 
-static int __init
+static int __init cf_check
 get_supported_ivhd_type(struct acpi_table_header *table)
 {
     size_t length = sizeof(struct acpi_table_ivrs);
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 036f5c2b1ffa..e75c82d11bd6 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1218,7 +1218,8 @@  static bool_t hest_source_is_pcie_aer(const struct acpi_hest_header *hest_hdr)
     return 0;
 }
 
-static int aer_hest_parse(const struct acpi_hest_header *hest_hdr, void *data)
+static int cf_check aer_hest_parse(
+    const struct acpi_hest_header *hest_hdr, void *data)
 {
     struct aer_hest_parse_info *info = data;
     const struct acpi_hest_aer_common *p;
diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
index b152f3da916b..b8e91f5be1ae 100644
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -767,7 +767,7 @@  acpi_parse_one_rhsa(struct acpi_dmar_header *header)
     return ret;
 }
 
-static int __init acpi_parse_dmar(struct acpi_table_header *table)
+static int __init cf_check acpi_parse_dmar(struct acpi_table_header *table)
 {
     struct acpi_table_dmar *dmar;
     struct acpi_dmar_header *entry_header;
diff --git a/xen/include/asm-x86/tboot.h b/xen/include/asm-x86/tboot.h
index bfeed1542fa3..818d5fa45132 100644
--- a/xen/include/asm-x86/tboot.h
+++ b/xen/include/asm-x86/tboot.h
@@ -124,7 +124,7 @@  void tboot_probe(void);
 void tboot_shutdown(uint32_t shutdown_type);
 int tboot_in_measured_env(void);
 int tboot_protect_mem_regions(void);
-int tboot_parse_dmar_table(acpi_table_handler dmar_handler);
+int cf_check tboot_parse_dmar_table(acpi_table_handler dmar_handler);
 int tboot_s3_resume(void);
 void tboot_s3_error(int error);
 int tboot_wake_ap(int apicid, unsigned long sipi_vec);
diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h
index 088c238a504a..c82d5367bfb5 100644
--- a/xen/include/xen/acpi.h
+++ b/xen/include/xen/acpi.h
@@ -90,7 +90,7 @@  struct acpi_subtable_header *acpi_table_get_entry_madt(enum acpi_madt_type id,
 int acpi_table_parse_madt(enum acpi_madt_type id, acpi_table_entry_handler handler, unsigned int max_entries);
 int acpi_table_parse_srat(int id, acpi_madt_entry_handler handler,
 	unsigned int max_entries);
-int acpi_parse_srat(struct acpi_table_header *);
+int cf_check acpi_parse_srat(struct acpi_table_header *);
 void acpi_table_print (struct acpi_table_header *header, unsigned long phys_addr);
 void acpi_table_print_madt_entry (struct acpi_subtable_header *madt);
 void acpi_table_print_srat_entry (struct acpi_subtable_header *srat);