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

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

Message-ID: <CAGXu5jJLEi2pkdMg6xfQiu-SHJuUaftTC+vShm5+ZyqV7GQZgw@mail.gmail.com>
Date:	Thu, 28 Apr 2016 09:31:28 -0700
From:	Kees Cook <keescook@...omium.org>
To:	Ingo Molnar <mingo@...nel.org>
Cc:	Yinghai Lu <yinghai@...nel.org>, Baoquan He <bhe@...hat.com>,
	Borislav Petkov <bp@...e.de>, 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>,
	Andy Lutomirski <luto@...nel.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] x86/boot: Rename overlapping memcpy() to memmove()

On Thu, Apr 28, 2016 at 2:37 AM, Ingo Molnar <mingo@...nel.org> wrote:
>
> * Kees Cook <keescook@...omium.org> wrote:
>
>> On Thu, Apr 28, 2016 at 2:04 AM, Ingo Molnar <mingo@...nel.org> wrote:
>> >
>> > * Kees Cook <keescook@...omium.org> 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