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

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]

Date:	Fri, 24 Feb 2012 10:52:30 -0800
From:	Kees Cook <keescook@...omium.org>
To:	Greg KH <gregkh@...uxfoundation.org>
Cc:	David Windsor <dwindsor@...il.com>,
	Roland Dreier <roland@...estorage.com>,
	Djalal Harouni <tixxdz@...ndz.org>,
	Vasiliy Kulikov <segoon@...nwall.com>,
	kernel-hardening@...ts.openwall.com,
	Ubuntu security discussion <ubuntu-hardened@...ts.ubuntu.com>,
	linux-kernel@...r.kernel.org, pageexec@...email.hu,
	spender@...ecurity.net
Subject: Re: [kernel-hardening] Re: Add overflow protection to kref

On Fri, Feb 24, 2012 at 10:37 AM, Greg KH <gregkh@...uxfoundation.org> 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@...il.com>
>> ---
>>  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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/