Re: [kernel-hardening] Re: Add overflow protection to kref

[Kees Cook]

On Fri, Feb 24, 2012 at 10:37 AM, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> On Fri, Feb 24, 2012 at 12:58:35PM -0500, David Windsor wrote:
>> <snip>
>>
>> >> Greg, I'm not sure why you're opposed to adding this checking...
>> >> it's pretty clear that buggy error paths that forget to do a put are
>> >> pretty common and will continue to be common in new code, and
>> >> making them harder to exploit seems pretty sane to me.
>> >>
>> >> What's the downside?
>> >
>> > The downside is that there has not even been a patch sent for any of
>> > this.  Combine that with a lack of understanding about reference
>> > counting and atomic_t usages in the kernel, and the whole thing is ripe
>> > for misunderstanding and confusion.
>> >
>> > greg k-h
>>
>> This approach to adding overflow protection to kref uses
>> atomic_add_unless to increment the refcounter only if it is not
>> already at INT_MAX.  This
>> leaks the internal representation of atomic_t, which is defined as an
>> int in linux/types.h, into kref.
>>
>> If we can agree on an approach to adding overflow protection, if it is
>> indeed desired, we can then discuss adding a Kconfig option and/or a
>> sysctl for this protection.
>>
>> Thanks,
>> David
>>
>>
>> Signed-off-by: David Windsor <dwindsor@xxxxxxxxx>
>> ---
>>  include/linux/kref.h |    6 +++++-
>>  1 files changed, 5 insertions(+), 1 deletions(-)
>>
>> diff --git a/include/linux/kref.h b/include/linux/kref.h
>> index 9c07dce..fc0756a 100644
>> --- a/include/linux/kref.h
>> +++ b/include/linux/kref.h
>> @@ -38,8 +38,12 @@ static inline void kref_init(struct kref *kref)
>>   */
>>  static inline void kref_get(struct kref *kref)
>>  {
>> +   int rc = 0;
>>     WARN_ON(!atomic_read(&kref->refcount));
>> -   atomic_inc(&kref->refcount);
>> +   smp_mb__before_atomic_inc();
>> +   rc = atomic_add_unless(&kref->refcount, 1, INT_MAX);
>> +   smp_mb__after_atomic_inc();
>> +   BUG_ON(!rc);
>
> So you are guaranteeing to crash a machine here if this fails?  And you
> were trying to say this is a "security" based fix?

This is the same principle as the stack protector. When something has
gone horribly wrong and cannot be sensibly recovered from, crash the
machine. Wrapping the refcount would cause all kinds of problems, so
that certainly seems worthy of a BUG().

-Kees

-- 
Kees Cook
ChromeOS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/