[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAGXu5j+G778YMcbiW2cdeZ++0MDcTiNVOnm5E5W86vfKKDvUjg@mail.gmail.com>
Date: Wed, 30 Apr 2014 09:44:26 -0700
From: Kees Cook <keescook@...omium.org>
To: Dan Carpenter <dan.carpenter@...cle.com>
Cc: LKML <linux-kernel@...r.kernel.org>, linux-acpi@...r.kernel.org,
devel@...ica.org,
"kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>, Dave Jones <davej@...hat.com>,
Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [patch] lib: check for strcpy() overflows to fixed length buffers
On Wed, Apr 30, 2014 at 9:19 AM, Dan Carpenter <dan.carpenter@...cle.com> wrote:
> On Wed, Apr 30, 2014 at 08:33:21AM -0700, Kees Cook wrote:
>> On Wed, Apr 30, 2014 at 8:08 AM, Dan Carpenter <dan.carpenter@...cle.com> wrote:
>> > +#if CONFIG_DEBUG_STRICT_SLOW_STRCPY_CHECKS
>> > +#define strcpy(dest, src) do { \
>> > + int len = __compiletime_size(dest); \
>> > + if (len > 1 && strlen(src) >= len) \
>> > + WARN_ONCE(1, "strcpy() overflow copying \"%s\"\n", src); \
>>
>> This should probably BUG. An overflowing strcpy shouldn't be allowed
>> to continue.
>
> I was worried about false positives.
Sure, good to be initially cautious but I think if this goes in, it should BUG.
> Speaking of false positives the STRICT checks on copy_from_user() have
> been disabled for a year now because of a three year old GCC bug. I
> wonder if the GCC people realize the security impact it has. See
> commit 2fb0815c9ee6 ('gcc4: disable __compiletime_object_size for GCC
> 4.6+') and http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48880
Yeah, lots of badness here. I'll see if I can find someone to look at
solutions for this.
>
>> > +config DEBUG_STRICT_SLOW_STRCPY_CHECKS
>> > + bool "Strict checks for strcpy() overflows"
>> > + depends on DEBUG_KERNEL
>> > + help
>> > + Enabling this option adds some extra sanity checks when strcpy() is
>> > + called(). This will slow down the kernel a bit.
>>
>> Isn't this an entirely compile-time check? I would expect it to be
>> entirely optimized by the compiler. In fact, could this be turned into
>> a build failure instead?
>
> No. The problem is when we don't know the size of the src string. Also
> if GCC is able to find the compile time size of both the src and
> dest string then Smatch and other static checkers are able to as well so
> I'm not very concerned about that case because we already catch them.
Ah, right, the source. But that shouldn't make it "slow". How about
naming this DEBUG_STRICT_STRCPY_SIZE_CHECKS or something? I can't
imagine the performance from adding a single compare to be bad. You
can even branch-hint it with "if (unlikely(...."
-Kees
--
Kees Cook
Chrome OS Security
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
