Re: [PATCH 0/5] fs, xfs refcount conversions

2017-04-20 Thread Kees Cook 
On Thu, Apr 20, 2017 at 3:04 PM, Darrick J. Wong
 wrote:
> On Thu, Apr 20, 2017 at 10:24:04AM -0700, Darrick J. Wong wrote:
>> On Thu, Apr 20, 2017 at 08:11:41AM +, Reshetova, Elena wrote:
>> >
>> >
>> > > v3:
>> > >  * fixed header file inclusion
>> >
>> > I don't think I have heard anything back on this v3 patch set.
>> > Is there still smth here to fix or could you take the changes in?
>>
>> Generally I think it looks ok; it's running through shakedown testing as
>> we speak.
>
> Well, it survives the shakedown testing all right, but I can't shake the
> feeling that this is overkill.  The xfs_{ef,bu,cu,ru}i_log_item
> structures should only ever be referenced by the log itself and the
> log-update-done log item: the refcount should only ever go 2 -> 1 -> 0.
> We set the refcount to 2 and never increment it.

I haven't studied this code, but I think it'd likely be worth keeping
it just to catch any future bug that might appear. If it doesn't
create a problem, we should use refcount_t for anything that is being
used as a reference counter.

> Given that I've seen occasional refcounting problems with the log intent
> items, I think it would suffice to ASSERT in xfs_*i_release that the
> refcount isn't already zero.  Using ASSERT is particularly useful
> because ASSERT compiles out in release builds.

We _absolutely_ don't want to compile out these checks: the point is
to catch flaws that are discovered and could be exploited in the wild
before updates could be delivered to a system (if they ever are at
all).

> As for the xlog_ticket, we increment its refcount as part of duplicating
> a transaction as a part of committing one transaction (which decrements
> the xlog_ticket's refcount) and reusing the log reservation to create a
> new transaction.  In this case the refcounting already seems to have
> sufficient non-zero checks, and since only one thread can hold a
> transaction at any given time, I don't see how overflow protection helps
> here.
>
> In other words, I'm ok with better refcount checking but need convincing
> that we need to do more than what a simple ASSERT would provide.

If it doesn't get in your way, and it really is reference counting
(which it sounds like it is), we should be using refcount_t. The kinds
of bugs we've seen over the last decade continually prove that we'll
see refcount issues where we least expect it, and we need to catch
them.

Thanks!

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 0/5] fs, xfs refcount conversions

2017-04-20 Thread Kees Cook 
On Thu, Apr 20, 2017 at 3:04 PM, Darrick J. Wong
 wrote:
> On Thu, Apr 20, 2017 at 10:24:04AM -0700, Darrick J. Wong wrote:
>> On Thu, Apr 20, 2017 at 08:11:41AM +, Reshetova, Elena wrote:
>> >
>> >
>> > > v3:
>> > >  * fixed header file inclusion
>> >
>> > I don't think I have heard anything back on this v3 patch set.
>> > Is there still smth here to fix or could you take the changes in?
>>
>> Generally I think it looks ok; it's running through shakedown testing as
>> we speak.
>
> Well, it survives the shakedown testing all right, but I can't shake the
> feeling that this is overkill.  The xfs_{ef,bu,cu,ru}i_log_item
> structures should only ever be referenced by the log itself and the
> log-update-done log item: the refcount should only ever go 2 -> 1 -> 0.
> We set the refcount to 2 and never increment it.

I haven't studied this code, but I think it'd likely be worth keeping
it just to catch any future bug that might appear. If it doesn't
create a problem, we should use refcount_t for anything that is being
used as a reference counter.

> Given that I've seen occasional refcounting problems with the log intent
> items, I think it would suffice to ASSERT in xfs_*i_release that the
> refcount isn't already zero.  Using ASSERT is particularly useful
> because ASSERT compiles out in release builds.

We _absolutely_ don't want to compile out these checks: the point is
to catch flaws that are discovered and could be exploited in the wild
before updates could be delivered to a system (if they ever are at
all).

> As for the xlog_ticket, we increment its refcount as part of duplicating
> a transaction as a part of committing one transaction (which decrements
> the xlog_ticket's refcount) and reusing the log reservation to create a
> new transaction.  In this case the refcounting already seems to have
> sufficient non-zero checks, and since only one thread can hold a
> transaction at any given time, I don't see how overflow protection helps
> here.
>
> In other words, I'm ok with better refcount checking but need convincing
> that we need to do more than what a simple ASSERT would provide.

If it doesn't get in your way, and it really is reference counting
(which it sounds like it is), we should be using refcount_t. The kinds
of bugs we've seen over the last decade continually prove that we'll
see refcount issues where we least expect it, and we need to catch
them.

Thanks!

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 0/5] fs, xfs refcount conversions

2017-04-20 Thread Darrick J. Wong 
On Thu, Apr 20, 2017 at 10:24:04AM -0700, Darrick J. Wong wrote:
> On Thu, Apr 20, 2017 at 08:11:41AM +, Reshetova, Elena wrote:
> > 
> >  
> > > v3:
> > >  * fixed header file inclusion
> > 
> > I don't think I have heard anything back on this v3 patch set. 
> > Is there still smth here to fix or could you take the changes in?
> 
> Generally I think it looks ok; it's running through shakedown testing as
> we speak.

Well, it survives the shakedown testing all right, but I can't shake the
feeling that this is overkill.  The xfs_{ef,bu,cu,ru}i_log_item
structures should only ever be referenced by the log itself and the
log-update-done log item: the refcount should only ever go 2 -> 1 -> 0.
We set the refcount to 2 and never increment it.

Given that I've seen occasional refcounting problems with the log intent
items, I think it would suffice to ASSERT in xfs_*i_release that the
refcount isn't already zero.  Using ASSERT is particularly useful
because ASSERT compiles out in release builds.

As for the xlog_ticket, we increment its refcount as part of duplicating
a transaction as a part of committing one transaction (which decrements
the xlog_ticket's refcount) and reusing the log reservation to create a
new transaction.  In this case the refcounting already seems to have
sufficient non-zero checks, and since only one thread can hold a
transaction at any given time, I don't see how overflow protection helps
here.

In other words, I'm ok with better refcount checking but need convincing
that we need to do more than what a simple ASSERT would provide.

--D

> 
> --D
> 
> > 
> > Best Regards,
> > Elena.
> > 
> > > 
> > > Now when new refcount_t type and API are finally merged
> > > (see include/linux/refcount.h), the following
> > > patches convert various refcounters in the xfs susystem from atomic_t
> > > to refcount_t. By doing this we prevent intentional or accidental
> > > underflows or overflows that can led to use-after-free vulnerabilities.
> > > 
> > > 
> > > Elena Reshetova (5):
> > >   fs, xfs: convert xfs_bui_log_item.bui_refcount from atomic_t to
> > > refcount_t
> > >   fs, xfs: convert xfs_efi_log_item.efi_refcount from atomic_t to
> > > refcount_t
> > >   fs, xfs: convert xlog_ticket.t_ref from atomic_t to refcount_t
> > >   fs, xfs: convert xfs_cui_log_item.cui_refcount from atomic_t to
> > > refcount_t
> > >   fs, xfs: convert xfs_rui_log_item.rui_refcount from atomic_t to
> > > refcount_t
> > > 
> > >  fs/xfs/xfs_bmap_item.c |  4 ++--
> > >  fs/xfs/xfs_bmap_item.h |  2 +-
> > >  fs/xfs/xfs_extfree_item.c  |  4 ++--
> > >  fs/xfs/xfs_extfree_item.h  |  2 +-
> > >  fs/xfs/xfs_linux.h |  1 +
> > >  fs/xfs/xfs_log.c   | 10 +-
> > >  fs/xfs/xfs_log_priv.h  |  2 +-
> > >  fs/xfs/xfs_refcount_item.c |  4 ++--
> > >  fs/xfs/xfs_refcount_item.h |  2 +-
> > >  fs/xfs/xfs_rmap_item.c |  4 ++--
> > >  fs/xfs/xfs_rmap_item.h |  2 +-
> > >  11 files changed, 19 insertions(+), 18 deletions(-)
> > > 
> > > --
> > > 2.7.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majord...@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] fs, xfs refcount conversions

2017-04-20 Thread Darrick J. Wong 
On Thu, Apr 20, 2017 at 10:24:04AM -0700, Darrick J. Wong wrote:
> On Thu, Apr 20, 2017 at 08:11:41AM +, Reshetova, Elena wrote:
> > 
> >  
> > > v3:
> > >  * fixed header file inclusion
> > 
> > I don't think I have heard anything back on this v3 patch set. 
> > Is there still smth here to fix or could you take the changes in?
> 
> Generally I think it looks ok; it's running through shakedown testing as
> we speak.

Well, it survives the shakedown testing all right, but I can't shake the
feeling that this is overkill.  The xfs_{ef,bu,cu,ru}i_log_item
structures should only ever be referenced by the log itself and the
log-update-done log item: the refcount should only ever go 2 -> 1 -> 0.
We set the refcount to 2 and never increment it.

Given that I've seen occasional refcounting problems with the log intent
items, I think it would suffice to ASSERT in xfs_*i_release that the
refcount isn't already zero.  Using ASSERT is particularly useful
because ASSERT compiles out in release builds.

As for the xlog_ticket, we increment its refcount as part of duplicating
a transaction as a part of committing one transaction (which decrements
the xlog_ticket's refcount) and reusing the log reservation to create a
new transaction.  In this case the refcounting already seems to have
sufficient non-zero checks, and since only one thread can hold a
transaction at any given time, I don't see how overflow protection helps
here.

In other words, I'm ok with better refcount checking but need convincing
that we need to do more than what a simple ASSERT would provide.

--D

> 
> --D
> 
> > 
> > Best Regards,
> > Elena.
> > 
> > > 
> > > Now when new refcount_t type and API are finally merged
> > > (see include/linux/refcount.h), the following
> > > patches convert various refcounters in the xfs susystem from atomic_t
> > > to refcount_t. By doing this we prevent intentional or accidental
> > > underflows or overflows that can led to use-after-free vulnerabilities.
> > > 
> > > 
> > > Elena Reshetova (5):
> > >   fs, xfs: convert xfs_bui_log_item.bui_refcount from atomic_t to
> > > refcount_t
> > >   fs, xfs: convert xfs_efi_log_item.efi_refcount from atomic_t to
> > > refcount_t
> > >   fs, xfs: convert xlog_ticket.t_ref from atomic_t to refcount_t
> > >   fs, xfs: convert xfs_cui_log_item.cui_refcount from atomic_t to
> > > refcount_t
> > >   fs, xfs: convert xfs_rui_log_item.rui_refcount from atomic_t to
> > > refcount_t
> > > 
> > >  fs/xfs/xfs_bmap_item.c |  4 ++--
> > >  fs/xfs/xfs_bmap_item.h |  2 +-
> > >  fs/xfs/xfs_extfree_item.c  |  4 ++--
> > >  fs/xfs/xfs_extfree_item.h  |  2 +-
> > >  fs/xfs/xfs_linux.h |  1 +
> > >  fs/xfs/xfs_log.c   | 10 +-
> > >  fs/xfs/xfs_log_priv.h  |  2 +-
> > >  fs/xfs/xfs_refcount_item.c |  4 ++--
> > >  fs/xfs/xfs_refcount_item.h |  2 +-
> > >  fs/xfs/xfs_rmap_item.c |  4 ++--
> > >  fs/xfs/xfs_rmap_item.h |  2 +-
> > >  11 files changed, 19 insertions(+), 18 deletions(-)
> > > 
> > > --
> > > 2.7.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majord...@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] fs, xfs refcount conversions

2017-04-20 Thread Darrick J. Wong 
On Thu, Apr 20, 2017 at 08:11:41AM +, Reshetova, Elena wrote:
> 
>  
> > v3:
> >  * fixed header file inclusion
> 
> I don't think I have heard anything back on this v3 patch set. 
> Is there still smth here to fix or could you take the changes in?

Generally I think it looks ok; it's running through shakedown testing as
we speak.

--D

> 
> Best Regards,
> Elena.
> 
> > 
> > Now when new refcount_t type and API are finally merged
> > (see include/linux/refcount.h), the following
> > patches convert various refcounters in the xfs susystem from atomic_t
> > to refcount_t. By doing this we prevent intentional or accidental
> > underflows or overflows that can led to use-after-free vulnerabilities.
> > 
> > 
> > Elena Reshetova (5):
> >   fs, xfs: convert xfs_bui_log_item.bui_refcount from atomic_t to
> > refcount_t
> >   fs, xfs: convert xfs_efi_log_item.efi_refcount from atomic_t to
> > refcount_t
> >   fs, xfs: convert xlog_ticket.t_ref from atomic_t to refcount_t
> >   fs, xfs: convert xfs_cui_log_item.cui_refcount from atomic_t to
> > refcount_t
> >   fs, xfs: convert xfs_rui_log_item.rui_refcount from atomic_t to
> > refcount_t
> > 
> >  fs/xfs/xfs_bmap_item.c |  4 ++--
> >  fs/xfs/xfs_bmap_item.h |  2 +-
> >  fs/xfs/xfs_extfree_item.c  |  4 ++--
> >  fs/xfs/xfs_extfree_item.h  |  2 +-
> >  fs/xfs/xfs_linux.h |  1 +
> >  fs/xfs/xfs_log.c   | 10 +-
> >  fs/xfs/xfs_log_priv.h  |  2 +-
> >  fs/xfs/xfs_refcount_item.c |  4 ++--
> >  fs/xfs/xfs_refcount_item.h |  2 +-
> >  fs/xfs/xfs_rmap_item.c |  4 ++--
> >  fs/xfs/xfs_rmap_item.h |  2 +-
> >  11 files changed, 19 insertions(+), 18 deletions(-)
> > 
> > --
> > 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] fs, xfs refcount conversions

2017-04-20 Thread Darrick J. Wong 
On Thu, Apr 20, 2017 at 08:11:41AM +, Reshetova, Elena wrote:
> 
>  
> > v3:
> >  * fixed header file inclusion
> 
> I don't think I have heard anything back on this v3 patch set. 
> Is there still smth here to fix or could you take the changes in?

Generally I think it looks ok; it's running through shakedown testing as
we speak.

--D

> 
> Best Regards,
> Elena.
> 
> > 
> > Now when new refcount_t type and API are finally merged
> > (see include/linux/refcount.h), the following
> > patches convert various refcounters in the xfs susystem from atomic_t
> > to refcount_t. By doing this we prevent intentional or accidental
> > underflows or overflows that can led to use-after-free vulnerabilities.
> > 
> > 
> > Elena Reshetova (5):
> >   fs, xfs: convert xfs_bui_log_item.bui_refcount from atomic_t to
> > refcount_t
> >   fs, xfs: convert xfs_efi_log_item.efi_refcount from atomic_t to
> > refcount_t
> >   fs, xfs: convert xlog_ticket.t_ref from atomic_t to refcount_t
> >   fs, xfs: convert xfs_cui_log_item.cui_refcount from atomic_t to
> > refcount_t
> >   fs, xfs: convert xfs_rui_log_item.rui_refcount from atomic_t to
> > refcount_t
> > 
> >  fs/xfs/xfs_bmap_item.c |  4 ++--
> >  fs/xfs/xfs_bmap_item.h |  2 +-
> >  fs/xfs/xfs_extfree_item.c  |  4 ++--
> >  fs/xfs/xfs_extfree_item.h  |  2 +-
> >  fs/xfs/xfs_linux.h |  1 +
> >  fs/xfs/xfs_log.c   | 10 +-
> >  fs/xfs/xfs_log_priv.h  |  2 +-
> >  fs/xfs/xfs_refcount_item.c |  4 ++--
> >  fs/xfs/xfs_refcount_item.h |  2 +-
> >  fs/xfs/xfs_rmap_item.c |  4 ++--
> >  fs/xfs/xfs_rmap_item.h |  2 +-
> >  11 files changed, 19 insertions(+), 18 deletions(-)
> > 
> > --
> > 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 0/5] fs, xfs refcount conversions

2017-04-20 Thread Reshetova, Elena 

 
> v3:
>  * fixed header file inclusion

I don't think I have heard anything back on this v3 patch set. 
Is there still smth here to fix or could you take the changes in?

Best Regards,
Elena.

> 
> Now when new refcount_t type and API are finally merged
> (see include/linux/refcount.h), the following
> patches convert various refcounters in the xfs susystem from atomic_t
> to refcount_t. By doing this we prevent intentional or accidental
> underflows or overflows that can led to use-after-free vulnerabilities.
> 
> 
> Elena Reshetova (5):
>   fs, xfs: convert xfs_bui_log_item.bui_refcount from atomic_t to
> refcount_t
>   fs, xfs: convert xfs_efi_log_item.efi_refcount from atomic_t to
> refcount_t
>   fs, xfs: convert xlog_ticket.t_ref from atomic_t to refcount_t
>   fs, xfs: convert xfs_cui_log_item.cui_refcount from atomic_t to
> refcount_t
>   fs, xfs: convert xfs_rui_log_item.rui_refcount from atomic_t to
> refcount_t
> 
>  fs/xfs/xfs_bmap_item.c |  4 ++--
>  fs/xfs/xfs_bmap_item.h |  2 +-
>  fs/xfs/xfs_extfree_item.c  |  4 ++--
>  fs/xfs/xfs_extfree_item.h  |  2 +-
>  fs/xfs/xfs_linux.h |  1 +
>  fs/xfs/xfs_log.c   | 10 +-
>  fs/xfs/xfs_log_priv.h  |  2 +-
>  fs/xfs/xfs_refcount_item.c |  4 ++--
>  fs/xfs/xfs_refcount_item.h |  2 +-
>  fs/xfs/xfs_rmap_item.c |  4 ++--
>  fs/xfs/xfs_rmap_item.h |  2 +-
>  11 files changed, 19 insertions(+), 18 deletions(-)
> 
> --
> 2.7.4

RE: [PATCH 0/5] fs, xfs refcount conversions

2017-04-20 Thread Reshetova, Elena 

 
> v3:
>  * fixed header file inclusion

I don't think I have heard anything back on this v3 patch set. 
Is there still smth here to fix or could you take the changes in?

Best Regards,
Elena.

> 
> Now when new refcount_t type and API are finally merged
> (see include/linux/refcount.h), the following
> patches convert various refcounters in the xfs susystem from atomic_t
> to refcount_t. By doing this we prevent intentional or accidental
> underflows or overflows that can led to use-after-free vulnerabilities.
> 
> 
> Elena Reshetova (5):
>   fs, xfs: convert xfs_bui_log_item.bui_refcount from atomic_t to
> refcount_t
>   fs, xfs: convert xfs_efi_log_item.efi_refcount from atomic_t to
> refcount_t
>   fs, xfs: convert xlog_ticket.t_ref from atomic_t to refcount_t
>   fs, xfs: convert xfs_cui_log_item.cui_refcount from atomic_t to
> refcount_t
>   fs, xfs: convert xfs_rui_log_item.rui_refcount from atomic_t to
> refcount_t
> 
>  fs/xfs/xfs_bmap_item.c |  4 ++--
>  fs/xfs/xfs_bmap_item.h |  2 +-
>  fs/xfs/xfs_extfree_item.c  |  4 ++--
>  fs/xfs/xfs_extfree_item.h  |  2 +-
>  fs/xfs/xfs_linux.h |  1 +
>  fs/xfs/xfs_log.c   | 10 +-
>  fs/xfs/xfs_log_priv.h  |  2 +-
>  fs/xfs/xfs_refcount_item.c |  4 ++--
>  fs/xfs/xfs_refcount_item.h |  2 +-
>  fs/xfs/xfs_rmap_item.c |  4 ++--
>  fs/xfs/xfs_rmap_item.h |  2 +-
>  11 files changed, 19 insertions(+), 18 deletions(-)
> 
> --
> 2.7.4