[PATCH 0/4] dm and bcache refcount conversions

[PATCH 0/4] dm and bcache refcount conversions

[Elena Reshetova]

This series, for dm and bcache parts, replaces atomic_t reference
counters with the new refcount_t type and API (see include/linux/refcount.h).
By doing this we prevent intentional or accidental
underflows or overflows that can led to use-after-free vulnerabilities.

The patches are fully independent and can be cherry-picked separately.
Patches are based on top of linux-next as of yesterday.
If there are no objections to the patches, please merge them via respective trees

Elena Reshetova (4):
  bcache: convert cached_dev.count from atomic_t to refcount_t
  dm cache: convert dm_cache_metadata.ref_count from atomic_t to
    refcount_t
  dm: convert dm_dev_internal.count from atomic_t to refcount_t
  dm: convert table_device.count from atomic_t to refcount_t

 drivers/md/bcache/bcache.h     |  7 ++++---
 drivers/md/bcache/super.c      |  6 +++---
 drivers/md/bcache/writeback.h  |  2 +-
 drivers/md/dm-cache-metadata.c |  9 +++++----
 drivers/md/dm-table.c          |  6 +++---
 drivers/md/dm.c                | 12 +++++++-----
 drivers/md/dm.h                |  3 ++-
 7 files changed, 25 insertions(+), 20 deletions(-)

-- 
2.7.4

[PATCH 1/4] bcache: convert cached_dev.count from atomic_t to refcount_t

[Elena Reshetova]

atomic_t variables are currently used to implement reference
counters with the following properties:
 - counter is initialized to 1 using atomic_set()
 - a resource is freed upon counter reaching zero
 - once counter reaches zero, its further
   increments aren't allowed
 - counter schema uses basic atomic operations
   (set, inc, inc_not_zero, dec_and_test, etc.)

Such atomic variables should be converted to a newly provided
refcount_t type and API that prevents accidental counter overflows
and underflows. This is important since overflows and underflows
can lead to use-after-free situation and be exploitable.

The variable cached_dev.count is used as pure reference counter.
Convert it to refcount_t and fix up the operations.

Suggested-by: Kees Cook <keescook@chromium.org>
Reviewed-by: David Windsor <dwindsor@gmail.com>
Reviewed-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
---
 drivers/md/bcache/bcache.h    | 7 ++++---
 drivers/md/bcache/super.c     | 6 +++---
 drivers/md/bcache/writeback.h | 2 +-
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
index 2ed9bd2..8ed8ad6 100644
--- a/drivers/md/bcache/bcache.h
+++ b/drivers/md/bcache/bcache.h
@@ -184,6 +184,7 @@
 #include <linux/mutex.h>
 #include <linux/rbtree.h>
 #include <linux/rwsem.h>
+#include <linux/refcount.h>
 #include <linux/types.h>
 #include <linux/workqueue.h>
 
@@ -299,7 +300,7 @@ struct cached_dev {
 	struct semaphore	sb_write_mutex;
 
 	/* Refcount on the cache set. Always nonzero when we're caching. */
-	atomic_t		count;
+	refcount_t		count;
 	struct work_struct	detach;
 
 	/*
@@ -806,13 +807,13 @@ do {									\
 
 static inline void cached_dev_put(struct cached_dev *dc)
 {
-	if (atomic_dec_and_test(&dc->count))
+	if (refcount_dec_and_test(&dc->count))
 		schedule_work(&dc->detach);
 }
 
 static inline bool cached_dev_get(struct cached_dev *dc)
 {
-	if (!atomic_inc_not_zero(&dc->count))
+	if (!refcount_inc_not_zero(&dc->count))
 		return false;
 
 	/* Paired with the mb in cached_dev_attach */
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index fc0a31b..676e6f4 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -889,7 +889,7 @@ static void cached_dev_detach_finish(struct work_struct *w)
 	closure_init_stack(&cl);
 
 	BUG_ON(!test_bit(BCACHE_DEV_DETACHING, &dc->disk.flags));
-	BUG_ON(atomic_read(&dc->count));
+	BUG_ON(refcount_read(&dc->count));
 
 	mutex_lock(&bch_register_lock);
 
@@ -1016,7 +1016,7 @@ int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c)
 	 * dc->c must be set before dc->count != 0 - paired with the mb in
 	 * cached_dev_get()
 	 */
-	atomic_set(&dc->count, 1);
+	refcount_set(&dc->count, 1);
 
 	/* Block writeback thread, but spawn it */
 	down_write(&dc->writeback_lock);
@@ -1028,7 +1028,7 @@ int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c)
 	if (BDEV_STATE(&dc->sb) == BDEV_STATE_DIRTY) {
 		bch_sectors_dirty_init(&dc->disk);
 		atomic_set(&dc->has_dirty, 1);
-		atomic_inc(&dc->count);
+		refcount_inc(&dc->count);
 		bch_writeback_queue(dc);
 	}
 
diff --git a/drivers/md/bcache/writeback.h b/drivers/md/bcache/writeback.h
index e35421d..5937e0d 100644
--- a/drivers/md/bcache/writeback.h
+++ b/drivers/md/bcache/writeback.h
@@ -89,7 +89,7 @@ static inline void bch_writeback_add(struct cached_dev *dc)
 {
 	if (!atomic_read(&dc->has_dirty) &&
 	    !atomic_xchg(&dc->has_dirty, 1)) {
-		atomic_inc(&dc->count);
+		refcount_inc(&dc->count);
 
 		if (BDEV_STATE(&dc->sb) != BDEV_STATE_DIRTY) {
 			SET_BDEV_STATE(&dc->sb, BDEV_STATE_DIRTY);
-- 
2.7.4

Re: [PATCH 1/4] bcache: convert cached_dev.count from atomic_t to refcount_t

[Michael Lyle]

On 10/20/2017 12:37 AM, Elena Reshetova wrote:
> atomic_t variables are currently used to implement reference
> counters with the following properties:
>  - counter is initialized to 1 using atomic_set()
>  - a resource is freed upon counter reaching zero
>  - once counter reaches zero, its further
>    increments aren't allowed
>  - counter schema uses basic atomic operations
>    (set, inc, inc_not_zero, dec_and_test, etc.)
> 
> Such atomic variables should be converted to a newly provided
> refcount_t type and API that prevents accidental counter overflows
> and underflows. This is important since overflows and underflows
> can lead to use-after-free situation and be exploitable.
> 
> The variable cached_dev.count is used as pure reference counter.
> Convert it to refcount_t and fix up the operations.
> 
> Suggested-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: David Windsor <dwindsor@gmail.com>
> Reviewed-by: Hans Liljestrand <ishkamiel@gmail.com>
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>

Reviewed-by: Michael Lyle <mlyle@lyle.org>

Thanks for this-- I'm including it in my tree for possible inclusion in
4.15 (I've already sent my main chunk of changes upwards).

Mike

RE: [PATCH 1/4] bcache: convert cached_dev.count from atomic_t to refcount_t

[Reshetova, Elena]

> On 10/20/2017 12:37 AM, Elena Reshetova wrote:
> > atomic_t variables are currently used to implement reference
> > counters with the following properties:
> >  - counter is initialized to 1 using atomic_set()
> >  - a resource is freed upon counter reaching zero
> >  - once counter reaches zero, its further
> >    increments aren't allowed
> >  - counter schema uses basic atomic operations
> >    (set, inc, inc_not_zero, dec_and_test, etc.)
> >
> > Such atomic variables should be converted to a newly provided
> > refcount_t type and API that prevents accidental counter overflows
> > and underflows. This is important since overflows and underflows
> > can lead to use-after-free situation and be exploitable.
> >
> > The variable cached_dev.count is used as pure reference counter.
> > Convert it to refcount_t and fix up the operations.
> >
> > Suggested-by: Kees Cook <keescook@chromium.org>
> > Reviewed-by: David Windsor <dwindsor@gmail.com>
> > Reviewed-by: Hans Liljestrand <ishkamiel@gmail.com>
> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> 
> Reviewed-by: Michael Lyle <mlyle@lyle.org>
> 
> Thanks for this-- I'm including it in my tree for possible inclusion in
> 4.15 (I've already sent my main chunk of changes upwards).

Thank you Mike! I am dropping then this patch from my list of maintained
conversions. 

Best Regards,
Elena.
> 
> Mike

[PATCH 2/4] dm cache: convert dm_cache_metadata.ref_count from atomic_t to refcount_t

[Elena Reshetova]

atomic_t variables are currently used to implement reference
counters with the following properties:
 - counter is initialized to 1 using atomic_set()
 - a resource is freed upon counter reaching zero
 - once counter reaches zero, its further
   increments aren't allowed
 - counter schema uses basic atomic operations
   (set, inc, inc_not_zero, dec_and_test, etc.)

Such atomic variables should be converted to a newly provided
refcount_t type and API that prevents accidental counter overflows
and underflows. This is important since overflows and underflows
can lead to use-after-free situation and be exploitable.

The variable dm_cache_metadata.ref_count is used as pure reference counter.
Convert it to refcount_t and fix up the operations.

Suggested-by: Kees Cook <keescook@chromium.org>
Reviewed-by: David Windsor <dwindsor@gmail.com>
Reviewed-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
---
 drivers/md/dm-cache-metadata.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/md/dm-cache-metadata.c b/drivers/md/dm-cache-metadata.c
index 4a4e9c7..0d72124 100644
--- a/drivers/md/dm-cache-metadata.c
+++ b/drivers/md/dm-cache-metadata.c
@@ -13,6 +13,7 @@
 #include "persistent-data/dm-transaction-manager.h"
 
 #include <linux/device-mapper.h>
+#include <linux/refcount.h>
 
 /*----------------------------------------------------------------*/
 
@@ -100,7 +101,7 @@ struct cache_disk_superblock {
 } __packed;
 
 struct dm_cache_metadata {
-	atomic_t ref_count;
+	refcount_t ref_count;
 	struct list_head list;
 
 	unsigned version;
@@ -753,7 +754,7 @@ static struct dm_cache_metadata *metadata_open(struct block_device *bdev,
 	}
 
 	cmd->version = metadata_version;
-	atomic_set(&cmd->ref_count, 1);
+	refcount_set(&cmd->ref_count, 1);
 	init_rwsem(&cmd->root_lock);
 	cmd->bdev = bdev;
 	cmd->data_block_size = data_block_size;
@@ -791,7 +792,7 @@ static struct dm_cache_metadata *lookup(struct block_device *bdev)
 
 	list_for_each_entry(cmd, &table, list)
 		if (cmd->bdev == bdev) {
-			atomic_inc(&cmd->ref_count);
+			refcount_inc(&cmd->ref_count);
 			return cmd;
 		}
 
@@ -862,7 +863,7 @@ struct dm_cache_metadata *dm_cache_metadata_open(struct block_device *bdev,
 
 void dm_cache_metadata_close(struct dm_cache_metadata *cmd)
 {
-	if (atomic_dec_and_test(&cmd->ref_count)) {
+	if (refcount_dec_and_test(&cmd->ref_count)) {
 		mutex_lock(&table_lock);
 		list_del(&cmd->list);
 		mutex_unlock(&table_lock);
-- 
2.7.4

[PATCH 3/4] dm: convert dm_dev_internal.count from atomic_t to refcount_t

[Elena Reshetova]

atomic_t variables are currently used to implement reference
counters with the following properties:
 - counter is initialized to 1 using atomic_set()
 - a resource is freed upon counter reaching zero
 - once counter reaches zero, its further
   increments aren't allowed
 - counter schema uses basic atomic operations
   (set, inc, inc_not_zero, dec_and_test, etc.)

Such atomic variables should be converted to a newly provided
refcount_t type and API that prevents accidental counter overflows
and underflows. This is important since overflows and underflows
can lead to use-after-free situation and be exploitable.

The variable dm_dev_internal.count is used as pure reference counter.
Convert it to refcount_t and fix up the operations.

Suggested-by: Kees Cook <keescook@chromium.org>
Reviewed-by: David Windsor <dwindsor@gmail.com>
Reviewed-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
---
 drivers/md/dm-table.c | 6 +++---
 drivers/md/dm.h       | 3 ++-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index ef7b8f2..fc7d240 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -451,15 +451,15 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
 			return r;
 		}
 
-		atomic_set(&dd->count, 0);
+		refcount_set(&dd->count, 1);
 		list_add(&dd->list, &t->devices);
 
 	} else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) {
 		r = upgrade_mode(dd, mode, t->md);
 		if (r)
 			return r;
+		refcount_inc(&dd->count);
 	}
-	atomic_inc(&dd->count);
 
 	*result = dd->dm_dev;
 	return 0;
@@ -515,7 +515,7 @@ void dm_put_device(struct dm_target *ti, struct dm_dev *d)
 		       dm_device_name(ti->table->md), d->name);
 		return;
 	}
-	if (atomic_dec_and_test(&dd->count)) {
+	if (refcount_dec_and_test(&dd->count)) {
 		dm_put_table_device(ti->table->md, d);
 		list_del(&dd->list);
 		kfree(dd);
diff --git a/drivers/md/dm.h b/drivers/md/dm.h
index 38c84c0..36399bb8 100644
--- a/drivers/md/dm.h
+++ b/drivers/md/dm.h
@@ -19,6 +19,7 @@
 #include <linux/hdreg.h>
 #include <linux/completion.h>
 #include <linux/kobject.h>
+#include <linux/refcount.h>
 
 #include "dm-stats.h"
 
@@ -38,7 +39,7 @@
  */
 struct dm_dev_internal {
 	struct list_head list;
-	atomic_t count;
+	refcount_t count;
 	struct dm_dev *dm_dev;
 };
 
-- 
2.7.4

Re: [dm-devel] [PATCH 3/4] dm: convert dm_dev_internal.count from atomic_t to refcount_t

[Alasdair G Kergon]

On Fri, Oct 20, 2017 at 10:37:38AM +0300, Elena Reshetova wrote:
>  	} else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) {
>  		r = upgrade_mode(dd, mode, t->md);
>  		if (r)
>  			return r;
> +		refcount_inc(&dd->count);
>  	}

Missing here:

        else
		refcount_inc(&dd->count);

?

Alasdair

RE: [dm-devel] [PATCH 3/4] dm: convert dm_dev_internal.count from atomic_t to refcount_t

[Reshetova, Elena]

> On Fri, Oct 20, 2017 at 10:37:38AM +0300, Elena Reshetova wrote:
> >  	} else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) {
> >  		r = upgrade_mode(dd, mode, t->md);
> >  		if (r)
> >  			return r;
> > +		refcount_inc(&dd->count);
> >  	}
> 
> Missing here:
> 
>         else
> 		refcount_inc(&dd->count);
> 
> ?

Oh, yes, thanks for catching this! I think this got unnoticed so far and patch was merged, so I am going to send a followup patch now. 

Best Regards,
Elena.

> 
> Alasdair

Re: [dm-devel] [PATCH 3/4] dm: convert dm_dev_internal.count from atomic_t to refcount_t

[Mike Snitzer]

On Fri, Nov 24, 2017 at 2:36 AM, Reshetova, Elena
<elena.reshetova@intel.com> wrote:
>> On Fri, Oct 20, 2017 at 10:37:38AM +0300, Elena Reshetova wrote:
>> >     } else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) {
>> >             r = upgrade_mode(dd, mode, t->md);
>> >             if (r)
>> >                     return r;
>> > +           refcount_inc(&dd->count);
>> >     }
>>
>> Missing here:
>>
>>         else
>>               refcount_inc(&dd->count);
>>
>> ?
>
> Oh, yes, thanks for catching this! I think this got unnoticed so far and patch was merged, so I am going to send a followup patch now.

I pushed this fix and will send to Linus next week:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.15&id=d908af82d06cc420f9581c97c6db941cb87e4434

RE: [dm-devel] [PATCH 3/4] dm: convert dm_dev_internal.count from atomic_t to refcount_t

[Reshetova, Elena]

[-- Attachment #1: Type: text/plain, Size: 2420 bytes --]


> On Fri, Nov 24, 2017 at 2:36 AM, Reshetova, Elena
> <elena.reshetova@intel.com> wrote:
> >> On Fri, Oct 20, 2017 at 10:37:38AM +0300, Elena Reshetova wrote:
> >> >     } else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) {
> >> >             r = upgrade_mode(dd, mode, t->md);
> >> >             if (r)
> >> >                     return r;
> >> > +           refcount_inc(&dd->count);
> >> >     }
> >>
> >> Missing here:
> >>
> >>         else
> >>               refcount_inc(&dd->count);
> >>
> >> ?
> >
> > Oh, yes, thanks for catching this! I think this got unnoticed so far and patch was
> merged, so I am going to send a followup patch now.
> 
> I pushed this fix and will send to Linus next week:
> https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-
> dm.git/commit/?h=dm-4.15&id=d908af82d06cc420f9581c97c6db941cb87e4434


I guess you mean this commit:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=c2318d07ead871f058dda62e942ed7b6b1c1cfcf

Unfortunately it is not correct:

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 88130b5..f6d32ee 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -451,15 +451,15 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
 			return r;
 		}
 
-		refcount_set(&dd->count, 1);
+		refcount_set(&dd->count, 0);
 		list_add(&dd->list, &t->devices);
 
 	} else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) {
 		r = upgrade_mode(dd, mode, t->md);
 		if (r)
 			return r;
-		refcount_inc(&dd->count);
 	}
+	refcount_inc(&dd->count);
 
Problem will be here if you hit this refcount_inc() after the refcount_set(&dd->count, 0) earlier. 
refcount_inc() does not increment on zero value *ever* for security reasons and instead people
should initialize refcounters to 1 always and do increments from there if needed. 
This was the reason for the initial change I did, my mistake was just to forget to increment it also
in case condition (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) fails. 

I have issues with my intel smpt server for sending patches (I will get it fixed tomorrow from internal network),
so I am attaching the patch I did end of last week to this thread instead (or alternatively can properly send it tomorrow after fix). 
Sorry for the delay!

Best Regards,
Elena.


[-- Attachment #2: 0001-dm-fix-the-missing-increment-for-dm_dev_internal.cou.patch --]
[-- Type: application/octet-stream, Size: 1210 bytes --]

From 715977ac0090cf3ee8ddc339c292434e20a9c93d Mon Sep 17 00:00:00 2001
From: Elena Reshetova <elena.reshetova@intel.com>
Date: Fri, 24 Nov 2017 10:02:48 +0200
Subject: [PATCH] dm: fix the missing increment for dm_dev_internal.count

The commit 2a0b4682e09d76466f7b8f5e347ae2ff02f033af
mistakenly forgotten one case where refcount must be
incremented. Fix this.

Reported-by: Alasdair G Kergon <agk@redhat.com>
Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
---
 drivers/md/dm-table.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 88130b5..eb0ba2a 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -453,14 +453,19 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
 
 		refcount_set(&dd->count, 1);
 		list_add(&dd->list, &t->devices);
+		*result = dd->dm_dev;
+		return 0;
+
+	}
 
-	} else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) {
+	if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) {
 		r = upgrade_mode(dd, mode, t->md);
 		if (r)
 			return r;
-		refcount_inc(&dd->count);
 	}
 
+	refcount_inc(&dd->count);
+
 	*result = dd->dm_dev;
 	return 0;
 }
-- 
2.7.4

Re: [PATCH 3/4] dm: convert dm_dev_internal.count from atomic_t to refcount_t

[Mike Snitzer]

On Tue, Nov 28 2017 at  5:07am -0500,
Reshetova, Elena <elena.reshetova@intel.com> wrote:

> 
> > On Fri, Nov 24, 2017 at 2:36 AM, Reshetova, Elena
> > <elena.reshetova@intel.com> wrote:
> > >> On Fri, Oct 20, 2017 at 10:37:38AM +0300, Elena Reshetova wrote:
> > >> >     } else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) {
> > >> >             r = upgrade_mode(dd, mode, t->md);
> > >> >             if (r)
> > >> >                     return r;
> > >> > +           refcount_inc(&dd->count);
> > >> >     }
> > >>
> > >> Missing here:
> > >>
> > >>         else
> > >>               refcount_inc(&dd->count);
> > >>
> > >> ?
> > >
> > > Oh, yes, thanks for catching this! I think this got unnoticed so far and patch was
> > merged, so I am going to send a followup patch now.
> > 
> > I pushed this fix and will send to Linus next week:
> > https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-
> > dm.git/commit/?h=dm-4.15&id=d908af82d06cc420f9581c97c6db941cb87e4434
> 
> 
> I guess you mean this commit:
> https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=c2318d07ead871f058dda62e942ed7b6b1c1cfcf
> 
> Unfortunately it is not correct:
> 
> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> index 88130b5..f6d32ee 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -451,15 +451,15 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
>  			return r;
>  		}
>  
> -		refcount_set(&dd->count, 1);
> +		refcount_set(&dd->count, 0);
>  		list_add(&dd->list, &t->devices);
>  
>  	} else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) {
>  		r = upgrade_mode(dd, mode, t->md);
>  		if (r)
>  			return r;
> -		refcount_inc(&dd->count);
>  	}
> +	refcount_inc(&dd->count);
>  
> Problem will be here if you hit this refcount_inc() after the refcount_set(&dd->count, 0) earlier. 
> refcount_inc() does not increment on zero value *ever* for security reasons and instead people
> should initialize refcounters to 1 always and do increments from there if needed. 

include/linux/refcount.h:refcount_inc() definitely doesn't avoid
incrementing zero value.

Neither does lib/refcount.c:refcount_inc() but it does spew a WARN_ON by
assuming a zero value means use-after-free.

> This was the reason for the initial change I did, my mistake was just to forget to increment it also
> in case condition (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) fails. 
> 
> I have issues with my intel smpt server for sending patches (I will get it fixed tomorrow from internal network),
> so I am attaching the patch I did end of last week to this thread instead (or alternatively can properly send it tomorrow after fix). 
> Sorry for the delay!

I was tempted to revert your original commits that switch DM code to
using refcount_t.  Already proved more trouble than it is worth.

But I'll drop my commit and take your fix.

Mike

RE: [PATCH 3/4] dm: convert dm_dev_internal.count from atomic_t to refcount_t

[Reshetova, Elena]

> On Tue, Nov 28 2017 at  5:07am -0500,
> Reshetova, Elena <elena.reshetova@intel.com> wrote:
> 
> >
> > > On Fri, Nov 24, 2017 at 2:36 AM, Reshetova, Elena
> > > <elena.reshetova@intel.com> wrote:
> > > >> On Fri, Oct 20, 2017 at 10:37:38AM +0300, Elena Reshetova wrote:
> > > >> >     } else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) {
> > > >> >             r = upgrade_mode(dd, mode, t->md);
> > > >> >             if (r)
> > > >> >                     return r;
> > > >> > +           refcount_inc(&dd->count);
> > > >> >     }
> > > >>
> > > >> Missing here:
> > > >>
> > > >>         else
> > > >>               refcount_inc(&dd->count);
> > > >>
> > > >> ?
> > > >
> > > > Oh, yes, thanks for catching this! I think this got unnoticed so far and patch
> was
> > > merged, so I am going to send a followup patch now.
> > >
> > > I pushed this fix and will send to Linus next week:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-
> > > dm.git/commit/?h=dm-
> 4.15&id=d908af82d06cc420f9581c97c6db941cb87e4434
> >
> >
> > I guess you mean this commit:
> > https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-
> dm.git/commit/?h=for-next&id=c2318d07ead871f058dda62e942ed7b6b1c1cfcf
> >
> > Unfortunately it is not correct:
> >
> > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> > index 88130b5..f6d32ee 100644
> > --- a/drivers/md/dm-table.c
> > +++ b/drivers/md/dm-table.c
> > @@ -451,15 +451,15 @@ int dm_get_device(struct dm_target *ti, const char
> *path, fmode_t mode,
> >  			return r;
> >  		}
> >
> > -		refcount_set(&dd->count, 1);
> > +		refcount_set(&dd->count, 0);
> >  		list_add(&dd->list, &t->devices);
> >
> >  	} else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) {
> >  		r = upgrade_mode(dd, mode, t->md);
> >  		if (r)
> >  			return r;
> > -		refcount_inc(&dd->count);
> >  	}
> > +	refcount_inc(&dd->count);
> >
> > Problem will be here if you hit this refcount_inc() after the refcount_set(&dd-
> >count, 0) earlier.
> > refcount_inc() does not increment on zero value *ever* for security reasons
> and instead people
> > should initialize refcounters to 1 always and do increments from there if
> needed.
> 
> include/linux/refcount.h:refcount_inc() definitely doesn't avoid
> incrementing zero value.

Ok, to be fully precise there are 3 different cases (depending on config options):

1) refcount_t = atomic_t and in this case yes, nothing prevents increment, but no
 protection is given, so we hope such cases are disabled for any distros that care about
security

2) CONFIG_FULL_REFCOUNT is on and refcount_t uses arch. independent implementation
In lib/refcount.c. In this case refcount_inc() won't increment from zero. It is really more
than just a WARN(), increment fails inside refcount_inc_not_zero() used underneath. 

3) arch. dependent implementation is used for refcount_t. Here different options are
possible based on how arch. decides to implement this. Currently we only have x86
one (arch/x86/include/asm/refcount.h) and it is indeed allows increments from zero to happen. 

So, what I described above was the worst case, but since we need the code to work reliably
in each case, we have to take it into account. 


> 
> Neither does lib/refcount.c:refcount_inc() but it does spew a WARN_ON by
> assuming a zero value means use-after-free.

No, it does not increment really, it is not just a WARN(). 
https://elixir.free-electrons.com/linux/latest/source/lib/refcount.c#L151
It uses refcount_inc_not_zero() underneath. 


> 
> > This was the reason for the initial change I did, my mistake was just to forget to
> increment it also
> > in case condition (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) fails.
> >
> > I have issues with my intel smpt server for sending patches (I will get it fixed
> tomorrow from internal network),
> > so I am attaching the patch I did end of last week to this thread instead (or
> alternatively can properly send it tomorrow after fix).
> > Sorry for the delay!
> 
> I was tempted to revert your original commits that switch DM code to
> using refcount_t.  Already proved more trouble than it is worth.
> 
> But I'll drop my commit and take your fix.

Thank you very much and sorry for the troubles!
Unfortunately none of us is free of mistakes, good that this one was caught so fast!

When it comes to value, it does provide security value for your code and makes
sure that your reference counters would not be a new target of many similar CVEs
we had in past around this. Overall each conversion matters since there is less and less
potential holes attackers can try to squeeze themselves!

Best Regards,
Elena.


> 
> Mike

[PATCH 4/4] dm: convert table_device.count from atomic_t to refcount_t

[Elena Reshetova]

atomic_t variables are currently used to implement reference
counters with the following properties:
 - counter is initialized to 1 using atomic_set()
 - a resource is freed upon counter reaching zero
 - once counter reaches zero, its further
   increments aren't allowed
 - counter schema uses basic atomic operations
   (set, inc, inc_not_zero, dec_and_test, etc.)

Such atomic variables should be converted to a newly provided
refcount_t type and API that prevents accidental counter overflows
and underflows. This is important since overflows and underflows
can lead to use-after-free situation and be exploitable.

The variable table_device.count is used as pure reference counter.
Convert it to refcount_t and fix up the operations.

Suggested-by: Kees Cook <keescook@chromium.org>
Reviewed-by: David Windsor <dwindsor@gmail.com>
Reviewed-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
---
 drivers/md/dm.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 4be8532..be12f3f 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -24,6 +24,7 @@
 #include <linux/delay.h>
 #include <linux/wait.h>
 #include <linux/pr.h>
+#include <linux/refcount.h>
 
 #define DM_MSG_PREFIX "core"
 
@@ -98,7 +99,7 @@ struct dm_md_mempools {
 
 struct table_device {
 	struct list_head list;
-	atomic_t count;
+	refcount_t count;
 	struct dm_dev dm_dev;
 };
 
@@ -685,10 +686,11 @@ int dm_get_table_device(struct mapped_device *md, dev_t dev, fmode_t mode,
 
 		format_dev_t(td->dm_dev.name, dev);
 
-		atomic_set(&td->count, 0);
+		refcount_set(&td->count, 1);
 		list_add(&td->list, &md->table_devices);
+	} else {
+		refcount_inc(&td->count);
 	}
-	atomic_inc(&td->count);
 	mutex_unlock(&md->table_devices_lock);
 
 	*result = &td->dm_dev;
@@ -701,7 +703,7 @@ void dm_put_table_device(struct mapped_device *md, struct dm_dev *d)
 	struct table_device *td = container_of(d, struct table_device, dm_dev);
 
 	mutex_lock(&md->table_devices_lock);
-	if (atomic_dec_and_test(&td->count)) {
+	if (refcount_dec_and_test(&td->count)) {
 		close_table_device(td, md);
 		list_del(&td->list);
 		kfree(td);
@@ -718,7 +720,7 @@ static void free_table_devices(struct list_head *devices)
 		struct table_device *td = list_entry(tmp, struct table_device, list);
 
 		DMWARN("dm_destroy: %s still exists with %d references",
-		       td->dm_dev.name, atomic_read(&td->count));
+		       td->dm_dev.name, refcount_read(&td->count));
 		kfree(td);
 	}
 }
-- 
2.7.4

Re: [PATCH 4/4] dm: convert table_device.count from atomic_t to refcount_t

[Zdenek Kabelac]

Dne 20.10.2017 v 09:37 Elena Reshetova napsal(a):
> atomic_t variables are currently used to implement reference
> counters with the following properties:
>   - counter is initialized to 1 using atomic_set()
>   - a resource is freed upon counter reaching zero
>   - once counter reaches zero, its further
>     increments aren't allowed
>   - counter schema uses basic atomic operations
>     (set, inc, inc_not_zero, dec_and_test, etc.)
> 
> Such atomic variables should be converted to a newly provided
> refcount_t type and API that prevents accidental counter overflows
> and underflows. This is important since overflows and underflows
> can lead to use-after-free situation and be exploitable.
> 
> The variable table_device.count is used as pure reference counter.
> Convert it to refcount_t and fix up the operations.
> 
> Suggested-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: David Windsor <dwindsor@gmail.com>
> Reviewed-by: Hans Liljestrand <ishkamiel@gmail.com>
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> ---
>   drivers/md/dm.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 4be8532..be12f3f 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -24,6 +24,7 @@
>   #include <linux/delay.h>
>   #include <linux/wait.h>
>   #include <linux/pr.h>
> +#include <linux/refcount.h>
>   
>   #define DM_MSG_PREFIX "core"
>   
> @@ -98,7 +99,7 @@ struct dm_md_mempools {
>   
>   struct table_device {
>   	struct list_head list;
> -	atomic_t count;
> +	refcount_t count;
>   	struct dm_dev dm_dev;
>   };
>   
> @@ -685,10 +686,11 @@ int dm_get_table_device(struct mapped_device *md, dev_t dev, fmode_t mode,
>   
>   		format_dev_t(td->dm_dev.name, dev);
>   
> -		atomic_set(&td->count, 0);
> +		refcount_set(&td->count, 1);
>   		list_add(&td->list, &md->table_devices);
> +	} else {
> +		refcount_inc(&td->count);
>   	}
> -	atomic_inc(&td->count);
>   	mutex_unlock(&md->table_devices_lock);
>   


NACK

This patch (2a0b4682e09d76466f7b8f5e347ae2ff02f033af) currently breaks 
accounting of opened devices.

I.e.   multisegment device  (target with 3 segments is not properly accounted)


Patch needs reworking and users of 'dm' and 4.15-rc0 kernel should rather 
switch back to 4.14 ATM as it's unclear which other parts can be affected.

Zdenek

>   	*result = &td->dm_dev;
> @@ -701,7 +703,7 @@ void dm_put_table_device(struct mapped_device *md, struct dm_dev *d)
>   	struct table_device *td = container_of(d, struct table_device, dm_dev);
>   
>   	mutex_lock(&md->table_devices_lock);
> -	if (atomic_dec_and_test(&td->count)) {
> +	if (refcount_dec_and_test(&td->count)) {
>   		close_table_device(td, md);
>   		list_del(&td->list);
>   		kfree(td);
> @@ -718,7 +720,7 @@ static void free_table_devices(struct list_head *devices)
>   		struct table_device *td = list_entry(tmp, struct table_device, list);
>   
>   		DMWARN("dm_destroy: %s still exists with %d references",
> -		       td->dm_dev.name, atomic_read(&td->count));
> +		       td->dm_dev.name, refcount_read(&td->count));
>   		kfree(td);
>   	}
>   }
> 

RE: [PATCH 4/4] dm: convert table_device.count from atomic_t to refcount_t

[Reshetova, Elena]

> Dne 20.10.2017 v 09:37 Elena Reshetova napsal(a):
> > atomic_t variables are currently used to implement reference
> > counters with the following properties:
> >   - counter is initialized to 1 using atomic_set()
> >   - a resource is freed upon counter reaching zero
> >   - once counter reaches zero, its further
> >     increments aren't allowed
> >   - counter schema uses basic atomic operations
> >     (set, inc, inc_not_zero, dec_and_test, etc.)
> >
> > Such atomic variables should be converted to a newly provided
> > refcount_t type and API that prevents accidental counter overflows
> > and underflows. This is important since overflows and underflows
> > can lead to use-after-free situation and be exploitable.
> >
> > The variable table_device.count is used as pure reference counter.
> > Convert it to refcount_t and fix up the operations.
> >
> > Suggested-by: Kees Cook <keescook@chromium.org>
> > Reviewed-by: David Windsor <dwindsor@gmail.com>
> > Reviewed-by: Hans Liljestrand <ishkamiel@gmail.com>
> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > ---
> >   drivers/md/dm.c | 12 +++++++-----
> >   1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> > index 4be8532..be12f3f 100644
> > --- a/drivers/md/dm.c
> > +++ b/drivers/md/dm.c
> > @@ -24,6 +24,7 @@
> >   #include <linux/delay.h>
> >   #include <linux/wait.h>
> >   #include <linux/pr.h>
> > +#include <linux/refcount.h>
> >
> >   #define DM_MSG_PREFIX "core"
> >
> > @@ -98,7 +99,7 @@ struct dm_md_mempools {
> >
> >   struct table_device {
> >   	struct list_head list;
> > -	atomic_t count;
> > +	refcount_t count;
> >   	struct dm_dev dm_dev;
> >   };
> >
> > @@ -685,10 +686,11 @@ int dm_get_table_device(struct mapped_device *md,
> dev_t dev, fmode_t mode,
> >
> >   		format_dev_t(td->dm_dev.name, dev);
> >
> > -		atomic_set(&td->count, 0);
> > +		refcount_set(&td->count, 1);
> >   		list_add(&td->list, &md->table_devices);
> > +	} else {
> > +		refcount_inc(&td->count);
> >   	}
> > -	atomic_inc(&td->count);
> >   	mutex_unlock(&md->table_devices_lock);
> >
> 
> 
> NACK
> 
> This patch (2a0b4682e09d76466f7b8f5e347ae2ff02f033af) currently breaks
> accounting of opened devices.
> 
> I.e.   multisegment device  (target with 3 segments is not properly accounted)

Could you please explain what exactly happens (i.e. missing/wrong increment?)
or provide a error dump?
By looking at the code, I don't see where the change in the reference counting
could have caused this. 

Best Regards,
Elena.

> 
> 
> Patch needs reworking and users of 'dm' and 4.15-rc0 kernel should rather
> switch back to 4.14 ATM as it's unclear which other parts can be affected.
> 
> Zdenek
> 
> >   	*result = &td->dm_dev;
> > @@ -701,7 +703,7 @@ void dm_put_table_device(struct mapped_device *md,
> struct dm_dev *d)
> >   	struct table_device *td = container_of(d, struct table_device,
> dm_dev);
> >
> >   	mutex_lock(&md->table_devices_lock);
> > -	if (atomic_dec_and_test(&td->count)) {
> > +	if (refcount_dec_and_test(&td->count)) {
> >   		close_table_device(td, md);
> >   		list_del(&td->list);
> >   		kfree(td);
> > @@ -718,7 +720,7 @@ static void free_table_devices(struct list_head *devices)
> >   		struct table_device *td = list_entry(tmp, struct
> table_device, list);
> >
> >   		DMWARN("dm_destroy: %s still exists with %d
> references",
> > -		       td->dm_dev.name, atomic_read(&td->count));
> > +		       td->dm_dev.name, refcount_read(&td->count));
> >   		kfree(td);
> >   	}
> >   }
> >

Re: [PATCH 4/4] dm: convert table_device.count from atomic_t to refcount_t

[Alasdair G Kergon]

On Fri, Nov 24, 2017 at 08:29:42AM +0000, Reshetova, Elena wrote:
> By looking at the code, I don't see where the change in the reference counting
> could have caused this. 
 
The cause was the bug I identified in patch 3, not this patch.

The regression is easily hit - tables that reference the same underlying device
more than once are very common.

Alasdair

RE: [PATCH 4/4] dm: convert table_device.count from atomic_t to refcount_t

[Reshetova, Elena]

> On Fri, Nov 24, 2017 at 08:29:42AM +0000, Reshetova, Elena wrote:
> > By looking at the code, I don't see where the change in the reference counting
> > could have caused this.
> 
> The cause was the bug I identified in patch 3, not this patch.

Oh, ok, because I could not figure an issue with this patch. 
I will send the fixing patch shortly. 

Best Regards,
Elena.

> 
> The regression is easily hit - tables that reference the same underlying device
> more than once are very common.
> 
> Alasdair