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
| ||
|
Date: Wed, 24 May 2017 15:04:18 +0200 From: Borislav Petkov <bp@...e.de> To: Mateusz Jurczyk <mjurczyk@...gle.com> Cc: Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, "H. Peter Anvin" <hpa@...or.com>, Andy Lutomirski <luto@...nel.org>, x86@...nel.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH] x86: Prevent uninitialized stack byte read in apply_alternatives() On Wed, May 24, 2017 at 02:51:20PM +0200, Mateusz Jurczyk wrote: > In the current form of the code, if a->replacementlen is 0, the reference > to *insnbuf for comparison touches potentially garbage memory. While it > doesn't affect the execution flow due to the subsequent a->replacementlen > comparison, it is (rightly) detected as use of uninitialized memory by a > runtime instrumentation currently under my development, and could be > detected as such by other tools in the future, too (e.g. KMSAN). > > Fix the "false-positive" by reordering the conditions to first check the > replacement instruction length before referencing specific opcode bytes. > > Signed-off-by: Mateusz Jurczyk <mjurczyk@...gle.com> > --- > arch/x86/kernel/alternative.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c > index c5b8f760473c..d03ba6bc97d8 100644 > --- a/arch/x86/kernel/alternative.c > +++ b/arch/x86/kernel/alternative.c > @@ -410,7 +410,7 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start, > insnbuf_sz = a->replacementlen; > > /* 0xe8 is a relative jump; fix the offset. */ Sure but put in short the reason to look at the instruction length first in the comment above so that it is clear why we're doing it this way. > - if (*insnbuf == 0xe8 && a->replacementlen == 5) { > + if (a->replacementlen == 5 && *insnbuf == 0xe8) { > *(s32 *)(insnbuf + 1) += replacement - instr; > DPRINTK("Fix CALL offset: 0x%x, CALL 0x%lx", > *(s32 *)(insnbuf + 1), > -- Thanks. -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) --
Powered by blists - more mailing lists