On Fri, Apr 22, 2016 at 12:49 AM, Ingo Molnar <mi...@kernel.org> wrote: > > * Kees Cook <keesc...@chromium.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 <ying...@kernel.org> >> Suggested-by: Lasse Collin <lasse.col...@tukaani.org> >> Signed-off-by: Kees Cook <keesc...@chromium.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
