[PATCH 0/5] fs, xfs refcount conversions

[PATCH 0/5] fs, xfs refcount conversions

[Elena Reshetova]

v3:
 * fixed header file inclusion

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

[PATCH 1/5] fs, xfs: convert xfs_bui_log_item.bui_refcount from atomic_t to refcount_t

[Elena Reshetova]

refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
 fs/xfs/xfs_bmap_item.c | 4 ++--
 fs/xfs/xfs_bmap_item.h | 2 +-
 fs/xfs/xfs_linux.h     | 1 +
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index 9bf57c7..33104ad 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -199,7 +199,7 @@ xfs_bui_init(
 	buip->bui_format.bui_nextents = XFS_BUI_MAX_FAST_EXTENTS;
 	buip->bui_format.bui_id = (uintptr_t)(void *)buip;
 	atomic_set(&buip->bui_next_extent, 0);
-	atomic_set(&buip->bui_refcount, 2);
+	refcount_set(&buip->bui_refcount, 2);
 
 	return buip;
 }
@@ -215,7 +215,7 @@ void
 xfs_bui_release(
 	struct xfs_bui_log_item	*buip)
 {
-	if (atomic_dec_and_test(&buip->bui_refcount)) {
+	if (refcount_dec_and_test(&buip->bui_refcount)) {
 		xfs_trans_ail_remove(&buip->bui_item, SHUTDOWN_LOG_IO_ERROR);
 		xfs_bui_item_free(buip);
 	}
diff --git a/fs/xfs/xfs_bmap_item.h b/fs/xfs/xfs_bmap_item.h
index c867daa..7048b14 100644
--- a/fs/xfs/xfs_bmap_item.h
+++ b/fs/xfs/xfs_bmap_item.h
@@ -61,7 +61,7 @@ struct kmem_zone;
  */
 struct xfs_bui_log_item {
 	struct xfs_log_item		bui_item;
-	atomic_t			bui_refcount;
+	refcount_t			bui_refcount;
 	atomic_t			bui_next_extent;
 	unsigned long			bui_flags;	/* misc flags */
 	struct xfs_bui_log_format	bui_format;
diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
index 7a989de..09b0396 100644
--- a/fs/xfs/xfs_linux.h
+++ b/fs/xfs/xfs_linux.h
@@ -19,6 +19,7 @@
 #define __XFS_LINUX__
 
 #include <linux/types.h>
+#include <linux/refcount.h>
 
 /*
  * Kernel specific type declarations for XFS
-- 
2.7.4

[PATCH 2/5] fs, xfs: convert xfs_efi_log_item.efi_refcount from atomic_t to refcount_t

[Elena Reshetova]

refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
 fs/xfs/xfs_extfree_item.c | 4 ++--
 fs/xfs/xfs_extfree_item.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index d7bc149..4e0acf0 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -220,7 +220,7 @@ xfs_efi_init(
 	efip->efi_format.efi_nextents = nextents;
 	efip->efi_format.efi_id = (uintptr_t)(void *)efip;
 	atomic_set(&efip->efi_next_extent, 0);
-	atomic_set(&efip->efi_refcount, 2);
+	refcount_set(&efip->efi_refcount, 2);
 
 	return efip;
 }
@@ -290,7 +290,7 @@ void
 xfs_efi_release(
 	struct xfs_efi_log_item	*efip)
 {
-	if (atomic_dec_and_test(&efip->efi_refcount)) {
+	if (refcount_dec_and_test(&efip->efi_refcount)) {
 		xfs_trans_ail_remove(&efip->efi_item, SHUTDOWN_LOG_IO_ERROR);
 		xfs_efi_item_free(efip);
 	}
diff --git a/fs/xfs/xfs_extfree_item.h b/fs/xfs/xfs_extfree_item.h
index a32c794..fadf736 100644
--- a/fs/xfs/xfs_extfree_item.h
+++ b/fs/xfs/xfs_extfree_item.h
@@ -64,7 +64,7 @@ struct kmem_zone;
  */
 typedef struct xfs_efi_log_item {
 	xfs_log_item_t		efi_item;
-	atomic_t		efi_refcount;
+	refcount_t		efi_refcount;
 	atomic_t		efi_next_extent;
 	unsigned long		efi_flags;	/* misc flags */
 	xfs_efi_log_format_t	efi_format;
-- 
2.7.4

[PATCH 3/5] fs, xfs: convert xlog_ticket.t_ref from atomic_t to refcount_t

[Elena Reshetova]

refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
 fs/xfs/xfs_log.c      | 10 +++++-----
 fs/xfs/xfs_log_priv.h |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index b1469f0..c127fa0 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -3500,8 +3500,8 @@ void
 xfs_log_ticket_put(
 	xlog_ticket_t	*ticket)
 {
-	ASSERT(atomic_read(&ticket->t_ref) > 0);
-	if (atomic_dec_and_test(&ticket->t_ref))
+	ASSERT(refcount_read(&ticket->t_ref) > 0);
+	if (refcount_dec_and_test(&ticket->t_ref))
 		kmem_zone_free(xfs_log_ticket_zone, ticket);
 }
 
@@ -3509,8 +3509,8 @@ xlog_ticket_t *
 xfs_log_ticket_get(
 	xlog_ticket_t	*ticket)
 {
-	ASSERT(atomic_read(&ticket->t_ref) > 0);
-	atomic_inc(&ticket->t_ref);
+	ASSERT(refcount_read(&ticket->t_ref) > 0);
+	refcount_inc(&ticket->t_ref);
 	return ticket;
 }
 
@@ -3632,7 +3632,7 @@ xlog_ticket_alloc(
 
 	unit_res = xfs_log_calc_unit_res(log->l_mp, unit_bytes);
 
-	atomic_set(&tic->t_ref, 1);
+	refcount_set(&tic->t_ref, 1);
 	tic->t_task		= current;
 	INIT_LIST_HEAD(&tic->t_queue);
 	tic->t_unit_res		= unit_res;
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index c2604a5..3fc4aba 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -168,7 +168,7 @@ typedef struct xlog_ticket {
 	struct list_head   t_queue;	 /* reserve/write queue */
 	struct task_struct *t_task;	 /* task that owns this ticket */
 	xlog_tid_t	   t_tid;	 /* transaction identifier	 : 4  */
-	atomic_t	   t_ref;	 /* ticket reference count       : 4  */
+	refcount_t	   t_ref;	 /* ticket reference count       : 4  */
 	int		   t_curr_res;	 /* current reservation in bytes : 4  */
 	int		   t_unit_res;	 /* unit reservation in bytes    : 4  */
 	char		   t_ocnt;	 /* original count		 : 1  */
-- 
2.7.4

Re: [PATCH 3/5] fs, xfs: convert xlog_ticket.t_ref from atomic_t to refcount_t

[Christoph Hellwig]

> -	ASSERT(atomic_read(&ticket->t_ref) > 0);
> -	atomic_inc(&ticket->t_ref);
> +	ASSERT(refcount_read(&ticket->t_ref) > 0);
> +	refcount_inc(&ticket->t_ref);

With strict refcount semantics refcount_inc should check that
the count is larger than 0, otherwise we'd need to use 
recount_inc_not_zero or whatever you're going to call it.

Is that something the recount code does / could do?

Re: [PATCH 3/5] fs, xfs: convert xlog_ticket.t_ref from atomic_t to refcount_t

[Kees Cook]

On Wed, Mar 8, 2017 at 7:50 AM, Christoph Hellwig <hch@infradead.org> wrote:
>> -     ASSERT(atomic_read(&ticket->t_ref) > 0);
>> -     atomic_inc(&ticket->t_ref);
>> +     ASSERT(refcount_read(&ticket->t_ref) > 0);
>> +     refcount_inc(&ticket->t_ref);
>
> With strict refcount semantics refcount_inc should check that
> the count is larger than 0, otherwise we'd need to use
> recount_inc_not_zero or whatever you're going to call it.
>
> Is that something the recount code does / could do?

Yes, refcount_inc() will not increment from 0 and WARNs. It looks like
xfs's ASSERT is also a warn (though with XFS-specific formatting), so
perhaps the ASSERT could be dropped? IIUC, Elena's approach to these
changes was to be conservative about removing the existing checks.

-Kees

-- 
Kees Cook
Pixel Security

RE: [PATCH 3/5] fs, xfs: convert xlog_ticket.t_ref from atomic_t to refcount_t

[Reshetova, Elena]

> On Wed, Mar 8, 2017 at 7:50 AM, Christoph Hellwig <hch@infradead.org> wrote:
> >> -     ASSERT(atomic_read(&ticket->t_ref) > 0);
> >> -     atomic_inc(&ticket->t_ref);
> >> +     ASSERT(refcount_read(&ticket->t_ref) > 0);
> >> +     refcount_inc(&ticket->t_ref);
> >
> > With strict refcount semantics refcount_inc should check that
> > the count is larger than 0, otherwise we'd need to use
> > recount_inc_not_zero or whatever you're going to call it.
> >
> > Is that something the recount code does / could do?
> 
> Yes, refcount_inc() will not increment from 0 and WARNs. It looks like
> xfs's ASSERT is also a warn (though with XFS-specific formatting), so
> perhaps the ASSERT could be dropped? IIUC, Elena's approach to these
> changes was to be conservative about removing the existing checks.

I am removing the existing WARNs now where they are 100% not needed, but leaving the ones like this ASSERT, 
because I am thinking that you might have test cases that are dependent on these ASSERTs with specific formatting and I don't want to break them. 
If you don't need them, I can remove them also. 

Best Regards,
Elena

> 
> -Kees
> 
> --
> Kees Cook
> Pixel Security

[PATCH 4/5] fs, xfs: convert xfs_cui_log_item.cui_refcount from atomic_t to refcount_t

[Elena Reshetova]

refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
 fs/xfs/xfs_refcount_item.c | 4 ++--
 fs/xfs/xfs_refcount_item.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index 6e4c744..61bc570 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -205,7 +205,7 @@ xfs_cui_init(
 	cuip->cui_format.cui_nextents = nextents;
 	cuip->cui_format.cui_id = (uintptr_t)(void *)cuip;
 	atomic_set(&cuip->cui_next_extent, 0);
-	atomic_set(&cuip->cui_refcount, 2);
+	refcount_set(&cuip->cui_refcount, 2);
 
 	return cuip;
 }
@@ -221,7 +221,7 @@ void
 xfs_cui_release(
 	struct xfs_cui_log_item	*cuip)
 {
-	if (atomic_dec_and_test(&cuip->cui_refcount)) {
+	if (refcount_dec_and_test(&cuip->cui_refcount)) {
 		xfs_trans_ail_remove(&cuip->cui_item, SHUTDOWN_LOG_IO_ERROR);
 		xfs_cui_item_free(cuip);
 	}
diff --git a/fs/xfs/xfs_refcount_item.h b/fs/xfs/xfs_refcount_item.h
index 5b74ddd..abc0377 100644
--- a/fs/xfs/xfs_refcount_item.h
+++ b/fs/xfs/xfs_refcount_item.h
@@ -63,7 +63,7 @@ struct kmem_zone;
  */
 struct xfs_cui_log_item {
 	struct xfs_log_item		cui_item;
-	atomic_t			cui_refcount;
+	refcount_t			cui_refcount;
 	atomic_t			cui_next_extent;
 	unsigned long			cui_flags;	/* misc flags */
 	struct xfs_cui_log_format	cui_format;
-- 
2.7.4

[PATCH 5/5] fs, xfs: convert xfs_rui_log_item.rui_refcount from atomic_t to refcount_t

[Elena Reshetova]

refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
 fs/xfs/xfs_rmap_item.c | 4 ++--
 fs/xfs/xfs_rmap_item.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index 73c8278..5e1664a 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -204,7 +204,7 @@ xfs_rui_init(
 	ruip->rui_format.rui_nextents = nextents;
 	ruip->rui_format.rui_id = (uintptr_t)(void *)ruip;
 	atomic_set(&ruip->rui_next_extent, 0);
-	atomic_set(&ruip->rui_refcount, 2);
+	refcount_set(&ruip->rui_refcount, 2);
 
 	return ruip;
 }
@@ -243,7 +243,7 @@ void
 xfs_rui_release(
 	struct xfs_rui_log_item	*ruip)
 {
-	if (atomic_dec_and_test(&ruip->rui_refcount)) {
+	if (refcount_dec_and_test(&ruip->rui_refcount)) {
 		xfs_trans_ail_remove(&ruip->rui_item, SHUTDOWN_LOG_IO_ERROR);
 		xfs_rui_item_free(ruip);
 	}
diff --git a/fs/xfs/xfs_rmap_item.h b/fs/xfs/xfs_rmap_item.h
index 340c968..1e425a9 100644
--- a/fs/xfs/xfs_rmap_item.h
+++ b/fs/xfs/xfs_rmap_item.h
@@ -64,7 +64,7 @@ struct kmem_zone;
  */
 struct xfs_rui_log_item {
 	struct xfs_log_item		rui_item;
-	atomic_t			rui_refcount;
+	refcount_t			rui_refcount;
 	atomic_t			rui_next_extent;
 	unsigned long			rui_flags;	/* misc flags */
 	struct xfs_rui_log_format	rui_format;
-- 
2.7.4

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

[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

[Darrick J. Wong]

On Thu, Apr 20, 2017 at 08:11:41AM +0000, 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[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 +0000, 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 majordomo@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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Kees Cook]

On Thu, Apr 20, 2017 at 3:04 PM, Darrick J. Wong
<darrick.wong@oracle.com> wrote:
> On Thu, Apr 20, 2017 at 10:24:04AM -0700, Darrick J. Wong wrote:
>> On Thu, Apr 20, 2017 at 08:11:41AM +0000, 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