Hi Linus,
please pull a single urgent x86 fix for 6.3.
Thx.
---
The following changes since commit 09a9639e56c01c7a00d6c0ca63f4c7c41abe075d:
Linux 6.3-rc6 (2023-04-09 11:15:57 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git tags/x86_urgent_for_v6.3_rc7
for you to fetch changes up to 775d3c514c5b2763a50ab7839026d7561795924d:
x86/rtc: Remove __init for runtime functions (2023-04-13 14:41:04 +0200)
----------------------------------------------------------------
- Drop __init annotation from two rtc functions which get called after
boot is done, in order to prevent a crash
----------------------------------------------------------------
Matija Glavinic Pecotic (1):
x86/rtc: Remove __init for runtime functions
arch/x86/kernel/x86_init.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Sun, Apr 16, 2023 at 5:29 AM Borislav Petkov <[email protected]> wrote:
>
> - Drop __init annotation from two rtc functions which get called after
> boot is done, in order to prevent a crash
.. ahh, and our automation to catch these things didn't find it,
because they are only accessed from x86_wallclock_init(), which is
also __init.
So it all looked superficially good, except for the "oh, we saved them
into a data structure that _isn't_ init".
It would require automation much smarter than the one we have to catch
that kind of thing.
Linus
The pull request you sent on Sun, 16 Apr 2023 14:29:13 +0200:
> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git tags/x86_urgent_for_v6.3_rc7
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/e623175f6418f5873b28ca91a88312843b2e0dab
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html
On Sun, Apr 16, 2023 at 10:33:19AM -0700, Linus Torvalds wrote:
> On Sun, Apr 16, 2023 at 5:29 AM Borislav Petkov <[email protected]> wrote:
> >
> > - Drop __init annotation from two rtc functions which get called after
> > boot is done, in order to prevent a crash
>
> .. ahh, and our automation to catch these things didn't find it,
> because they are only accessed from x86_wallclock_init(), which is
> also __init.
>
> So it all looked superficially good, except for the "oh, we saved them
> into a data structure that _isn't_ init".
>
> It would require automation much smarter than the one we have to catch
> that kind of thing.
Looking at
objdump -D arch/x86/kernel/x86_init.o
output, all the info is there in order to check whether a function ptr from
a .init section gets assigned to .data section structure.
Question is whether this is worth to put into objtool. From all the
objtool code I've seen flying by in recent years, we should pretty much
have everything needed so it should relatively easy to do.
But lemme add Josh and Peter...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Mon, Apr 17, 2023 at 11:04:12AM +0200, Borislav Petkov wrote:
> On Sun, Apr 16, 2023 at 10:33:19AM -0700, Linus Torvalds wrote:
> > On Sun, Apr 16, 2023 at 5:29 AM Borislav Petkov <[email protected]> wrote:
> > >
> > > - Drop __init annotation from two rtc functions which get called after
> > > boot is done, in order to prevent a crash
> >
> > .. ahh, and our automation to catch these things didn't find it,
> > because they are only accessed from x86_wallclock_init(), which is
> > also __init.
> >
> > So it all looked superficially good, except for the "oh, we saved them
> > into a data structure that _isn't_ init".
> >
> > It would require automation much smarter than the one we have to catch
> > that kind of thing.
>
> Looking at
>
> objdump -D arch/x86/kernel/x86_init.o
>
> output, all the info is there in order to check whether a function ptr from
> a .init section gets assigned to .data section structure.
>
> Question is whether this is worth to put into objtool. From all the
> objtool code I've seen flying by in recent years, we should pretty much
> have everything needed so it should relatively easy to do.
>
> But lemme add Josh and Peter...
I implemented it in the patch below. It was pretty straightforward
since this case always(?) involves two relocations within a single
instruction, like:
3d9: 48 c7 05 00 00 00 00 00 00 00 00 movq $0x0,0x0(%rip) # 3e4 <x86_wallclock_init+0xa4> 3dc: R_X86_64_PC32 x86_platform+0x8
3e0: R_X86_64_32S .init.text+0x280
So objtool warns if the instruction's first relocation is non-init and
the second one is init.
Out of curiosity, I also added a few more init-related checks, as I'm
not sure what modpost's "section mismatch" does. Specifically I added
the following three warnings:
- "disallowed store of init symbol %s to non-init symbol %s"
- "disallowed reference to init symbol %s in non-init code"
- "disallowed reference to init symbol %s in non-init data %s at %s+0x%lx"
It spits out a lot of warnings on allyesconfig, though it's quite
possible many/most of them should be whitelisted as I haven't yet looked
at what modpost skips.
There are 9 "disallowed store" warnings. There would have been more,
but I added per-symbol rate-limiting to keep the total number of
warnings down. For example, instead of the x86_wallclock_init() issue
discovered above, it shows a similar warning from elsewhere:
vmlinux.o: warning: objtool: xen_init_apic+0xab: disallowed store of init symbol xen_apic_check to non-init data x86_platform
Anyway, this is kind of experimental. If any of these warnings don't
turn out to be useful I could drop some or all them.
vmlinux.o: warning: objtool: early_setup_idt+0x9: disallowed reference to init symbol vc_boot_ghcb in non-init code
vmlinux.o: warning: objtool: skip_addr+0x3f: disallowed reference to init symbol start_cpu0 in non-init code
vmlinux.o: warning: objtool: event_trigger_unlock_commit.constprop.0+0x20: disallowed reference to init symbol event_trace_file in non-init code
vmlinux.o: warning: objtool: bitmap_copy.constprop.0+0x13: disallowed reference to init symbol cp.25 in non-init code
vmlinux.o: warning: objtool: intel_pmu_init+0x896: disallowed store of init symbol __quirk.126 to non-init data x86_pmu
vmlinux.o: warning: objtool: xen_init_apic+0xab: disallowed store of init symbol xen_apic_check to non-init data x86_platform
vmlinux.o: warning: objtool: xen_start_kernel+0x2cc: disallowed store of init symbol xen_write_gdt_entry_boot to non-init data pv_ops
vmlinux.o: warning: objtool: xen_smp_init+0xc3: disallowed store of init symbol xen_smp_cpus_done to non-init data smp_ops
vmlinux.o: warning: objtool: set_cmdline_ftrace+0x99: disallowed store of init symbol bootup_tracer_buf to non-init data .bss
vmlinux.o: warning: objtool: kmem_cache_init+0x51: disallowed store of init symbol boot_kmem_cache_node.12 to non-init data __key.7
vmlinux.o: warning: objtool: debug_objects_selftest+0xdd: disallowed store of init symbol descr_type_test to non-init data obj_cache
vmlinux.o: warning: objtool: matroxfb_init_params+0x183: disallowed store of init symbol vesamap to non-init data __key.34
vmlinux.o: warning: objtool: early_console_setup+0xca: disallowed store of init symbol port_cfg to non-init data sci_ports
vmlinux.o: warning: objtool: disallowed reference to init symbol .head.text in non-init data .note.Xen at .note.Xen+0x10
vmlinux.o: warning: objtool: disallowed reference to init symbol btrfs_props_init in non-init data mod_init_seq at .rodata+0x701e80
vmlinux.o: warning: objtool: disallowed reference to init symbol imx_mu_imx7ulp_of_init in non-init data imx_mu_msi_irqchip_match_table at .rodata+0x7e3240
vmlinux.o: warning: objtool: disallowed reference to init symbol exynos3250_of_data in non-init data samsung_pinctrl_dt_match at .rodata+0x9f8600
vmlinux.o: warning: objtool: disallowed reference to init symbol s5pv210_wkup_irq_chip in non-init data exynos_wkup_irq_ids at .rodata+0x9f98a0
vmlinux.o: warning: objtool: disallowed reference to init symbol ids in non-init data __param_string_ids at .rodata+0xab5648
vmlinux.o: warning: objtool: disallowed reference to init symbol ks_pcie_host_init in non-init data ks_pcie_am654_host_ops at .rodata+0xad1800
vmlinux.o: warning: objtool: disallowed reference to init symbol num_slave_addrs in non-init data __param_arr_slave_addrs at .rodata+0xb42668
vmlinux.o: warning: objtool: disallowed reference to init symbol num_regshifts in non-init data __param_arr_regshifts at .rodata+0xb426a8
vmlinux.o: warning: objtool: disallowed reference to init symbol num_regsizes in non-init data __param_arr_regsizes at .rodata+0xb426e8
vmlinux.o: warning: objtool: disallowed reference to init symbol num_regspacings in non-init data __param_arr_regspacings at .rodata+0xb42728
vmlinux.o: warning: objtool: disallowed reference to init symbol num_irqs in non-init data __param_arr_irqs at .rodata+0xb42768
vmlinux.o: warning: objtool: disallowed reference to init symbol si_type_str in non-init data __param_string_type at .rodata+0xb42818
vmlinux.o: warning: objtool: disallowed reference to init symbol t2080_init_periph in non-init data chipinfo at .rodata+0xb86a28
vmlinux.o: warning: objtool: disallowed reference to init symbol nfi1x_parents in non-init data top_muxes at .rodata+0xbd9f90
vmlinux.o: warning: objtool: disallowed reference to init symbol infra_uart_parent in non-init data infra_muxes at .rodata+0xbdc0d0
vmlinux.o: warning: objtool: disallowed reference to init symbol sgmii1_clks in non-init data sgmii1_desc at .rodata+0xbdc6e0
vmlinux.o: warning: objtool: disallowed reference to init symbol sgmii0_clks in non-init data sgmii0_desc at .rodata+0xbdc760
vmlinux.o: warning: objtool: disallowed reference to init symbol eth_clks in non-init data eth_desc at .rodata+0xbdc7e0
vmlinux.o: warning: objtool: disallowed reference to init symbol nfi1x_parents in non-init data top_muxes at .rodata+0xbdd110
vmlinux.o: warning: objtool: disallowed reference to init symbol infra_uart_parent in non-init data infra_muxes at .rodata+0xbdeb10
vmlinux.o: warning: objtool: disallowed reference to init symbol ca53_parents in non-init data cpu_muxes at .rodata+0xbe16d0
vmlinux.o: warning: objtool: disallowed reference to init symbol r9a07g043_core_clks in non-init data r9a07g043_cpg_info at .rodata+0xc81020
vmlinux.o: warning: objtool: disallowed reference to init symbol core_clks in non-init data r9a07g054_cpg_info at .rodata+0xc81140
vmlinux.o: warning: objtool: disallowed reference to init symbol r9a07g044_crit_mod_clks in non-init data r9a07g044_cpg_info at .rodata+0xc811e8
vmlinux.o: warning: objtool: disallowed reference to init symbol r9a09g011_core_clks in non-init data r9a09g011_cpg_info at .rodata+0xc81aa0
vmlinux.o: warning: objtool: disallowed reference to init symbol r7s9210_cpg_mssr_info in non-init data cpg_mssr_match at .rodata+0xc83fe0
vmlinux.o: warning: objtool: disallowed reference to init symbol exynos5250_disp_gate_clks in non-init data exynos5250_disp_subcmu at .rodata+0xc88090
vmlinux.o: warning: objtool: disallowed reference to init symbol exynos5800_mau_gate_clks in non-init data exynos5800_mau_subcmu at .rodata+0xc88950
vmlinux.o: warning: objtool: disallowed reference to init symbol exynos5x_mscl_div_clks in non-init data exynos5x_mscl_subcmu at .rodata+0xc88980
vmlinux.o: warning: objtool: disallowed reference to init symbol exynos5x_mfc_div_clks in non-init data exynos5x_mfc_subcmu at .rodata+0xc889c0
vmlinux.o: warning: objtool: disallowed reference to init symbol exynos5x_g3d_gate_clks in non-init data exynos5x_g3d_subcmu at .rodata+0xc88a10
vmlinux.o: warning: objtool: disallowed reference to init symbol exynos5x_gsc_div_clks in non-init data exynos5x_gsc_subcmu at .rodata+0xc88a40
vmlinux.o: warning: objtool: disallowed reference to init symbol exynos5x_disp_div_clks in non-init data exynos5x_disp_subcmu at .rodata+0xc88a80
vmlinux.o: warning: objtool: disallowed reference to init symbol aud_cmu_info in non-init data exynos5433_cmu_of_match at .rodata+0xc88cc0
vmlinux.o: warning: objtool: disallowed reference to init symbol core_cmu_info in non-init data exynos7885_cmu_of_match at .rodata+0xc8a760
vmlinux.o: warning: objtool: disallowed reference to init symbol apm_cmu_info in non-init data exynos850_cmu_of_match at .rodata+0xc8a9c0
vmlinux.o: warning: objtool: disallowed reference to init symbol busmc_cmu_info in non-init data exynosautov9_cmu_of_match at .rodata+0xc8b0e0
vmlinux.o: warning: objtool: disallowed reference to init symbol peric_cmu_info in non-init data fsd_cmu_of_match at .rodata+0xc8bc00
vmlinux.o: warning: objtool: disallowed reference to init symbol imx8mm_soc_revision in non-init data imx8mp_soc_data at .rodata+0xcfce38
vmlinux.o: warning: objtool: disallowed reference to init symbol imx8mq_soc_revision in non-init data imx8mq_soc_data at .rodata+0xcfce68
vmlinux.o: warning: objtool: disallowed reference to init symbol i915_active_module_init in non-init data init_funcs at .rodata+0x108b4f8
vmlinux.o: warning: objtool: disallowed reference to init symbol ata_force_param_buf in non-init data __param_string_force at .rodata+0x1514a48
vmlinux.o: warning: objtool: disallowed reference to init symbol ubiblock_set_param in non-init data ubiblock_param_ops at .rodata+0x159da88
vmlinux.o: warning: objtool: disallowed reference to init symbol hisi_sfc_v3xx_dmi_quirk in non-init data hisi_sfc_v3xx_dmi_quirk_table at .rodata+0x15ad780
vmlinux.o: warning: objtool: disallowed reference to init symbol mtk_init in non-init data mtk_netdev_ops at .rodata+0x17dd548
vmlinux.o: warning: objtool: disallowed reference to init symbol netdev_init in non-init data netdev_ops at .rodata+0x1834128
vmlinux.o: warning: objtool: disallowed reference to init symbol fore200e_pca_prom_read in non-init data fore200e_pci_ops at .rodata+0x1d70cc8
vmlinux.o: warning: objtool: disallowed reference to init symbol ids in non-init data __param_string_ids at .rodata+0x1d8e218
vmlinux.o: warning: objtool: disallowed reference to init symbol init_common in non-init data ops_generic_v2 at .rodata+0x236cb30
vmlinux.o: warning: objtool: disallowed reference to init symbol init_8956 in non-init data ops_8956 at .rodata+0x236e4b0
vmlinux.o: warning: objtool: disallowed reference to init symbol init_8939 in non-init data ops_8939 at .rodata+0x236fed0
vmlinux.o: warning: objtool: disallowed reference to init symbol virtio_scmi_init in non-init data scmi_virtio_desc at .rodata+0x24e11a0
vmlinux.o: warning: objtool: disallowed reference to init symbol dmi_matched in non-init data huawei_quirks at .rodata+0x26655a0
vmlinux.o: warning: objtool: disallowed reference to init symbol set_ibm_param in non-init data __param_ops_fan at .rodata+0x26853e8
vmlinux.o: warning: objtool: disallowed reference to init symbol asus_t100_matched in non-init data inv_mpu_dev_list at .rodata+0x2801060
vmlinux.o: warning: objtool: disallowed reference to init symbol ima_appraise_cmdline_default in non-init data __param_ima_appraise at __param+0x4d28
vmlinux.o: warning: objtool: disallowed reference to init symbol rmem_cma_setup in non-init data __of_table_cma at __reservedmem_of_table+0xc0
vmlinux.o: warning: objtool: disallowed reference to init symbol al_fic_init_dt in non-init data __of_table_al_fic at __irqchip_of_table+0xc0
vmlinux.o: warning: objtool: disallowed reference to init symbol __fixed_factor_clk_of_clk_init_declare in non-init data __of_table_fixed_factor_clk at __clk_of_table+0xc0
vmlinux.o: warning: objtool: disallowed reference to init symbol xenboot_earlycon_setup in non-init data __UNIQUE_ID___earlycon_xenboot1165 at __earlycon_table+0x90
vmlinux.o: warning: objtool: disallowed reference to init symbol at91sam926x_pit_dt_init in non-init data __of_table_at91sam926x_pit at __timer_of_table+0xc0
vmlinux.o: warning: objtool: disallowed reference to init symbol need_page_owner in non-init data page_owner_ops at .data+0x8377d0
vmlinux.o: warning: objtool: disallowed reference to init symbol need_page_table_check in non-init data page_table_check_ops at .data+0x839bf0
vmlinux.o: warning: objtool: disallowed reference to init symbol trusted_tpm_init in non-init data trusted_key_tpm_ops at .data+0xa16b48
vmlinux.o: warning: objtool: disallowed reference to init symbol xenboot_console_setup in non-init data xenboot_console at .data+0xfb8650
vmlinux.o: warning: objtool: disallowed reference to init symbol lpc32xx_hsuart_console_setup in non-init data lpc32xx_hsuart_console at .data+0xfc1130
vmlinux.o: warning: objtool: disallowed reference to init symbol atmel_console_setup in non-init data atmel_console at .data+0xfc4e50
vmlinux.o: warning: objtool: disallowed reference to init symbol serial_omap_console_setup in non-init data serial_omap_console at .data+0xfc73f0
vmlinux.o: warning: objtool: disallowed reference to init symbol altera_uart_console_setup in non-init data altera_uart_console at .data+0xfc7790
vmlinux.o: warning: objtool: disallowed reference to init symbol altera_jtaguart_console_setup in non-init data altera_jtaguart_console at .data+0xfc81d0
vmlinux.o: warning: objtool: disallowed reference to init symbol vt8500_console_setup in non-init data vt8500_console at .data+0xfc8490
vmlinux.o: warning: objtool: disallowed reference to init symbol pch_console_setup in non-init data pch_console at .data+0xfc89b0
vmlinux.o: warning: objtool: disallowed reference to init symbol auart_console_setup in non-init data auart_console at .data+0xfc8e30
vmlinux.o: warning: objtool: disallowed reference to init symbol lqasc_console_setup in non-init data lqasc_console at .data+0xfc9150
vmlinux.o: warning: objtool: disallowed reference to init symbol lpuart_console_setup in non-init data lpuart32_console at .data+0xfca9d0
vmlinux.o: warning: objtool: disallowed reference to init symbol linflex_console_setup in non-init data linflex_console at .data+0xfcaeb0
vmlinux.o: warning: objtool: disallowed reference to init symbol mlb_usio_console_setup in non-init data mlb_usio_console at .data+0xfcd630
vmlinux.o: warning: objtool: disallowed reference to init symbol sifive_serial_console_setup in non-init data sifive_serial_console at .data+0xfcd950
vmlinux.o: warning: objtool: disallowed reference to init symbol sunplus_console_setup in non-init data sunplus_uart_console at .data+0xfcdf30
vmlinux.o: warning: objtool: disallowed reference to init symbol amd_iommu_prepare in non-init data amd_iommu_irq_ops at .data+0xfe2ba8
vmlinux.o: warning: objtool: disallowed reference to init symbol intel_prepare_irq_remapping in non-init data intel_irq_remap_ops at .data+0xfe6e08
vmlinux.o: warning: objtool: disallowed reference to init symbol hyperv_prepare_irq_remapping in non-init data hyperv_irq_remap_ops at .data+0xfedee8
vmlinux.o: warning: objtool: disallowed reference to init symbol __dtbo_overlay_base_begin in non-init data overlays at .data+0x1c6db60
vmlinux.o: warning: objtool: disallowed reference to init symbol uv_acpi_madt_oem_check in non-init data apic_x2apic_uv_x at .data..ro_after_init+0x18e30
vmlinux.o: warning: objtool: disallowed reference to init symbol qed_mfw_legacy_1g in non-init data qed_mfw_legacy_maps at .data..ro_after_init+0x1e998
vmlinux.o: warning: objtool: disallowed reference to init symbol qed_mfw_ext_1g in non-init data qed_mfw_ext_maps at .data..ro_after_init+0x1eab8
vmlinux.o: warning: objtool: disallowed reference to init symbol qede_forced_speed_1000 in non-init data qede_forced_speed_maps at .data..ro_after_init+0x1ebf8
vmlinux.o: warning: objtool: disallowed reference to init symbol __TRACE_SYSTEM_RET_PF_SPURIOUS in non-init data TRACE_SYSTEM_RET_PF_SPURIOUS at _ftrace_eval_map+0x0
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index f937be1afe65..e60879dcf72d 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -405,6 +405,20 @@ static int decode_instructions(struct objtool_file *file)
u8 prev_len = 0;
u8 idx = 0;
+ /*
+ * .init.text code is ran before userspace and thus doesn't
+ * strictly need retpolines, except for modules which are
+ * loaded late, they very much do need retpoline in their
+ * .init.text
+ */
+ if (!opts.module &&
+ (strstr(sec->name, ".init") || !strcmp(sec->name, ".head.text")))
+ sec->init = true;
+
+ if (strstr(sec->name, ".ref"))
+ sec->ref = true;
+
+
if (!(sec->sh.sh_flags & SHF_EXECINSTR))
continue;
@@ -419,15 +433,6 @@ static int decode_instructions(struct objtool_file *file)
!strncmp(sec->name, ".text.__x86.", 12))
sec->noinstr = true;
- /*
- * .init.text code is ran before userspace and thus doesn't
- * strictly need retpolines, except for modules which are
- * loaded late, they very much do need retpoline in their
- * .init.text
- */
- if (!strcmp(sec->name, ".init.text") && !opts.module)
- sec->init = true;
-
for (offset = 0; offset < sec->sh.sh_size; offset += insn->len) {
if (!insns || idx == INSN_CHUNK_MAX) {
insns = calloc(sizeof(*insn), INSN_CHUNK_SIZE);
@@ -4317,12 +4322,38 @@ static bool noendbr_range(struct objtool_file *file, struct instruction *insn)
return insn->offset == sym->offset + sym->len;
}
+static struct symbol *reloc_sym(struct reloc *reloc)
+{
+ struct symbol *sym = reloc->sym;
+ bool is_text = (sym->sec->sh.sh_flags & SHF_EXECINSTR);
+ unsigned long off;
+
+ if (sym->type != STT_SECTION)
+ return sym;
+
+ off = reloc->sym->offset;
+ if (reloc->type == R_X86_64_PC32 || reloc->type == R_X86_64_PLT32)
+ off += arch_dest_reloc_offset(reloc->addend);
+ else
+ off += reloc->addend;
+
+ if (is_text)
+ sym = find_func_containing(sym->sec, off);
+ else
+ sym = find_symbol_containing(sym->sec, off);
+
+ if (sym)
+ return sym;
+
+ return reloc->sym;
+}
+
static int validate_ibt_insn(struct objtool_file *file, struct instruction *insn)
{
struct instruction *dest;
- struct reloc *reloc;
+ struct reloc *reloc, *first_reloc;
unsigned long off;
- int warnings = 0;
+ int warnings = 0, i;
/*
* Looking for function pointer load relocations. Ignore
@@ -4342,11 +4373,38 @@ static int validate_ibt_insn(struct objtool_file *file, struct instruction *insn
break;
}
- for (reloc = insn_reloc(file, insn);
+ for (reloc = insn_reloc(file, insn), i = 0;
reloc;
reloc = find_reloc_by_dest_range(file->elf, insn->sec,
reloc->offset + 1,
- (insn->offset + insn->len) - (reloc->offset + 1))) {
+ (insn->offset + insn->len) - (reloc->offset + 1)), i++) {
+
+ if (!insn->sec->ref) {
+ if (i == 0)
+ first_reloc = reloc;
+
+ if (!insn->sec->init) {
+ if (reloc->sym->sec->init) {
+ struct symbol *rsym = reloc_sym(reloc);
+ if (!insn->sym->warned && !rsym->warned) {
+ WARN_FUNC("disallowed reference to init symbol %s in non-init code",
+ insn->sec, insn->offset, rsym->name);
+ insn->sym->warned = 1;
+ rsym->warned = 1;
+ }
+ }
+ } else if (i > 0) {
+ if (!first_reloc->sym->sec->init && reloc->sym->sec->init) {
+ struct symbol *first_rsym = reloc_sym(first_reloc);
+ if (!insn->sym->warned && !first_rsym->warned) {
+ WARN_FUNC("disallowed store of init symbol %s to non-init data %s",
+ insn->sec, insn->offset, reloc_sym(reloc)->name, first_rsym->name);
+ insn->sym->warned = 1;
+ first_rsym->warned = 1;
+ }
+ }
+ }
+ }
/*
* static_call_update() references the trampoline, which
@@ -4437,6 +4495,55 @@ static int validate_ibt_data_reloc(struct objtool_file *file,
return 1;
}
+static char *sym_name(struct section *sec, unsigned long offset)
+{
+ bool is_text = (sec->sh.sh_flags & SHF_EXECINSTR);
+ struct symbol *sym;
+
+ if (is_text)
+ sym = find_func_containing(sec, offset);
+ else
+ sym = find_symbol_containing(sec, offset);
+
+ if (sym)
+ return sym->name;
+
+ return sec->name;
+}
+
+static int validate_init_data_reloc(struct objtool_file *file, struct section *sec, struct reloc *reloc)
+{
+ struct symbol *sym, *rsym;
+
+ if (!sec->init && !sec->ref && reloc->sym->sec->init) {
+ if (sec->warned &&
+ strcmp(sec->name, ".data") &&
+ strcmp(sec->name, ".rodata") &&
+ strcmp(sec->name, ".data..ro_after_init"))
+ return 0;
+ sec->warned = 1;
+
+ sym = find_symbol_containing(sec, reloc->offset);
+ if (sym && sym->warned)
+ return 0;
+
+ rsym = reloc_sym(reloc);
+ if (rsym->warned)
+ return 0;
+
+ sec->warned = 1;
+ if (sym)
+ sym->warned = 1;
+ rsym->warned = 1;
+
+ WARN("disallowed reference to init symbol %s in non-init data %s at %s+0x%lx",
+ rsym->name, sym_name(sec, reloc->offset), sec->name, reloc->offset);
+ return 1;
+ }
+
+ return 0;
+}
+
/*
* Validate IBT rules and remove used ENDBR instructions from the seal list.
* Unused ENDBR instructions will be annotated for sealing (i.e., replaced with
@@ -4485,8 +4592,10 @@ static int validate_ibt(struct objtool_file *file)
strstr(sec->name, "__patchable_function_entries"))
continue;
- list_for_each_entry(reloc, &sec->reloc->reloc_list, list)
+ list_for_each_entry(reloc, &sec->reloc->reloc_list, list) {
warnings += validate_ibt_data_reloc(file, reloc);
+ warnings += validate_init_data_reloc(file, sec, reloc);
+ }
}
return warnings;
diff --git a/tools/objtool/include/objtool/elf.h b/tools/objtool/include/objtool/elf.h
index ad0024da262b..44daec92caa6 100644
--- a/tools/objtool/include/objtool/elf.h
+++ b/tools/objtool/include/objtool/elf.h
@@ -38,7 +38,7 @@ struct section {
Elf_Data *data;
char *name;
int idx;
- bool changed, text, rodata, noinstr, init, truncate;
+ bool changed, text, rodata, noinstr, init, ref, truncate, warned;
struct reloc *reloc_data;
};
@@ -61,6 +61,7 @@ struct symbol {
u8 return_thunk : 1;
u8 fentry : 1;
u8 profiling_func : 1;
+ u8 warned : 1;
struct list_head pv_target;
struct list_head reloc_list;
};
On Mon, Apr 17, 2023 at 06:24:35PM -0700, Josh Poimboeuf wrote:
> I implemented it in the patch below.
Ha, amazing! Thanks man. :-)
> It was pretty straightforward
> since this case always(?) involves two relocations within a single
> instruction, like:
>
> 3d9: 48 c7 05 00 00 00 00 00 00 00 00 movq $0x0,0x0(%rip) # 3e4 <x86_wallclock_init+0xa4> 3dc: R_X86_64_PC32 x86_platform+0x8
> 3e0: R_X86_64_32S .init.text+0x280
>
> So objtool warns if the instruction's first relocation is non-init and
> the second one is init.
Sounds plausible to me.
> There are 9 "disallowed store" warnings. There would have been more,
> but I added per-symbol rate-limiting to keep the total number of
> warnings down. For example, instead of the x86_wallclock_init() issue
> discovered above, it shows a similar warning from elsewhere:
>
> vmlinux.o: warning: objtool: xen_init_apic+0xab: disallowed store of init symbol xen_apic_check to non-init data x86_platform
Obviously a good catch.
However, the only call site is in apic_intr_mode_init() which itself is
__init. So yeah, strictly speaking nothing wrong.
*IF* something calls it later, when __init is gone, then boom.
Dunno, maybe
a) track call sites too but maybe expensive and too much
or
b) make those warnings debug messages in case someone wants to run objtool
in debug mode, feels really bored and wants to fix potential issues.
Or someone has a better idea.
> Anyway, this is kind of experimental. If any of these warnings don't
> turn out to be useful I could drop some or all them.
Right, I can certainly see potential and as said, since we're already
doing objtool massaging of object files, thought this should be pretty
easy to do. As you've shown. ;-)
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 19.04.23 17:59, Borislav Petkov wrote:
> On Mon, Apr 17, 2023 at 06:24:35PM -0700, Josh Poimboeuf wrote:
>> I implemented it in the patch below.
>
> Ha, amazing! Thanks man. :-)
>
>> It was pretty straightforward
>> since this case always(?) involves two relocations within a single
>> instruction, like:
>>
>> 3d9: 48 c7 05 00 00 00 00 00 00 00 00 movq $0x0,0x0(%rip) # 3e4 <x86_wallclock_init+0xa4> 3dc: R_X86_64_PC32 x86_platform+0x8
>> 3e0: R_X86_64_32S .init.text+0x280
>>
>> So objtool warns if the instruction's first relocation is non-init and
>> the second one is init.
>
> Sounds plausible to me.
>
>> There are 9 "disallowed store" warnings. There would have been more,
>> but I added per-symbol rate-limiting to keep the total number of
>> warnings down. For example, instead of the x86_wallclock_init() issue
>> discovered above, it shows a similar warning from elsewhere:
>>
>> vmlinux.o: warning: objtool: xen_init_apic+0xab: disallowed store of init symbol xen_apic_check to non-init data x86_platform
>
> Obviously a good catch.
>
> However, the only call site is in apic_intr_mode_init() which itself is
> __init. So yeah, strictly speaking nothing wrong.
>
> *IF* something calls it later, when __init is gone, then boom.
>
> Dunno, maybe
>
> a) track call sites too but maybe expensive and too much
>
> or
>
> b) make those warnings debug messages in case someone wants to run objtool
> in debug mode, feels really bored and wants to fix potential issues.
>
> Or someone has a better idea.
>
>> Anyway, this is kind of experimental. If any of these warnings don't
>> turn out to be useful I could drop some or all them.
>
> Right, I can certainly see potential and as said, since we're already
> doing objtool massaging of object files, thought this should be pretty
> easy to do. As you've shown. ;-)
What about splitting the function vectors into __init and non-init parts?
This would solve above problem automatically, as a non-init function couldn't
deref the __init part of x86_platform.
Juergen
On Wed, Apr 19, 2023 at 06:03:53PM +0200, Juergen Gross wrote:
> > However, the only call site is in apic_intr_mode_init() which itself is
> > __init. So yeah, strictly speaking nothing wrong.
> >
> > *IF* something calls it later, when __init is gone, then boom.
> >
> > Dunno, maybe
> >
> > a) track call sites too but maybe expensive and too much
That would indeed be a lot harder to do.
> > or
> >
> > b) make those warnings debug messages in case someone wants to run objtool
> > in debug mode, feels really bored and wants to fix potential issues.
Possibly, though I suspect nobody's ever going to be that bored ;-)
> > Or someone has a better idea.
> >
> > > Anyway, this is kind of experimental. If any of these warnings don't
> > > turn out to be useful I could drop some or all them.
> >
> > Right, I can certainly see potential and as said, since we're already
> > doing objtool massaging of object files, thought this should be pretty
> > easy to do. As you've shown. ;-)
>
> What about splitting the function vectors into __init and non-init parts?
>
> This would solve above problem automatically, as a non-init function couldn't
> deref the __init part of x86_platform.
Right, if we moved the __init parts of x86_platform out into a separate
struct x86_platform_init which is __initdata, that would solve this
particular issue rather cleanly.
Though, more generally, if we make the rule that non-init data can't
contain references to init data, that would be a much bigger patch set.
Which might end up being the reasonable thing to do, but before deciding
whether enforcing such a rule would be worth it, we might want to look
more deeply at the warnings to figure out what percentage of those (if
any) could be real bugs.
--
Josh