[PATCH 1/2] Fail I/O to thin pool devices

[PATCH 1/2] Fail I/O to thin pool devices

[Demi Marie Obenour]

A thin pool device currently just passes all I/O to its origin device,
but this is a footgun: the user might not realize that tools that
operate on thin pool metadata must operate on the metadata volume.  This
could have security implications.

Fix this by failing all I/O to thin pool devices.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
 drivers/md/dm-thin.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
index 64cfcf46881dc5d87d5dfdb5650ba9babd32cd31..d85fdbd782ae5426003c99a4b4bf53818cc85efa 100644
--- a/drivers/md/dm-thin.c
+++ b/drivers/md/dm-thin.c
@@ -3405,19 +3405,14 @@ static int pool_ctr(struct dm_target *ti, unsigned argc, char **argv)
 
 static int pool_map(struct dm_target *ti, struct bio *bio)
 {
-	int r;
-	struct pool_c *pt = ti->private;
-	struct pool *pool = pt->pool;
-
 	/*
-	 * As this is a singleton target, ti->begin is always zero.
+	 * Previously, access to the pool was passed down to the origin device.
+	 * However, this turns out to be error-prone: if the user runs any of
+	 * the thin tools on the pool device, the tools could wind up parsing
+	 * potentially attacker-controlled data.  This mistake has actually
+	 * happened in practice.  Therefore, fail all I/O on the pool device.
 	 */
-	spin_lock_irq(&pool->lock);
-	bio_set_dev(bio, pt->data_dev->bdev);
-	r = DM_MAPIO_REMAPPED;
-	spin_unlock_irq(&pool->lock);
-
-	return r;
+	return -EIO;
 }
 
 static int maybe_resize_data_dev(struct dm_target *ti, bool *need_commit)
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[PATCH 2/2] dm-thin: Allow specifying an offset

[Demi Marie Obenour]

This allows exposing only part of a thin volume without having to layer
dm-linear.  One use-case is a hypervisor replacing a partition table.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
 drivers/md/dm-thin.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
index d85fdbd782ae5426003c99a4b4bf53818cc85efa..87f14933375b050a950a5f58e98c13b4d28f6af0 100644
--- a/drivers/md/dm-thin.c
+++ b/drivers/md/dm-thin.c
@@ -357,6 +357,7 @@ struct thin_c {
 	 */
 	refcount_t refcount;
 	struct completion can_destroy;
+	u64 offset;
 };
 
 /*----------------------------------------------------------------*/
@@ -1180,9 +1181,9 @@ static void process_prepared_discard_passdown_pt1(struct dm_thin_new_mapping *m)
 	discard_parent = bio_alloc(NULL, 1, 0, GFP_NOIO);
 	discard_parent->bi_end_io = passdown_endio;
 	discard_parent->bi_private = m;
- 	if (m->maybe_shared)
- 		passdown_double_checking_shared_status(m, discard_parent);
- 	else {
+	if (m->maybe_shared)
+		passdown_double_checking_shared_status(m, discard_parent);
+	else {
 		struct discard_op op;
 
 		begin_discard(&op, tc, discard_parent);
@@ -4149,7 +4150,7 @@ static int thin_ctr(struct dm_target *ti, unsigned argc, char **argv)
 
 	mutex_lock(&dm_thin_pool_table.mutex);
 
-	if (argc != 2 && argc != 3) {
+	if (argc < 2 || argc > 4) {
 		ti->error = "Invalid argument count";
 		r = -EINVAL;
 		goto out_unlock;
@@ -4168,7 +4169,8 @@ static int thin_ctr(struct dm_target *ti, unsigned argc, char **argv)
 	bio_list_init(&tc->retry_on_resume_list);
 	tc->sort_bio_list = RB_ROOT;
 
-	if (argc == 3) {
+	/* Use "/" to indicate "no origin device" while providing an offset */
+	if (argc >= 3 && strcmp(argv[2], "/")) {
 		if (!strcmp(argv[0], argv[2])) {
 			ti->error = "Error setting origin device";
 			r = -EINVAL;
@@ -4196,6 +4198,23 @@ static int thin_ctr(struct dm_target *ti, unsigned argc, char **argv)
 		goto bad_common;
 	}
 
+	tc->offset = 0;
+	if (argc > 3) {
+		sector_t sector_offset;
+
+		if (kstrtoull(argv[3], 10, &tc->offset)) {
+			ti->error = "Invalid offset";
+			r = -EINVAL;
+			goto bad_common;
+		}
+
+		if (check_add_overflow(tc->offset, ti->len, &sector_offset)) {
+			ti->error = "Offset + len overflows sector_t";
+			r = -EINVAL;
+			goto bad_common;
+		}
+	}
+
 	pool_md = dm_get_md(tc->pool_dev->bdev->bd_dev);
 	if (!pool_md) {
 		ti->error = "Couldn't get pool mapped device";
@@ -4285,8 +4304,9 @@ static int thin_ctr(struct dm_target *ti, unsigned argc, char **argv)
 
 static int thin_map(struct dm_target *ti, struct bio *bio)
 {
-	bio->bi_iter.bi_sector = dm_target_offset(ti, bio->bi_iter.bi_sector);
+	struct thin_c *tc = ti->private;
 
+	bio->bi_iter.bi_sector = dm_target_offset(ti, bio->bi_iter.bi_sector) + tc->offset;
 	return thin_bio_map(ti, bio);
 }
 
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Re: [dm-devel] [PATCH 1/2] Fail I/O to thin pool devices

[Demi Marie Obenour]

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

On Tue, Feb 07, 2023 at 03:02:51PM +0000, Joe Thornber wrote:
> Nack.
> 
> I don't see the security issue; how is this any different from running the
> thin tools on any incorrect device?  Or even the data device that the pool
> is mirroring.

I special-cased the pool device for two reasons:

1. I have run the thin tools on the pool device myself before realising
   that they needed to be run on the metadata device.  It took me a
   while to realize that I was using the wrong device.  I have not made
   that mistake with other devices, which is why I special-cased the
   pool device in this patch.

2. Doing I/O to the pool device is pointless.  The pool device is
   strictly slower than the data device and exposes the exact same
   contents, so accessing the pool device directly is never what one
   wants.

If there are backwards compatibility concerns, I could make this be
controlled by a Kconfig option, module parameter, or both.

> In general the thin tools don't modify the metadata they're
> running on.  If you know of a security issue with the thin tools please let
> me know.

I am not aware of a concrete security problem, but in general I prefer
not to expose unnecessary attack surface.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [dm-devel] [PATCH 2/2] dm-thin: Allow specifying an offset

[Demi Marie Obenour]

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

On Tue, Feb 07, 2023 at 03:03:57PM +0000, Joe Thornber wrote:
> Nack.  I'm not building a linear target into every other target.  Layering
> targets is simple.

It also introduces a performance penalty, which is measurable on some
workloads.  Even dm-linear is not free.  The crypt target also has this
feature, so there is precedent.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [dm-devel] [PATCH 1/2] Fail I/O to thin pool devices

[Zdenek Kabelac]

Dne 07. 02. 23 v 17:19 Demi Marie Obenour napsal(a):
> On Tue, Feb 07, 2023 at 03:02:51PM +0000, Joe Thornber wrote:
>> Nack.
>>
>> I don't see the security issue; how is this any different from running the
>> thin tools on any incorrect device?  Or even the data device that the pool
>> is mirroring.
> 
> I special-cased the pool device for two reasons:
> 
> 1. I have run the thin tools on the pool device myself before realising
>     that they needed to be run on the metadata device.  It took me a
>     while to realize that I was using the wrong device.  I have not made
>     that mistake with other devices, which is why I special-cased the
>     pool device in this patch.
> 
> 2. Doing I/O to the pool device is pointless.  The pool device is
>     strictly slower than the data device and exposes the exact same
>     contents, so accessing the pool device directly is never what one
>     wants.
> 
> If there are backwards compatibility concerns, I could make this be
> controlled by a Kconfig option, module parameter, or both.
> 
>> In general the thin tools don't modify the metadata they're
>> running on.  If you know of a security issue with the thin tools please let
>> me know.
> 
> I am not aware of a concrete security problem, but in general I prefer
> not to expose unnecessary attack surface.

lvm2 introduced 'protection' layer device - which keeps   -tpool opened and 
thus avoid possibility to use  i.e.  mkfs on thin-pool itself (as it requires 
exclusive open)

Zdenek