linux-kernel - Re: [PATCH] x86/cmdline: Disable instrumentation of cmdline unconditionally

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200917160548.GA1877352@rani.riverdale.lan>
Date:   Thu, 17 Sep 2020 12:05:48 -0400
From:   Arvind Sankar <nivedita@...m.mit.edu>
To:     Borislav Petkov <bp@...en8.de>
Cc:     Arvind Sankar <nivedita@...m.mit.edu>, x86@...nel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] x86/cmdline: Disable instrumentation of cmdline
 unconditionally

On Thu, Sep 17, 2020 at 11:40:55AM +0200, Borislav Petkov wrote:
> On Sun, Sep 06, 2020 at 11:46:37AM -0400, Arvind Sankar wrote:
> > On 32-bit, cmdline_find_option_bool() is used before paging is enabled,
> > from check_loader_disabled_bsp() in the early microcode loader.
> > Instrumentation options that insert accesses to global data will likely
> > crash or corrupt memory at this point.
> 
> What is the use case here, can you trigger an actual crash?
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

Hm this is a bit embarassing.

I did have a crash and this patch fixed it, but it seems it was on a
branch where I was making changes to cmdline.c, which triggered clang to
use a jump table for cmdline_find_option_bool(). That was the cause of
the crash, and the reason this patch fixed it was because it enabled
-fno-jump-tables, rather than because it disabled instrumentation.

The instrumentation code does write data to random addresses, but
apparently that doesn't necessarily crash the system. This patch would
also be insufficient to fix it, since load_ucode_bsp() itself can have
instrumentation code in it.

Eg with GCOV_PROFILE_ALL enabled, the start of the function is:

	c2a7706a <load_ucode_bsp>:
	c2a7706a:       55                      push   %ebp
	c2a7706b:       83 05 c0 4d ba c2 01    addl   $0x1,0xc2ba4dc0
	c2a77072:       83 15 c4 4d ba c2 00    adcl   $0x0,0xc2ba4dc4
	c2a77079:       89 e5                   mov    %esp,%ebp

but when it's called from arch/x86/kernel/head_32.S, paging is disabled
and the code is executing out of physical addresses, so it's going to
read/write data from garbage addresses.

Anyway, please ignore this patch and sorry for the noise.

Thanks.