[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAGXu5jJGQgDSueZeZfSAVoSJLPEybmE_7csQA8dK4T0GwDe-Dg@mail.gmail.com>
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: 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
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.
