On Wed, Apr 30, 2014 at 9:19 AM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
