2023-02-07 01:19:12

by Demi Marie Obenour

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

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 <[email protected]>
---
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

2023-02-07 01:19:14

by Demi Marie Obenour

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

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 <[email protected]>
---
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