[PATCH 0/4] dm and bcache refcount conversions

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

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 <redacted>
Reviewed-by: David Windsor <redacted>
Reviewed-by: Hans Liljestrand <redacted>
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

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 <redacted>
Reviewed-by: David Windsor <redacted>
Reviewed-by: Hans Liljestrand <redacted>
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

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 <redacted>
Reviewed-by: David Windsor <redacted>
Reviewed-by: Hans Liljestrand <redacted>
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

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 <redacted>
Reviewed-by: David Windsor <redacted>
Reviewed-by: Hans Liljestrand <redacted>
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

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 <redacted>
Reviewed-by: David Windsor <redacted>
Reviewed-by: Hans Liljestrand <redacted>
Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>

Reviewed-by: Michael Lyle <redacted>

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

On 10/20/2017 12:37 AM, Elena Reshetova wrote:

quoted

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 <redacted>
Reviewed-by: David Windsor <redacted>
Reviewed-by: Hans Liljestrand <redacted>
Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>

Reviewed-by: Michael Lyle <redacted>

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

Dne 20.10.2017 v 09:37 Elena Reshetova napsal(a):

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

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

On Fri, Oct 20, 2017 at 10:37:38AM +0300, Elena Reshetova wrote:

quoted

 	} 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

Dne 20.10.2017 v 09:37 Elena Reshetova napsal(a):

quoted

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 <redacted>
Reviewed-by: David Windsor <redacted>
Reviewed-by: Hans Liljestrand <redacted>
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,

quoted

  		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

quoted

  	*result = &td->dm_dev;

@@ -701,7 +703,7 @@ void dm_put_table_device(struct mapped_device *md,

struct dm_dev *d)

quoted

  	struct table_device *td = container_of(d, struct table_device,

dm_dev);

quoted

  	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);

quoted

  		DMWARN("dm_destroy: %s still exists with %d

references",

quoted

-		       td->dm_dev.name, atomic_read(&td->count));
+		       td->dm_dev.name, refcount_read(&td->count));
  		kfree(td);
  	}
  }

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

On Fri, Nov 24, 2017 at 08:29:42AM +0000, Reshetova, Elena wrote:

quoted

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

On Fri, Nov 24, 2017 at 2:36 AM, Reshetova, Elena
[off-list ref] wrote:

quoted

On Fri, Oct 20, 2017 at 10:37:38AM +0300, Elena Reshetova wrote:

quoted

    } 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

On Fri, Nov 24, 2017 at 2:36 AM, Reshetova, Elena
[off-list ref] wrote:

quoted

quoted

On Fri, Oct 20, 2017 at 10:37:38AM +0300, Elena Reshetova wrote:

quoted

    } 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.

On Tue, Nov 28 2017 at  5:07am -0500,
Reshetova, Elena [off-list ref] wrote:

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

On Tue, Nov 28 2017 at  5:07am -0500,
Reshetova, Elena [off-list ref] wrote:

quoted

quoted

On Fri, Nov 24, 2017 at 2:36 AM, Reshetova, Elena
[off-list ref] wrote:

quoted

quoted

On Fri, Oct 20, 2017 at 10:37:38AM +0300, Elena Reshetova wrote:

quoted

    } 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

quoted

quoted

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

quoted

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

quoted

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,

quoted

 			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

quoted

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. 

quoted

This was the reason for the initial change I did, my mistake was just to forget to

increment it also

quoted

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),

quoted

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).

quoted

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