Re: [PATCH v2] x86/boot: Rename overlapping memcpy() to memmove()

[Kees Cook]

On Thu, Apr 28, 2016 at 2:37 AM, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
> * Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
>> On Thu, Apr 28, 2016 at 2:04 AM, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>> >
>> > * Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>> >> +void *memcpy(void *dest, const void *src, size_t n)
>> >
>> > btw., if there's any doubt about other overlapping uses, we could add this to
>> > memcpy():
>> >
>> >   WARN_ON_ONCE(dest > src && dest-src < n);
>> >
>> > or so? Does printk() work so early on?
>>
>> It does not, but we could use either "error" or the new "warn". Should
>> we abort a boot in this case, or just warn about it? (Our
>> implementations of memcpy, fwiw, currently seem to support overlap, so
>> I would suggest warn.)
>
> Yeah, I'd definitely not try to crash the bootup for the user, but try to
> continue.

So, I can do this either with a macro to try to inline every caller
with meaningful reporting (__func__, __line__, etc), but I feel like
that is needless bloat to the stub, given the rare situation. I think
the best thing to do is just complain about it happening at all, and
we should be able to find the problem manually if it is ever reported.

For example, this is what I've got currently:

/* Detect and warn about potential overlaps. */
void *memcpy(void *dest, const void *src, size_t n)
{
        if (dest > src && dest - src < n)
                warn("Potentially unsafe overlapping memcpy detected!");
        return __memcpy(dest, src, n);
}

Does that seem okay? If so, I'll send the patch...

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security