Commit e32683c6f7d2 ("x86/mm: Fix RESERVE_BRK() for older binutils")
put the brk area into the .bss..brk section (placed directly behind
.bss), causing it not to be cleared initially. As the brk area is used
to allocate early page tables, these might contain garbage in not
explicitly written entries.
This is especially a problem for Xen PV guests, as the hypervisor will
validate page tables (check for writable page tables and hypervisor
private bits) before accepting them to be used. There have been reports
of early crashes of PV guests due to illegal page table contents.
Fix that by letting clear_bss() clear the brk area, too.
Fixes: e32683c6f7d2 ("x86/mm: Fix RESERVE_BRK() for older binutils")
Signed-off-by: Juergen Gross <[email protected]>
---
arch/x86/kernel/head64.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index e7e233209a8c..6a3cfaf6b72a 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -430,6 +430,8 @@ void __init clear_bss(void)
{
memset(__bss_start, 0,
(unsigned long) __bss_stop - (unsigned long) __bss_start);
+ memset(__brk_base, 0,
+ (unsigned long) __brk_limit - (unsigned long) __brk_base);
}
static unsigned long get_cmd_line_ptr(void)
--
2.35.3
Hi Juergen,
It helps to actually Cc the person who broke it ;-)
On Thu, Jun 23, 2022 at 11:46:07AM +0200, Juergen Gross wrote:
> Commit e32683c6f7d2 ("x86/mm: Fix RESERVE_BRK() for older binutils")
> put the brk area into the .bss..brk section (placed directly behind
> .bss),
Hm? It didn't actually do that.
For individual translation units, it did rename the section from
".brk_reservation" to ".bss..brk". But then during linking it's still
placed in .brk in vmlinux, just like before.
> causing it not to be cleared initially. As the brk area is used
> to allocate early page tables, these might contain garbage in not
> explicitly written entries.
>
> This is especially a problem for Xen PV guests, as the hypervisor will
> validate page tables (check for writable page tables and hypervisor
> private bits) before accepting them to be used. There have been reports
> of early crashes of PV guests due to illegal page table contents.
>
> Fix that by letting clear_bss() clear the brk area, too.
While it does make sense to clear the brk area, I don't understand how
my patch broke this. How was it getting cleared before?
--
Josh
On 29.06.22 19:14, Josh Poimboeuf wrote:
> Hi Juergen,
>
> It helps to actually Cc the person who broke it ;-)
>
> On Thu, Jun 23, 2022 at 11:46:07AM +0200, Juergen Gross wrote:
>> Commit e32683c6f7d2 ("x86/mm: Fix RESERVE_BRK() for older binutils")
>> put the brk area into the .bss..brk section (placed directly behind
>> .bss),
>
> Hm? It didn't actually do that.
>
> For individual translation units, it did rename the section from
> ".brk_reservation" to ".bss..brk". But then during linking it's still
> placed in .brk in vmlinux, just like before.
Sorry, I misread the patch commit message and was fooled by the fact that
bisection clearly pointed at this patch to have introduced the problem.
I only discovered later that the main issue was the added "NOLOAD"
attribute.
>> causing it not to be cleared initially. As the brk area is used
>> to allocate early page tables, these might contain garbage in not
>> explicitly written entries.
>>
>> This is especially a problem for Xen PV guests, as the hypervisor will
>> validate page tables (check for writable page tables and hypervisor
>> private bits) before accepting them to be used. There have been reports
>> of early crashes of PV guests due to illegal page table contents.
>>
>> Fix that by letting clear_bss() clear the brk area, too.
>
> While it does make sense to clear the brk area, I don't understand how
> my patch broke this. How was it getting cleared before?
It seemed to have worked by chance. The Xen hypervisor is clearing all
alloc-only sections when loading a kernel (this will "fix" the dom0
case reliably together with patch 3 of this series).
Grub might do the clearing, too (for the PV domU case), but I haven't
verified that by code inspection.
I'll drop the "Fixes:" tag.
Juergen