linux-kernel - Re: [PATCH 4/5] x86, boot: Make memcpy handle overlaps

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] [day] [month] [year] [list]

Message-ID: <CAGXu5jJDMF9JqR1U9L7Myfv+5i6eh5fgyB0wTCgMny689cr9qQ@mail.gmail.com>
Date:	Fri, 22 Apr 2016 15:18:07 -0700
From:	Kees Cook <keescook@...omium.org>
To:	Ingo Molnar <mingo@...nel.org>
Cc:	Yinghai Lu <yinghai@...nel.org>, Baoquan He <bhe@...hat.com>,
	Ingo Molnar <mingo@...hat.com>,
	"x86@...nel.org" <x86@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Andrey Ryabinin <aryabinin@...tuozzo.com>,
	Dmitry Vyukov <dvyukov@...gle.com>,
	"H.J. Lu" <hjl.tools@...il.com>,
	Josh Poimboeuf <jpoimboe@...hat.com>,
	Borislav Petkov <bp@...e.de>,
	Andy Lutomirski <luto@...nel.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/5] x86, boot: Make memcpy handle overlaps

On Fri, Apr 22, 2016 at 12:49 AM, Ingo Molnar <mingo@...nel.org> wrote:
>
> * Kees Cook <keescook@...omium.org> wrote:
>
>> Two uses of memcpy (screen scrolling and ELF parsing) were handling
>> overlapping memory areas. While there were no explicitly noticed bugs
>> here (yet), it is best to fix this so that the copying will always be
>> safe.
>>
>> Instead of making a new memmove function that might collide with other
>> memmove definitions in the decompressors, this just makes the compressed
>> boot's copy of memcpy overlap safe.
>>
>> Reported-by: Yinghai Lu <yinghai@...nel.org>
>> Suggested-by: Lasse Collin <lasse.collin@...aani.org>
>> Signed-off-by: Kees Cook <keescook@...omium.org>
>> ---
>>  arch/x86/boot/compressed/misc.c   |  4 +---
>>  arch/x86/boot/compressed/string.c | 22 ++++++++++++++++++++--
>>  2 files changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/boot/compressed/string.c b/arch/x86/boot/compressed/string.c
>> index 00e788be1db9..1e10e40f49dd 100644
>> --- a/arch/x86/boot/compressed/string.c
>> +++ b/arch/x86/boot/compressed/string.c
>> @@ -1,7 +1,7 @@
>>  #include "../string.c"
>>
>>  #ifdef CONFIG_X86_32
>
> I've applied this patch, but could you please also do another patch that adds a
> comment block to the top of this special version of compressed/string.c, which
> explains why this file exists and what its purpose is?

So... this isn't exactly clearly to me. I assume that something about
the builtin memcpy doesn't work during compressed boot, so
compressed/string.c needed to explicitly overload them. If that
matches your understanding, I can add a comment to that effect.

> Also:
>
> +/*
> + * This memcpy is overlap safe (i.e. it is memmove without conflicting
> + * with other definitions of memmove from the various decompressors.
> + */
> +void *memcpy(void *dest, const void *src, size_t n)
>
> I'd not name it memcpy() if its semantics are not the same as the regular kernel
> memcpy() - that will only cause confusion later on.
>
> I'd try to name it memmove() and would fix the memmove() hacks in decompressors:
>
>  lib/decompress_unxz.c:#ifndef memmove
>  lib/decompress_unxz.c:void *memmove(void *dest, const void *src, size_t size)
>  lib/decompress_unxz.c: * Since we need memmove anyway, would use it as memcpy too.
>  lib/decompress_unxz.c:# define memcpy memmove
>
> any strong reason this cannot be done?

Lasse asked for this too, but I'm going to avoid poking at the
decompressor code and just use the interface it already defines: the
"memmove" define.

> Some other decompressors seem to avoid memmove() intentionally:
>
>  lib/decompress_bunzip2.c:                *by 256 in any case, using memmove here would
>  lib/decompress_unlzo.c:          * of the buffer. This way memmove() isn't needed which
>  lib/decompress_unlzo.c:                  * Use a loop to avoid memmove() dependency.

Yeah, seems like it's not hard to add memmove! :)

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security