[dm-devel] [PATCH v2] dm: make it possible to allocate error strings

[Mikulas Patocka]

Hi

Here I'm sending the second version of the patch for allocated error 
strings.

In the previous version, there was a bug that if we went down the "can't 
change device type" path, we didn't set ti_error and it resulted in random 
address being freed and a crash.

Mikulas



From: Mikulas Patocka <mpatocka at redhat.com>

Previously, ti->error and ti_error strings were pointing to statically
allocated memory (the .rodata section) and it was not possible to add
parameters to the error strings.

This patch makes possible to allocate error strings dynamically using the
"kasprintf" function, so we can add arbitrary parameters to the strings.

We need to free the error string only if is allocated with kasprintf. So,
we introduce a function "dm_free_error" that tests if the string points to
the module area (and doesn't free the string if it does), then it calls
kfree_const. kfree_const detects if the string points to the kernel
.rodata section and frees the string if it is not in .rodata.

Signed-off-by: Mikulas Patocka <mpatocka at redhat.com>

---
 drivers/md/dm-ioctl.c         |   10 +++++++++-
 drivers/md/dm-table.c         |   16 ++++++++++------
 include/linux/device-mapper.h |   15 ++++++++++++++-
 3 files changed, 33 insertions(+), 8 deletions(-)

Index: linux-2.6/drivers/md/dm-ioctl.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-ioctl.c	2022-10-03 15:22:15.000000000 +0200
+++ linux-2.6/drivers/md/dm-ioctl.c	2022-10-03 15:44:04.000000000 +0200
@@ -1465,7 +1465,8 @@ static int table_load(struct file *filp,
 		DMERR("can't replace immutable target type %s",
 		      immutable_target_type->name);
 		r = -EINVAL;
-		ti_error = "can't replace immutable target";
+		ti_error = kasprintf(GFP_NOIO, "can't replace immutable target type %s",
+			immutable_target_type->name);
 		goto err_unlock_md_type;
 	}
 
@@ -1481,6 +1482,8 @@ static int table_load(struct file *filp,
 		DMERR("can't change device type (old=%u vs new=%u) after initial table load.",
 		      dm_get_md_type(md), dm_table_get_type(t));
 		r = -EINVAL;
+		ti_error = kasprintf(GFP_NOIO, "can't change device type (old=%u vs new=%u) after initial table load.",
+			dm_get_md_type(md), dm_table_get_type(t));
 		goto err_unlock_md_type;
 	}
 
@@ -1521,11 +1524,16 @@ err_destroy_table:
 err:
 	dm_put(md);
 err0:
+	if (!ti_error)
+		ti_error = "can't allocate error string";
 	if (param->flags & DM_RETURN_ERROR_FLAG) {
 		param->flags &= ~DM_RETURN_ERROR_FLAG;
 		strlcpy(param->name, ti_error, sizeof param->name);
 		param->error = r;
+		dm_free_error(ti_error);
 		return 0;
+	} else {
+		dm_free_error(ti_error);
 	}
 	return r;
 }
Index: linux-2.6/drivers/md/dm-table.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-table.c	2022-10-03 15:22:15.000000000 +0200
+++ linux-2.6/drivers/md/dm-table.c	2022-10-03 15:22:15.000000000 +0200
@@ -656,32 +656,32 @@ int dm_table_add_target(struct dm_table
 
 	ti->type = dm_get_target_type(type);
 	if (!ti->type) {
-		ti->error = "unknown target type";
+		ti->error = kasprintf(GFP_NOIO, "unknown target type %s", type);
 		r = -EINVAL;
 		goto bad0;
 	}
 
 	if (dm_target_needs_singleton(ti->type)) {
 		if (t->num_targets) {
-			ti->error = "singleton target type must appear alone in table";
+			ti->error = kasprintf(GFP_NOIO, "singleton target type %s must appear alone in table", type);
 			goto bad1;
 		}
 		t->singleton = true;
 	}
 
 	if (dm_target_always_writeable(ti->type) && !(t->mode & FMODE_WRITE)) {
-		ti->error = "target type may not be included in a read-only table";
+		ti->error = kasprintf(GFP_NOIO, "target type %s may not be included in a read-only table", type);
 		goto bad1;
 	}
 
 	if (t->immutable_target_type) {
 		if (t->immutable_target_type != ti->type) {
-			ti->error = "immutable target type cannot be mixed with other target types";
+			ti->error = kasprintf(GFP_NOIO, "immutable target type %s cannot be mixed with other target types", type);
 			goto bad1;
 		}
 	} else if (dm_target_is_immutable(ti->type)) {
 		if (t->num_targets) {
-			ti->error = "immutable target type cannot be mixed with other target types";
+			ti->error = kasprintf(GFP_NOIO, "immutable target type %s cannot be mixed with other target types", type);
 			goto bad1;
 		}
 		t->immutable_target_type = ti->type;
@@ -705,7 +705,7 @@ int dm_table_add_target(struct dm_table
 
 	r = dm_split_args(&argc, &argv, params);
 	if (r) {
-		ti->error = "couldn't split parameters";
+		ti->error = kasprintf(GFP_NOIO, "couldn't split parameters for target %s", type);
 		goto bad1;
 	}
 
@@ -728,9 +728,13 @@ int dm_table_add_target(struct dm_table
 bad1:
 	dm_put_target_type(ti->type);
 bad0:
+	if (!ti->error)
+		ti->error = "can't allocate error string";
 	DMERR("%s: %s: %s (%pe)", dm_device_name(t->md), type, ti->error, ERR_PTR(r));
 	if (ti_error)
 		*ti_error = ti->error;
+	else
+		dm_free_error(ti->error);
 	return r;
 }
 
Index: linux-2.6/include/linux/device-mapper.h
===================================================================
--- linux-2.6.orig/include/linux/device-mapper.h	2022-10-03 15:22:15.000000000 +0200
+++ linux-2.6/include/linux/device-mapper.h	2022-10-03 15:45:15.000000000 +0200
@@ -342,7 +342,12 @@ struct dm_target {
 	/* target specific data */
 	void *private;
 
-	/* Used to provide an error string from the ctr */
+	/*
+	 * Used to provide an error string from the ctr. It may pointe to a
+	 * statically allocated string in the kernel's or module's .rodata
+	 * section, or to a dynamically allocated string using kmalloc (for
+	 * example, using kasprintf). It must not point to vmallocated memory.
+	 */
 	char *error;
 
 	/*
@@ -679,4 +684,12 @@ static inline unsigned long to_bytes(sec
 	return (n << SECTOR_SHIFT);
 }
 
+#include <asm/sections.h>
+
+static inline void dm_free_error(char *ti_error)
+{
+	if (!is_vmalloc_or_module_addr(ti_error))
+		kfree_const(ti_error);
+}
+
 #endif	/* _LINUX_DEVICE_MAPPER_H */