Re: [PATCH] x86/idt: Make sure idt_table takes a whole page

[hpa@zytor.com]

On July 9, 2020 3:33:55 AM PDT, Joerg Roedel <joro@8bytes.org> wrote:
>From: Joerg Roedel <jroedel@suse.de>
>
>On x86-32 the idt_table with 256 entries needs only 2048 bytes. It is
>page-aligned, but the end of the .bss..page_aligned section is not
>guaranteed to be page-aligned.
>
>As a result, symbols from other .bss sections may end up on the same
>4k page as the idt_table, and will accidentially get mapped read-only
>during boot, causing unexpected page-faults when the kernel writes to
>them.
>
>Avoid this by making the idt_table 4kb in size even on x86-32. On
>x86-64 the idt_table is already 4kb large, so nothing changes there.
>
>Fixes: 3e77abda65b1c ("x86/idt: Consolidate idt functionality")
>Signed-off-by: Joerg Roedel <jroedel@suse.de>
>---
> arch/x86/kernel/idt.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
>diff --git a/arch/x86/kernel/idt.c b/arch/x86/kernel/idt.c
>index 0db21206f2f3..83e24f837127 100644
>--- a/arch/x86/kernel/idt.c
>+++ b/arch/x86/kernel/idt.c
>@@ -157,8 +157,13 @@ static const __initconst struct idt_data
>apic_idts[] = {
> #endif
> };
> 
>-/* Must be page-aligned because the real IDT is used in the cpu entry
>area */
>-static gate_desc idt_table[IDT_ENTRIES] __page_aligned_bss;
>+/*
>+ * Must be page-aligned because the real IDT is used in the cpu entry
>area.
>+ * Allocate more entries than needed so that idt_table takes a whole
>page, so it
>+ * is safe to map the idt_table read-only and into the user-space
>page-table.
>+ */
>+#define IDT_ENTRIES_ALLOCATED	(PAGE_SIZE / sizeof(gate_desc))
>+static gate_desc idt_table[IDT_ENTRIES_ALLOCATED] __page_aligned_bss;
> 
> struct desc_ptr idt_descr __ro_after_init = {
> 	.size		= IDT_TABLE_SIZE - 1,
>@@ -335,6 +340,9 @@ void __init idt_setup_apic_and_irq_gates(void)
> 	idt_map_in_cea();
> 	load_idt(&idt_descr);
> 
>+	BUILD_BUG_ON(IDT_ENTRIES_ALLOCATED < IDT_ENTRIES);
>+	BUILD_BUG_ON(sizeof(idt_table) != PAGE_SIZE);
>+
> 	/* Make the IDT table read only */
> 	set_memory_ro((unsigned long)&idt_table, 1);
> 

NAK... this isn't the right way to fix this and just really kicks the can down the road. The reason is that you aren't fixing the module that actually has a problem.

The Right Way[TM] is to figure out which module(s) lack the proper alignment for this section. A script using objdump -h or readelf -SW running over the .o files looking for alignment less than 2**12 should spot the modules that are missing the proper .balign directives.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.