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
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
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
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
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
<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
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
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.
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
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
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
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
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
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 [email protected]
> 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
On Fri, Feb 17, 2012 at 3:39 PM, Djalal Harouni <[email protected]> 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.
On Fri, Feb 17, 2012 at 8:44 PM, Roland Dreier <[email protected]> wrote:
> On Fri, Feb 17, 2012 at 3:39 PM, Djalal Harouni <[email protected]> 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
On Fri, Feb 17, 2012 at 05:44:57PM -0800, Roland Dreier wrote:
> On Fri, Feb 17, 2012 at 3:39 PM, Djalal Harouni <[email protected]> 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
On Sat, Feb 18, 2012 at 11:15 -0500, David Windsor wrote:
> On Fri, Feb 17, 2012 at 8:44 PM, Roland Dreier <[email protected]> wrote:
> > On Fri, Feb 17, 2012 at 3:39 PM, Djalal Harouni <[email protected]> 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
<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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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?
And people wonder why I no longer have any hair...
greg k-h
On Fri, Feb 24, 2012 at 10:37 AM, Greg KH <[email protected]> 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 <[email protected]>
>> ---
>> ?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
On Fri, Feb 24, 2012 at 10:37 -0800, Greg KH wrote:
> On Fri, Feb 24, 2012 at 12:58:35PM -0500, David Windsor wrote:
> > 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?
>
> And people wonder why I no longer have any hair...
If a refcounter overflows there is NO WAY to recover. The choise is to BUG()
and not allow any security harm to the system (privilege escalation, etc.)
or to try to do some more CPU cycles until actual use after free, privilege
escalation, etc. The former is a _guarantee_ that nothing bad (in security
sense) doesn't happen. The latter is an opportunistic approach, which
doesn't work with security.
Do you claim that a refcounter overflow is a recoverable state? I'd want to
know what you can do with it.
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
On Fri, Feb 24, 2012 at 1:37 PM, Greg KH <[email protected]> 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 <[email protected]>
>> ---
>> ?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?
>
Suggestions on recovering from an overflow here?
The salient possibilities are:
1. Do nothing, as was the case before this patch.
This leads to undefined behavior, but typically, a wrap happens. This
leads to use-after-free bugs.
2. Detect the overflow before it happens, don't increment the counter,
issue a warning but no BUG.
This could also lead to some "undefined" behavior in subsystems. I
can't imagine many users of kref have considered the situation of
kref_get essentially being a no-op, which is what would happen in this
situation.
3. Detect the overflow before it happens, don't increment the counter,
issue a BUG.
This is what the proposed patch does. Suggestions for recovering
gracefully from this overflow are welcome.
--
PGP: 6141 5FFD 11AE 9844 153E ?F268 7C98 7268 6B19 6CC9
On 2012-02-24 10:52 -0800, Kees Cook wrote:
> On Fri, Feb 24, 2012 at 10:37 AM, Greg KH <[email protected]> wrote:
> > On Fri, Feb 24, 2012 at 12:58:35PM -0500, David Windsor wrote:
[...]
> >> 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().
But in this case, the principle does not apply because we can recover.
The reason we cannot recover from the stack protector case is because
the stack protector is reacting after the fact, which is not the case
here. Simply peg the reference count at the maximum value, neither
incrementing it nor decrementing it further.
Cheers,
--
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)
On Fri, Feb 24, 2012 at 14:05 -0500, Nick Bowler wrote:
> On 2012-02-24 10:52 -0800, Kees Cook wrote:
> > On Fri, Feb 24, 2012 at 10:37 AM, Greg KH <[email protected]> wrote:
> > > On Fri, Feb 24, 2012 at 12:58:35PM -0500, David Windsor wrote:
> [...]
> > >> 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().
>
> But in this case, the principle does not apply because we can recover.
> The reason we cannot recover from the stack protector case is because
> the stack protector is reacting after the fact, which is not the case
> here. Simply peg the reference count at the maximum value, neither
> incrementing it nor decrementing it further.
...and simply loose one reference, which leads to use-after-free.
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
On 2012-02-24 23:13 +0400, Vasiliy Kulikov wrote:
> On Fri, Feb 24, 2012 at 14:05 -0500, Nick Bowler wrote:
> > On 2012-02-24 10:52 -0800, Kees Cook wrote:
> > > On Fri, Feb 24, 2012 at 10:37 AM, Greg KH <[email protected]> wrote:
> > > > On Fri, Feb 24, 2012 at 12:58:35PM -0500, David Windsor wrote:
> > [...]
> > > >> 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().
> >
> > But in this case, the principle does not apply because we can recover.
> > The reason we cannot recover from the stack protector case is because
> > the stack protector is reacting after the fact, which is not the case
> > here. Simply peg the reference count at the maximum value, neither
> > incrementing it nor decrementing it further.
>
> ...and simply loose one reference, which leads to use-after-free.
Please explain how a use-after-free could possibly occur if the
reference count is never incremented or decremented again?
Cheers,
--
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)
On Fri, Feb 24, 2012 at 10:58:25PM +0400, Vasiliy Kulikov wrote:
> On Fri, Feb 24, 2012 at 10:37 -0800, Greg KH wrote:
> > On Fri, Feb 24, 2012 at 12:58:35PM -0500, David Windsor wrote:
> > > 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?
> >
> > And people wonder why I no longer have any hair...
>
> If a refcounter overflows there is NO WAY to recover. The choise is to BUG()
> and not allow any security harm to the system (privilege escalation, etc.)
> or to try to do some more CPU cycles until actual use after free, privilege
> escalation, etc. The former is a _guarantee_ that nothing bad (in security
> sense) doesn't happen. The latter is an opportunistic approach, which
> doesn't work with security.
The only way you could legimitaly get a real use-after-free problem if
you were overflowing the reference counter and pegged it at the max
value, was if you had code that could decrement the reference count as
many times as you incremented it. So far, all bugs we've seen are
one-off where on an error path, we forgot to decrement the count. So
how could the decrement ever happen?
> Do you claim that a refcounter overflow is a recoverable state? I'd want to
> know what you can do with it.
I'm not saying it is a "recoverable" state, but to crash the machine is
not acceptable. At the very least, let the user know something went
wrong, and stick around long enough to let them know and do something,
before shutting the thing down.
But before people start micro-engineering this whole thing, remember,
I'm still not sold on this type of change at all.
greg k-h
p.s. Has anyone ever tried an endless open() loop on a sysfs file to see
what happens today?...
On Fri, Feb 24, 2012 at 11:41 AM, Greg KH <[email protected]> wrote:
> On Fri, Feb 24, 2012 at 10:58:25PM +0400, Vasiliy Kulikov wrote:
>> On Fri, Feb 24, 2012 at 10:37 -0800, Greg KH wrote:
>> > On Fri, Feb 24, 2012 at 12:58:35PM -0500, David Windsor wrote:
>> > > ?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?
>> >
>> > And people wonder why I no longer have any hair...
>>
>> If a refcounter overflows there is NO WAY to recover. ?The choise is to BUG()
>> and not allow any security harm to the system (privilege escalation, etc.)
>> or to try to do some more CPU cycles until actual use after free, privilege
>> escalation, etc. ?The former is a _guarantee_ that nothing bad (in security
>> sense) doesn't happen. ?The latter is an opportunistic approach, which
>> doesn't work with security.
>
> The only way you could legimitaly get a real use-after-free problem if
> you were overflowing the reference counter and pegged it at the max
> value, was if you had code that could decrement the reference count as
> many times as you incremented it. ?So far, all bugs we've seen are
> one-off where on an error path, we forgot to decrement the count. ?So
> how could the decrement ever happen?
Based on what I've seen, the "normal" exploit follows this pattern:
user1: alloc(), inc
user2: inc
user2: fail to dec
*repeat user2's actions until wrap*
user3: inc
user3: dec, free()
user1: operate on freed memory zomg
We could avoid the BUG by locking the counter to INT_MAX if it ever
reaches it (i.e. the put path would need to be modified too), and then
a WARN could be added to the get. Is that what was being suggested as
the alternative to the BUG patch?
>> Do you claim that a refcounter overflow is a recoverable state? ?I'd want to
>> know what you can do with it.
>
> I'm not saying it is a "recoverable" state, but to crash the machine is
> not acceptable. ?At the very least, let the user know something went
> wrong, and stick around long enough to let them know and do something,
> before shutting the thing down.
>
> But before people start micro-engineering this whole thing, remember,
> I'm still not sold on this type of change at all.
>
> greg k-h
>
> p.s. Has anyone ever tried an endless open() loop on a sysfs file to see
> ? ? what happens today?...
AIUI, you'd hit nrfile well before wrapping the counter. To test this,
we'd need to simulate a failed decrement.
-Kees
--
Kees Cook
ChromeOS Security
On 24 Feb 2012 at 23:13, Vasiliy Kulikov wrote:
> > But in this case, the principle does not apply because we can recover.
> > The reason we cannot recover from the stack protector case is because
> > the stack protector is reacting after the fact, which is not the case
> > here. Simply peg the reference count at the maximum value, neither
> > incrementing it nor decrementing it further.
>
> ...and simply loose one reference, which leads to use-after-free.
saturating the refcount keeps the protected object allocated, so it is
a memory leak, but it is not a use-after-free.
On 24 Feb 2012 at 14:04, David Windsor wrote:
> Suggestions on recovering from an overflow here?
do what PaX does? :)
1. saturate the refcount
2. report the event (pax_report_refcount_overflow)
3. kill the triggering userland process (this may be optional but it's a good measure
if you have stuff like grsecurity's lockout feature)
> 2. Detect the overflow before it happens, don't increment the counter,
> issue a warning but no BUG.
> This could also lead to some "undefined" behavior in subsystems.
it's called a memory leak :).