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

[Kees Cook]

On Fri, Apr 22, 2016 at 12:49 AM, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
> * Kees Cook <keescook@xxxxxxxxxxxx> 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@xxxxxxxxxx>
>> Suggested-by: Lasse Collin <lasse.collin@xxxxxxxxxxx>
>> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
>> ---
>>  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