Add overflow protection to kref

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
31 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Add overflow protection to kref

David Windsor
Hi,

We are attempting to add various grsecurity/PAX features to upstream
Ubuntu kernels.

The PAX folks added refcount overflow protection by inserting
architecture-specific code in the increment paths of atomic_t.  For
instance:

static inline void atomic_inc(atomic_t *v)
 {
        asm volatile(LOCK_PREFIX "incl %0\n"

#ifdef CONFIG_PAX_REFCOUNT
                     "jno 0f\n"
                     LOCK_PREFIX "decl %0\n"
                     "int $4\n0:\n"
                     _ASM_EXTABLE(0b, 0b)
#endif

                     : "+m" (v->counter));
}

There are two distinct classes of users we need to consider here:
those who use atomic_t for reference counters and those who use
atomic_t for keeping track of statistics, like performance counters,
etc.; it makes little sense to overflow a performance counter, so we
shouldn't subject those users to the same protections as imposed on
actual reference counters.  The solution implemented by PAX is to
create a family of *_unchecked() functions and to patch
statistics-based users of atomic_t to use this interface.

PAX refcount overflow protection was developed before kref was
created.  I'd like to move overflow protection out of atomic_t and
into kref and gradually migrate atomic_t users to kref, leaving
atomic_t for those users who don't need overflow protection (e.g.
statistics-based counters).

I realize that there are many users of atomic_t needing overflow
protection, but the move to kref seems like the right thing to do in
this case.

Leaving the semantics of overflow detection aside for the moment, what
are everyone's thoughts on adding overflow protection to kref rather
than to atomic_t?

Also, I cherrypicked the refcount protection feature directly from the
PAX patch, with the original atomic_t protections in place, before
considering kref.  If anyone is interested, I can post that patch.

Thanks,
David Windsor

--
PGP: 6141 5FFD 11AE 9844 153E  F268 7C98 7268 6B19 6CC9

--
ubuntu-hardened mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-hardened
Reply | Threaded
Open this post in threaded view
|

Re: Add overflow protection to kref

Kees Cook-2
Hi,

[This should probably be discussed on LKML for an even wider audience, so
I've added a CC for it there.]

On Thu, Feb 16, 2012 at 09:02:13AM -0500, David Windsor wrote:
> Hi,
>
> We are attempting to add various grsecurity/PAX features to upstream
> Ubuntu kernels.

This didn't parse quite right for me. I think you meant that the intent
is to get these features into the upstream Linux kernel, with potential
staging in Ubuntu kernels.

(Also s/PAX/PaX/g)

> The PAX folks added refcount overflow protection by inserting
> architecture-specific code in the increment paths of atomic_t.  For
> instance:
>
> static inline void atomic_inc(atomic_t *v)
>  {
> asm volatile(LOCK_PREFIX "incl %0\n"
>
> #ifdef CONFIG_PAX_REFCOUNT
>     "jno 0f\n"
>     LOCK_PREFIX "decl %0\n"
>     "int $4\n0:\n"
>     _ASM_EXTABLE(0b, 0b)
> #endif
>
>     : "+m" (v->counter));
> }
>
> There are two distinct classes of users we need to consider here:
> those who use atomic_t for reference counters and those who use
> atomic_t for keeping track of statistics, like performance counters,
> etc.; it makes little sense to overflow a performance counter, so we
> shouldn't subject those users to the same protections as imposed on
> actual reference counters.  The solution implemented by PAX is to
> create a family of *_unchecked() functions and to patch
> statistics-based users of atomic_t to use this interface.
>
> PAX refcount overflow protection was developed before kref was
> created.  I'd like to move overflow protection out of atomic_t and
> into kref and gradually migrate atomic_t users to kref, leaving
> atomic_t for those users who don't need overflow protection (e.g.
> statistics-based counters).

For people new to this, can you give an overview of what attacks are foiled
by adding overflow protection?

> I realize that there are many users of atomic_t needing overflow
> protection, but the move to kref seems like the right thing to do in
> this case.
>
> Leaving the semantics of overflow detection aside for the moment, what
> are everyone's thoughts on adding overflow protection to kref rather
> than to atomic_t?

Why was kref introduced? Or rather, how is kref currently different from
atomic_t?

> Also, I cherrypicked the refcount protection feature directly from the
> PAX patch, with the original atomic_t protections in place, before
> considering kref.  If anyone is interested, I can post that patch.
>
> Thanks,
> David Windsor
>
> --
> PGP: 6141 5FFD 11AE 9844 153E  F268 7C98 7268 6B19 6CC9

Thanks for bringing this up!

-Kees

--
Kees Cook
ChromeOS Security

--
ubuntu-hardened mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-hardened
Reply | Threaded
Open this post in threaded view
|

Re: Add overflow protection to kref

Greg KH-3
On Thu, Feb 16, 2012 at 12:45:15PM -0800, Kees Cook wrote:

> Hi,
>
> [This should probably be discussed on LKML for an even wider audience, so
> I've added a CC for it there.]
>
> On Thu, Feb 16, 2012 at 09:02:13AM -0500, David Windsor wrote:
> > Hi,
> >
> > We are attempting to add various grsecurity/PAX features to upstream
> > Ubuntu kernels.
>
> This didn't parse quite right for me. I think you meant that the intent
> is to get these features into the upstream Linux kernel, with potential
> staging in Ubuntu kernels.
>
> (Also s/PAX/PaX/g)
>
> > The PAX folks added refcount overflow protection by inserting
> > architecture-specific code in the increment paths of atomic_t.  For
> > instance:
> >
> > static inline void atomic_inc(atomic_t *v)
> >  {
> > asm volatile(LOCK_PREFIX "incl %0\n"
> >
> > #ifdef CONFIG_PAX_REFCOUNT
> >     "jno 0f\n"
> >     LOCK_PREFIX "decl %0\n"
> >     "int $4\n0:\n"
> >     _ASM_EXTABLE(0b, 0b)
> > #endif
> >
> >     : "+m" (v->counter));
> > }
> >
> > There are two distinct classes of users we need to consider here:
> > those who use atomic_t for reference counters and those who use
> > atomic_t for keeping track of statistics, like performance counters,
> > etc.; it makes little sense to overflow a performance counter, so we
> > shouldn't subject those users to the same protections as imposed on
> > actual reference counters.  The solution implemented by PAX is to
> > create a family of *_unchecked() functions and to patch
> > statistics-based users of atomic_t to use this interface.
> >
> > PAX refcount overflow protection was developed before kref was
> > created.  I'd like to move overflow protection out of atomic_t and
> > into kref and gradually migrate atomic_t users to kref, leaving
> > atomic_t for those users who don't need overflow protection (e.g.
> > statistics-based counters).
>
> For people new to this, can you give an overview of what attacks are foiled
> by adding overflow protection?
>
> > I realize that there are many users of atomic_t needing overflow
> > protection, but the move to kref seems like the right thing to do in
> > this case.
> >
> > Leaving the semantics of overflow detection aside for the moment, what
> > are everyone's thoughts on adding overflow protection to kref rather
> > than to atomic_t?
>
> Why was kref introduced? Or rather, how is kref currently different from
> atomic_t?

a kref is to handle reference counting for an object, so you don't have
to constantly "roll your own" all the time using an atomic_t or
whatever.  It's the basis for the struct kobject and other object
reference counting structures in the kernel for a very long time now.

And in all that time, I've never seen an instance where you can overflow
the reference count, so I'm hard pressed to see how changing kref in
this manner will help anything at all.

So no, I don't recommend changing this logic at all in kref.

Now if there are instances in the kernel where a "raw" atomic_t is being
used for object reference counting, moving that to use 'struct kref'
would be gladly appreciated, but that's kind of outside the scope of
what you are attempting to do here.

sorry,

greg k-h

--
ubuntu-hardened mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-hardened
Reply | Threaded
Open this post in threaded view
|

Re: Add overflow protection to kref

Kees Cook-2
On Thu, Feb 16, 2012 at 04:24:05PM -0800, Greg Kroah-Hartman wrote:

> On Thu, Feb 16, 2012 at 12:45:15PM -0800, Kees Cook wrote:
> > Hi,
> >
> > [This should probably be discussed on LKML for an even wider audience, so
> > I've added a CC for it there.]
> >
> > On Thu, Feb 16, 2012 at 09:02:13AM -0500, David Windsor wrote:
> > > Hi,
> > >
> > > We are attempting to add various grsecurity/PAX features to upstream
> > > Ubuntu kernels.
> >
> > This didn't parse quite right for me. I think you meant that the intent
> > is to get these features into the upstream Linux kernel, with potential
> > staging in Ubuntu kernels.
> >
> > (Also s/PAX/PaX/g)
> >
> > > The PAX folks added refcount overflow protection by inserting
> > > architecture-specific code in the increment paths of atomic_t.  For
> > > instance:
> > >
> > > static inline void atomic_inc(atomic_t *v)
> > >  {
> > > asm volatile(LOCK_PREFIX "incl %0\n"
> > >
> > > #ifdef CONFIG_PAX_REFCOUNT
> > >     "jno 0f\n"
> > >     LOCK_PREFIX "decl %0\n"
> > >     "int $4\n0:\n"
> > >     _ASM_EXTABLE(0b, 0b)
> > > #endif
> > >
> > >     : "+m" (v->counter));
> > > }
> > >
> > > There are two distinct classes of users we need to consider here:
> > > those who use atomic_t for reference counters and those who use
> > > atomic_t for keeping track of statistics, like performance counters,
> > > etc.; it makes little sense to overflow a performance counter, so we
> > > shouldn't subject those users to the same protections as imposed on
> > > actual reference counters.  The solution implemented by PAX is to
> > > create a family of *_unchecked() functions and to patch
> > > statistics-based users of atomic_t to use this interface.
> > >
> > > PAX refcount overflow protection was developed before kref was
> > > created.  I'd like to move overflow protection out of atomic_t and
> > > into kref and gradually migrate atomic_t users to kref, leaving
> > > atomic_t for those users who don't need overflow protection (e.g.
> > > statistics-based counters).
> >
> > For people new to this, can you give an overview of what attacks are foiled
> > by adding overflow protection?
> >
> > > I realize that there are many users of atomic_t needing overflow
> > > protection, but the move to kref seems like the right thing to do in
> > > this case.
> > >
> > > Leaving the semantics of overflow detection aside for the moment, what
> > > are everyone's thoughts on adding overflow protection to kref rather
> > > than to atomic_t?
> >
> > Why was kref introduced? Or rather, how is kref currently different from
> > atomic_t?
>
> a kref is to handle reference counting for an object, so you don't have
> to constantly "roll your own" all the time using an atomic_t or
> whatever.  It's the basis for the struct kobject and other object
> reference counting structures in the kernel for a very long time now.
>
> And in all that time, I've never seen an instance where you can overflow
> the reference count, so I'm hard pressed to see how changing kref in
> this manner will help anything at all.

A quick search gives me:
CVE-2005-3359: https://bugzilla.redhat.com/show_bug.cgi?id=175769
CVE-2006-3741: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=b8444d00762703e1b6146fce12ce2684885f8bf6

And actually an earlier discussion you were actually involved in:
https://lkml.org/lkml/2008/7/16/300

> So no, I don't recommend changing this logic at all in kref.

If it's inexpensive and helps defend against problems, it seems sensible to
add to me.

> Now if there are instances in the kernel where a "raw" atomic_t is being
> used for object reference counting, moving that to use 'struct kref'
> would be gladly appreciated, but that's kind of outside the scope of
> what you are attempting to do here.

-Kees

--
Kees Cook
ChromeOS Security

--
ubuntu-hardened mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-hardened
Reply | Threaded
Open this post in threaded view
|

Re: Add overflow protection to kref

Greg KH-3
On Thu, Feb 16, 2012 at 05:06:24PM -0800, Kees Cook wrote:

Any reason you forgot to cc: me on the response?

> On Thu, Feb 16, 2012 at 04:24:05PM -0800, Greg Kroah-Hartman wrote:
> > On Thu, Feb 16, 2012 at 12:45:15PM -0800, Kees Cook wrote:
> > > Hi,
> > >
> > > [This should probably be discussed on LKML for an even wider audience, so
> > > I've added a CC for it there.]
> > >
> > > On Thu, Feb 16, 2012 at 09:02:13AM -0500, David Windsor wrote:
> > > > Hi,
> > > >
> > > > We are attempting to add various grsecurity/PAX features to upstream
> > > > Ubuntu kernels.
> > >
> > > This didn't parse quite right for me. I think you meant that the intent
> > > is to get these features into the upstream Linux kernel, with potential
> > > staging in Ubuntu kernels.
> > >
> > > (Also s/PAX/PaX/g)
> > >
> > > > The PAX folks added refcount overflow protection by inserting
> > > > architecture-specific code in the increment paths of atomic_t.  For
> > > > instance:
> > > >
> > > > static inline void atomic_inc(atomic_t *v)
> > > >  {
> > > > asm volatile(LOCK_PREFIX "incl %0\n"
> > > >
> > > > #ifdef CONFIG_PAX_REFCOUNT
> > > >     "jno 0f\n"
> > > >     LOCK_PREFIX "decl %0\n"
> > > >     "int $4\n0:\n"
> > > >     _ASM_EXTABLE(0b, 0b)
> > > > #endif
> > > >
> > > >     : "+m" (v->counter));
> > > > }
> > > >
> > > > There are two distinct classes of users we need to consider here:
> > > > those who use atomic_t for reference counters and those who use
> > > > atomic_t for keeping track of statistics, like performance counters,
> > > > etc.; it makes little sense to overflow a performance counter, so we
> > > > shouldn't subject those users to the same protections as imposed on
> > > > actual reference counters.  The solution implemented by PAX is to
> > > > create a family of *_unchecked() functions and to patch
> > > > statistics-based users of atomic_t to use this interface.
> > > >
> > > > PAX refcount overflow protection was developed before kref was
> > > > created.  I'd like to move overflow protection out of atomic_t and
> > > > into kref and gradually migrate atomic_t users to kref, leaving
> > > > atomic_t for those users who don't need overflow protection (e.g.
> > > > statistics-based counters).
> > >
> > > For people new to this, can you give an overview of what attacks are foiled
> > > by adding overflow protection?
> > >
> > > > I realize that there are many users of atomic_t needing overflow
> > > > protection, but the move to kref seems like the right thing to do in
> > > > this case.
> > > >
> > > > Leaving the semantics of overflow detection aside for the moment, what
> > > > are everyone's thoughts on adding overflow protection to kref rather
> > > > than to atomic_t?
> > >
> > > Why was kref introduced? Or rather, how is kref currently different from
> > > atomic_t?
> >
> > a kref is to handle reference counting for an object, so you don't have
> > to constantly "roll your own" all the time using an atomic_t or
> > whatever.  It's the basis for the struct kobject and other object
> > reference counting structures in the kernel for a very long time now.
> >
> > And in all that time, I've never seen an instance where you can overflow
> > the reference count, so I'm hard pressed to see how changing kref in
> > this manner will help anything at all.
>
> A quick search gives me:
> CVE-2005-3359: https://bugzilla.redhat.com/show_bug.cgi?id=175769
> CVE-2006-3741: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=b8444d00762703e1b6146fce12ce2684885f8bf6

Neither of those are kref issues, just bugs with other types of
counting things.

> And actually an earlier discussion you were actually involved in:
> https://lkml.org/lkml/2008/7/16/300

That wasn't about a kref issue either.  It was also a fun flamefest, but
I don't see how that is relevant here.  What am I missing?

> > So no, I don't recommend changing this logic at all in kref.
>
> If it's inexpensive and helps defend against problems, it seems sensible to
> add to me.

I have yet to see a patch, so why are we arguing about this?  :)

Again, I don't know of any kref overflows that have ever happened, so
trying to "protect" this type of thing, seems odd to me.

thanks,

greg k-h

--
ubuntu-hardened mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-hardened
Reply | Threaded
Open this post in threaded view
|

Re: Add overflow protection to kref

Kees Cook-2
On Thu, Feb 16, 2012 at 05:40:08PM -0800, Greg KH wrote:
> Any reason you forgot to cc: me on the response?

Sorry about that; my MUA and I were fighting. :( Looks like David got
dropped too. Fixed.

> On Thu, Feb 16, 2012 at 05:06:24PM -0800, Kees Cook wrote:
> > On Thu, Feb 16, 2012 at 04:24:05PM -0800, Greg Kroah-Hartman wrote:
> > > On Thu, Feb 16, 2012 at 12:45:15PM -0800, Kees Cook wrote:
> > > > Hi,
> > > >
> > > > [This should probably be discussed on LKML for an even wider audience, so
> > > > I've added a CC for it there.]
> > > >
> > > > On Thu, Feb 16, 2012 at 09:02:13AM -0500, David Windsor wrote:
> > > > > Hi,
> > > > >
> > > > > We are attempting to add various grsecurity/PAX features to upstream
> > > > > Ubuntu kernels.
> > > >
> > > > This didn't parse quite right for me. I think you meant that the intent
> > > > is to get these features into the upstream Linux kernel, with potential
> > > > staging in Ubuntu kernels.
> > > >
> > > > (Also s/PAX/PaX/g)
> > > >
> > > > > The PAX folks added refcount overflow protection by inserting
> > > > > architecture-specific code in the increment paths of atomic_t.  For
> > > > > instance:
> > > > >
> > > > > static inline void atomic_inc(atomic_t *v)
> > > > >  {
> > > > > asm volatile(LOCK_PREFIX "incl %0\n"
> > > > >
> > > > > #ifdef CONFIG_PAX_REFCOUNT
> > > > >     "jno 0f\n"
> > > > >     LOCK_PREFIX "decl %0\n"
> > > > >     "int $4\n0:\n"
> > > > >     _ASM_EXTABLE(0b, 0b)
> > > > > #endif
> > > > >
> > > > >     : "+m" (v->counter));
> > > > > }
> > > > >
> > > > > There are two distinct classes of users we need to consider here:
> > > > > those who use atomic_t for reference counters and those who use
> > > > > atomic_t for keeping track of statistics, like performance counters,
> > > > > etc.; it makes little sense to overflow a performance counter, so we
> > > > > shouldn't subject those users to the same protections as imposed on
> > > > > actual reference counters.  The solution implemented by PAX is to
> > > > > create a family of *_unchecked() functions and to patch
> > > > > statistics-based users of atomic_t to use this interface.
> > > > >
> > > > > PAX refcount overflow protection was developed before kref was
> > > > > created.  I'd like to move overflow protection out of atomic_t and
> > > > > into kref and gradually migrate atomic_t users to kref, leaving
> > > > > atomic_t for those users who don't need overflow protection (e.g.
> > > > > statistics-based counters).
> > > >
> > > > For people new to this, can you give an overview of what attacks are foiled
> > > > by adding overflow protection?
> > > >
> > > > > I realize that there are many users of atomic_t needing overflow
> > > > > protection, but the move to kref seems like the right thing to do in
> > > > > this case.
> > > > >
> > > > > Leaving the semantics of overflow detection aside for the moment, what
> > > > > are everyone's thoughts on adding overflow protection to kref rather
> > > > > than to atomic_t?
> > > >
> > > > Why was kref introduced? Or rather, how is kref currently different from
> > > > atomic_t?
> > >
> > > a kref is to handle reference counting for an object, so you don't have
> > > to constantly "roll your own" all the time using an atomic_t or
> > > whatever.  It's the basis for the struct kobject and other object
> > > reference counting structures in the kernel for a very long time now.
> > >
> > > And in all that time, I've never seen an instance where you can overflow
> > > the reference count, so I'm hard pressed to see how changing kref in
> > > this manner will help anything at all.
> >
> > A quick search gives me:
> > CVE-2005-3359: https://bugzilla.redhat.com/show_bug.cgi?id=175769
> > CVE-2006-3741: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=b8444d00762703e1b6146fce12ce2684885f8bf6
>
> Neither of those are kref issues, just bugs with other types of
> counting things.
>
> > And actually an earlier discussion you were actually involved in:
> > https://lkml.org/lkml/2008/7/16/300
>
> That wasn't about a kref issue either.  It was also a fun flamefest, but
> I don't see how that is relevant here.  What am I missing?

I was just trying to find some background on this topic. Perhaps other
folks have more details?

> > > So no, I don't recommend changing this logic at all in kref.
> >
> > If it's inexpensive and helps defend against problems, it seems sensible to
> > add to me.
>
> I have yet to see a patch, so why are we arguing about this?  :)
>
> Again, I don't know of any kref overflows that have ever happened, so
> trying to "protect" this type of thing, seems odd to me.

Well, I think the issue was to protect counting things (which seems to
be what PaX was after originally), and that kref seemed like the place
to put it. I'll let David take it further.

Thanks,

-Kees

--
Kees Cook
ChromeOS Security

--
ubuntu-hardened mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-hardened
Reply | Threaded
Open this post in threaded view
|

Re: Add overflow protection to kref

David Windsor
<snip>

>>
>> I have yet to see a patch, so why are we arguing about this?  :)
>>
>> Again, I don't know of any kref overflows that have ever happened, so
>> trying to "protect" this type of thing, seems odd to me.
>
> Well, I think the issue was to protect counting things (which seems to
> be what PaX was after originally), and that kref seemed like the place
> to put it. I'll let David take it further.
>

Patches are forthcoming that will first introduce overflow protection
to kref.  Once that's in place, I'll move a few refcount users from
atomic_t to kref as a reference for other subsystems; statistics-based
users (and others not requiring overflow protection) can continue
using atomic_t.

As Kees said, we just wanted to introduce the idea and get some
general feedback before beginning.  Thanks.

> Thanks,
>
> -Kees
>
> --
> Kees Cook
> ChromeOS Security



--
PGP: 6141 5FFD 11AE 9844 153E  F268 7C98 7268 6B19 6CC9

--
ubuntu-hardened mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-hardened
Reply | Threaded
Open this post in threaded view
|

Re: Add overflow protection to kref

Greg KH-3
On Thu, Feb 16, 2012 at 09:48:38PM -0500, David Windsor wrote:

> <snip>
>
> >>
> >> I have yet to see a patch, so why are we arguing about this?  :)
> >>
> >> Again, I don't know of any kref overflows that have ever happened, so
> >> trying to "protect" this type of thing, seems odd to me.
> >
> > Well, I think the issue was to protect counting things (which seems to
> > be what PaX was after originally), and that kref seemed like the place
> > to put it. I'll let David take it further.
> >
>
> Patches are forthcoming that will first introduce overflow protection
> to kref.  Once that's in place, I'll move a few refcount users from
> atomic_t to kref as a reference for other subsystems;

I'd like to see some more users first, as I don't see how the current
users could ever overflow an atomic_t, so any changes to prevent this
will be kind of pointless, right?

So feel free to send all of these changes together.

thanks,

greg k-h

--
ubuntu-hardened mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-hardened
Reply | Threaded
Open this post in threaded view
|

Re: Add overflow protection to kref

Alexey Dobriyan
In reply to this post by David Windsor
On Thu, Feb 16, 2012 at 09:48:38PM -0500, David Windsor wrote:

> <snip>
>
> >>
> >> I have yet to see a patch, so why are we arguing about this?  :)
> >>
> >> Again, I don't know of any kref overflows that have ever happened, so
> >> trying to "protect" this type of thing, seems odd to me.
> >
> > Well, I think the issue was to protect counting things (which seems to
> > be what PaX was after originally), and that kref seemed like the place
> > to put it. I'll let David take it further.
> >
>
> Patches are forthcoming that will first introduce overflow protection
> to kref.

Patches have already been posted:
http://marc.info/?l=linux-kernel&m=132337541830590&w=4
They were dropped for various (uninteresting) reasons, though.

> Once that's in place, I'll move a few refcount users from
> atomic_t to kref as a reference for other subsystems;

This sucks because dtor argument is mandatory.

--
ubuntu-hardened mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-hardened
Reply | Threaded
Open this post in threaded view
|

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

Vasiliy Kulikov-2
In reply to this post by Greg KH-3
Hi,

> And in all that time, I've never seen an instance where you can overflow
> the reference count,

Do you mean that the overflow is theoretically impossible or that this
type of programmer error is rare?

If the former, it is only 2**32 incs - if you can find open() implementation
with a missing atomic_dec() in error path and you can call open() faster than
10000 times per second, you can overflow the counter in ~4 days.

If the latter, it is just a question of finding missing put() in some triggerable
error path.  Kees has already posted a link to a bug with a missing fput().


BTW, moving from atomic_t to 64 bit refcounter would kill the possibility of
overflow.  Unfortunately, AFAIU, 64 bit operations are not atomic on some 64 bit
archs.

Thanks,

--
Vasiliy

--
ubuntu-hardened mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-hardened
Reply | Threaded
Open this post in threaded view
|

Re: Add overflow protection to kref

pageexec
In reply to this post by Greg KH-3
On 16 Feb 2012 at 17:40, Greg KH wrote:

> > A quick search gives me:
> > CVE-2005-3359: https://bugzilla.redhat.com/show_bug.cgi?id=175769
> > CVE-2006-3741: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=b8444d00762703e1b6146fce12ce2684885f8bf6
>
> Neither of those are kref issues, just bugs with other types of
> counting things.

kref is just one of the many users of the atomic type that use it for refcounting.
and kref users are no more immune to programming mistakes that can result in overflow
than direct atomic users.

> Again, I don't know of any kref overflows that have ever happened, so
> trying to "protect" this type of thing, seems odd to me.

well, do you analyze all bugreports/patches/etc for this problem? even if you did
it's easy to miss them because kref_put has so many wrappers around it whose uses
would also have to be checked for. anyway, a very lightweight grep in the git log
turned up these (i spent half a minute on it only, don't expect a flood ;):

14660ccd599dc7bd6ecef17408bd76dc853f9b77
ece84241b93c4693bfe0a8ec9a043a16d216d0cd
b65d457913d1c0644394287d5d834373f42fb99a
94735ec4044a6d318b83ad3c5794e931ed168d10


--
ubuntu-hardened mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-hardened
Reply | Threaded
Open this post in threaded view
|

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

Greg KH-3
In reply to this post by Vasiliy Kulikov-2
On Fri, Feb 17, 2012 at 11:59:45AM +0400, Vasiliy Kulikov wrote:

> Hi,
>
> > And in all that time, I've never seen an instance where you can overflow
> > the reference count,
>
> Do you mean that the overflow is theoretically impossible or that this
> type of programmer error is rare?
>
> If the former, it is only 2**32 incs - if you can find open() implementation
> with a missing atomic_dec() in error path and you can call open() faster than
> 10000 times per second, you can overflow the counter in ~4 days.
>
> If the latter, it is just a question of finding missing put() in some triggerable
> error path.  Kees has already posted a link to a bug with a missing fput().
>
>
> BTW, moving from atomic_t to 64 bit refcounter would kill the possibility of
> overflow.  Unfortunately, AFAIU, 64 bit operations are not atomic on some 64 bit
> archs.

Can we switch it on those arches where it is an atomic operation?  That
would be a nice simple solution.

thanks,

greg k-h

--
ubuntu-hardened mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-hardened
Reply | Threaded
Open this post in threaded view
|

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

Greg KH-3
In reply to this post by Vasiliy Kulikov-2
On Fri, Feb 17, 2012 at 11:59:45AM +0400, Vasiliy Kulikov wrote:

> Hi,
>
> > And in all that time, I've never seen an instance where you can overflow
> > the reference count,
>
> Do you mean that the overflow is theoretically impossible or that this
> type of programmer error is rare?
>
> If the former, it is only 2**32 incs - if you can find open() implementation
> with a missing atomic_dec() in error path and you can call open() faster than
> 10000 times per second, you can overflow the counter in ~4 days.
>
> If the latter, it is just a question of finding missing put() in some triggerable
> error path.  Kees has already posted a link to a bug with a missing fput().

I'm referring to the fact that the use of kref in this type of error or
problem is rare.

Yes, we have these types of problems at times, but a kref doesn't seem
to be involved in them that I know of, so changing the kref code
wouldn't help here from what I can tell.

thanks,

greg k-h

--
ubuntu-hardened mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-hardened
Reply | Threaded
Open this post in threaded view
|

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

Vasiliy Kulikov-2
On Fri, Feb 17, 2012 at 09:54 -0800, Greg KH wrote:
> I'm referring to the fact that the use of kref in this type of error or
> problem is rare.
>
> Yes, we have these types of problems at times, but a kref doesn't seem
> to be involved in them that I know of, so changing the kref code
> wouldn't help here from what I can tell.

Ehr, what's the difference between kref and "raw" atomic_t in a refcounting case?
There is _no_ difference in sense of overflows as a kref uses the same atomic_t.

I second David that we should use kref for overflow protection: we want to
hook an overflow case somehow in cases atomic_t is used as a refcounter.  It is
_ideally_ handled by introducing atomic_t's subtype.  And this subtype already
exists - it is called kref.


I expect all atomic_t refcounters users have

        if (atomic_dec_and_test()) smth_put()

pattern, otherwise it is not a true refcounter :)  It should be straightforward to
move to kref.


Moving to atomic64_t is attractive, but:

1) we still should find all atomic_t refcounters.  Why not move to kref then?

2) what to do with architectures-loosers?


Thanks,

--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

--
ubuntu-hardened mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-hardened
Reply | Threaded
Open this post in threaded view
|

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

Djalal Harouni
On Fri, Feb 17, 2012 at 11:37:19PM +0400, Vasiliy Kulikov wrote:
> pattern, otherwise it is not a true refcounter :)  It should be straightforward to
> move to kref.
>
>
> Moving to atomic64_t is attractive, but:
>
> 1) we still should find all atomic_t refcounters.  Why not move to kref then?
>
> 2) what to do with architectures-loosers?
There is lib/atomic64.c but with a static hashed array of raw_spinlocks.

>
> Thanks,
>
> --
> Vasiliy Kulikov
> http://www.openwall.com - bringing security into open computing environments
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [hidden email]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

--
tixxdz
http://opendz.org

--
ubuntu-hardened mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-hardened
Reply | Threaded
Open this post in threaded view
|

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

Roland Dreier
On Fri, Feb 17, 2012 at 3:39 PM, Djalal Harouni <[hidden email]> wrote:
>> 2) what to do with architectures-loosers?
> There is lib/atomic64.c but with a static hashed array of raw_spinlocks.

Even leaving aside performance impact of atomic64_t (and probably
in most cases the performance of kref is not important at all), it is
unfortunate to bloat the size from 4 bytes to 8 bytes.

It seems much better to have some out-of-line code for overflow
checking rather than increasing the size of every data structure
that embeds a kref.

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?

 - R.

--
ubuntu-hardened mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-hardened
Reply | Threaded
Open this post in threaded view
|

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

David Windsor
On Fri, Feb 17, 2012 at 8:44 PM, Roland Dreier <[hidden email]> wrote:

> On Fri, Feb 17, 2012 at 3:39 PM, Djalal Harouni <[hidden email]> wrote:
>>> 2) what to do with architectures-loosers?
>> There is lib/atomic64.c but with a static hashed array of raw_spinlocks.
>
> Even leaving aside performance impact of atomic64_t (and probably
> in most cases the performance of kref is not important at all), it is
> unfortunate to bloat the size from 4 bytes to 8 bytes.
>
> It seems much better to have some out-of-line code for overflow
> checking rather than increasing the size of every data structure
> that embeds a kref.
>

kref is mostly a set of operations (init, get, sub, put) to be
performed on an atomic_t object.

From linux/kref.h:

struct kref {
    atomic_t refcount;
};

Moving overflow protection into kref amounts to placing some
procedural code into kref_get and kref_sub, adding a rather small
constant factor of time, not space, to users of kref.  Introducing
overflow protection doesn't necessitate adding anything to kref for
greater state tracking.

Did you have something else in mind when you suggested a potential
increase in the size of kref?


--
PGP: 6141 5FFD 11AE 9844 153E  F268 7C98 7268 6B19 6CC9

--
ubuntu-hardened mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-hardened
Reply | Threaded
Open this post in threaded view
|

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

Greg KH-3
In reply to this post by Roland Dreier
On Fri, Feb 17, 2012 at 05:44:57PM -0800, Roland Dreier wrote:

> On Fri, Feb 17, 2012 at 3:39 PM, Djalal Harouni <[hidden email]> wrote:
> >> 2) what to do with architectures-loosers?
> > There is lib/atomic64.c but with a static hashed array of raw_spinlocks.
>
> Even leaving aside performance impact of atomic64_t (and probably
> in most cases the performance of kref is not important at all), it is
> unfortunate to bloat the size from 4 bytes to 8 bytes.
>
> It seems much better to have some out-of-line code for overflow
> checking rather than increasing the size of every data structure
> that embeds a kref.

Please realize that kref is an in-line structure now.

> 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

--
ubuntu-hardened mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-hardened
Reply | Threaded
Open this post in threaded view
|

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

Vasiliy Kulikov-2
In reply to this post by David Windsor
On Sat, Feb 18, 2012 at 11:15 -0500, David Windsor wrote:

> On Fri, Feb 17, 2012 at 8:44 PM, Roland Dreier <[hidden email]> wrote:
> > On Fri, Feb 17, 2012 at 3:39 PM, Djalal Harouni <[hidden email]> wrote:
> >>> 2) what to do with architectures-loosers?
> >> There is lib/atomic64.c but with a static hashed array of raw_spinlocks.
> >
> > Even leaving aside performance impact of atomic64_t (and probably
> > in most cases the performance of kref is not important at all), it is
> > unfortunate to bloat the size from 4 bytes to 8 bytes.
> >
> > It seems much better to have some out-of-line code for overflow
> > checking rather than increasing the size of every data structure
> > that embeds a kref.
> >
>
> kref is mostly a set of operations (init, get, sub, put) to be
> performed on an atomic_t object.
>
> >From linux/kref.h:
>
> struct kref {
>     atomic_t refcount;
> };
>
> Moving overflow protection into kref amounts to placing some
> procedural code into kref_get and kref_sub, adding a rather small
> constant factor of time, not space, to users of kref.  Introducing
> overflow protection doesn't necessitate adding anything to kref for
> greater state tracking.
>
> Did you have something else in mind when you suggested a potential
> increase in the size of kref?

4 bytes => 8 bytes of atomic_t => atomic64_t in case we increase the refcounter
range to make it impossible to overflow the refcounter

compared to

add checks into kref_get()/atomic_inc*() without changing refcounter ranges.


Thanks,

--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

--
ubuntu-hardened mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-hardened
Reply | Threaded
Open this post in threaded view
|

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

David Windsor
In reply to this post by Greg KH-3
<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 <[hidden email]>
---
 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);
 }

 /**
--
1.7.5.4


--
PGP: 6141 5FFD 11AE 9844 153E  F268 7C98 7268 6B19 6CC9

--
ubuntu-hardened mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-hardened
12