From b60528d9e68113e2c297c3a45102332cb1d3e608 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 3 Jun 2023 10:52:39 -0400 Subject: dm ioctl: Check dm_target_spec is sufficiently aligned Otherwise subsequent code, if given malformed input, could dereference a misaligned 'struct dm_target_spec *'. Signed-off-by: Demi Marie Obenour Signed-off-by: Stephen Rothwell # use %zu Reviewed-by: Mikulas Patocka Signed-off-by: Mike Snitzer --- drivers/md/dm-ioctl.c | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'drivers/md/dm-ioctl.c') diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c index 8ba4cbb92351..3a6989b7817d 100644 --- a/drivers/md/dm-ioctl.c +++ b/drivers/md/dm-ioctl.c @@ -1394,6 +1394,15 @@ static inline blk_mode_t get_mode(struct dm_ioctl *param) static int next_target(struct dm_target_spec *last, uint32_t next, void *end, struct dm_target_spec **spec, char **target_params) { + static_assert(__alignof__(struct dm_target_spec) <= 8, + "struct dm_target_spec must not require more than 8-byte alignment"); + + if (next % __alignof__(struct dm_target_spec)) { + DMERR("Next dm_target_spec (offset %u) is not %zu-byte aligned", + next, __alignof__(struct dm_target_spec)); + return -EINVAL; + } + *spec = (struct dm_target_spec *) ((unsigned char *) last + next); *target_params = (char *) (*spec + 1); -- cgit v1.2.3 From 13f4a697f8b4feb705569f9336127e9e2f9ac596 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 3 Jun 2023 10:52:40 -0400 Subject: dm ioctl: Avoid pointer arithmetic overflow Especially on 32-bit systems, it is possible for the pointer arithmetic to overflow and cause a userspace pointer to be dereferenced in the kernel. Signed-off-by: Demi Marie Obenour Reviewed-by: Mikulas Patocka Signed-off-by: Mike Snitzer --- drivers/md/dm-ioctl.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) (limited to 'drivers/md/dm-ioctl.c') diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c index 3a6989b7817d..e322fd490634 100644 --- a/drivers/md/dm-ioctl.c +++ b/drivers/md/dm-ioctl.c @@ -1397,6 +1397,22 @@ static int next_target(struct dm_target_spec *last, uint32_t next, void *end, static_assert(__alignof__(struct dm_target_spec) <= 8, "struct dm_target_spec must not require more than 8-byte alignment"); + /* + * Number of bytes remaining, starting with last. This is always + * sizeof(struct dm_target_spec) or more, as otherwise *last was + * out of bounds already. + */ + size_t remaining = (char *)end - (char *)last; + + /* + * There must be room for both the next target spec and the + * NUL-terminator of the target itself. + */ + if (remaining - sizeof(struct dm_target_spec) <= next) { + DMERR("Target spec extends beyond end of parameters"); + return -EINVAL; + } + if (next % __alignof__(struct dm_target_spec)) { DMERR("Next dm_target_spec (offset %u) is not %zu-byte aligned", next, __alignof__(struct dm_target_spec)); -- cgit v1.2.3 From 10655c7a48570315343fdd9cc6acb261d57c2c7a Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 3 Jun 2023 10:52:41 -0400 Subject: dm ioctl: structs and parameter strings must not overlap The NUL terminator for each target parameter string must precede the following 'struct dm_target_spec'. Otherwise, dm_split_args() might corrupt this struct. Furthermore, the first 'struct dm_target_spec' must come after the 'struct dm_ioctl', as if it overlaps too much dm_split_args() could corrupt the 'struct dm_ioctl'. Signed-off-by: Demi Marie Obenour Reviewed-by: Mikulas Patocka Signed-off-by: Mike Snitzer --- drivers/md/dm-ioctl.c | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) (limited to 'drivers/md/dm-ioctl.c') diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c index e322fd490634..a92abbe90981 100644 --- a/drivers/md/dm-ioctl.c +++ b/drivers/md/dm-ioctl.c @@ -1391,7 +1391,7 @@ static inline blk_mode_t get_mode(struct dm_ioctl *param) return mode; } -static int next_target(struct dm_target_spec *last, uint32_t next, void *end, +static int next_target(struct dm_target_spec *last, uint32_t next, const char *end, struct dm_target_spec **spec, char **target_params) { static_assert(__alignof__(struct dm_target_spec) <= 8, @@ -1402,7 +1402,7 @@ static int next_target(struct dm_target_spec *last, uint32_t next, void *end, * sizeof(struct dm_target_spec) or more, as otherwise *last was * out of bounds already. */ - size_t remaining = (char *)end - (char *)last; + size_t remaining = end - (char *)last; /* * There must be room for both the next target spec and the @@ -1422,10 +1422,7 @@ static int next_target(struct dm_target_spec *last, uint32_t next, void *end, *spec = (struct dm_target_spec *) ((unsigned char *) last + next); *target_params = (char *) (*spec + 1); - if (*spec < (last + 1)) - return -EINVAL; - - return invalid_str(*target_params, end); + return 0; } static int populate_table(struct dm_table *table, @@ -1435,8 +1432,9 @@ static int populate_table(struct dm_table *table, unsigned int i = 0; struct dm_target_spec *spec = (struct dm_target_spec *) param; uint32_t next = param->data_start; - void *end = (void *) param + param_size; + const char *const end = (const char *) param + param_size; char *target_params; + size_t min_size = sizeof(struct dm_ioctl); if (!param->target_count) { DMERR("%s: no targets specified", __func__); @@ -1444,6 +1442,13 @@ static int populate_table(struct dm_table *table, } for (i = 0; i < param->target_count; i++) { + const char *nul_terminator; + + if (next < min_size) { + DMERR("%s: next target spec (offset %u) overlaps %s", + __func__, next, i ? "previous target" : "'struct dm_ioctl'"); + return -EINVAL; + } r = next_target(spec, next, end, &spec, &target_params); if (r) { @@ -1451,6 +1456,15 @@ static int populate_table(struct dm_table *table, return r; } + nul_terminator = memchr(target_params, 0, (size_t)(end - target_params)); + if (nul_terminator == NULL) { + DMERR("%s: target parameters not NUL-terminated", __func__); + return -EINVAL; + } + + /* Add 1 for NUL terminator */ + min_size = (size_t)(nul_terminator - (const char *)spec) + 1; + r = dm_table_add_target(table, spec->target_type, (sector_t) spec->sector_start, (sector_t) spec->length, -- cgit v1.2.3 From 249bed821b4db6d95a99160f7d6d236ea5fe6362 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 3 Jun 2023 10:52:42 -0400 Subject: dm ioctl: Avoid double-fetch of version The version is fetched once in check_version(), which then does some validation and then overwrites the version in userspace with the API version supported by the kernel. copy_params() then fetches the version from userspace *again*, and this time no validation is done. The result is that the kernel's version number is completely controllable by userspace, provided that userspace can win a race condition. Fix this flaw by not copying the version back to the kernel the second time. This is not exploitable as the version is not further used in the kernel. However, it could become a problem if future patches start relying on the version field. Cc: stable@vger.kernel.org Signed-off-by: Demi Marie Obenour Signed-off-by: Mike Snitzer --- drivers/md/dm-ioctl.c | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-) (limited to 'drivers/md/dm-ioctl.c') diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c index a92abbe90981..bfaebc02833a 100644 --- a/drivers/md/dm-ioctl.c +++ b/drivers/md/dm-ioctl.c @@ -1872,30 +1872,36 @@ static ioctl_fn lookup_ioctl(unsigned int cmd, int *ioctl_flags) * As well as checking the version compatibility this always * copies the kernel interface version out. */ -static int check_version(unsigned int cmd, struct dm_ioctl __user *user) +static int check_version(unsigned int cmd, struct dm_ioctl __user *user, + struct dm_ioctl *kernel_params) { - uint32_t version[3]; int r = 0; - if (copy_from_user(version, user->version, sizeof(version))) + /* Make certain version is first member of dm_ioctl struct */ + BUILD_BUG_ON(offsetof(struct dm_ioctl, version) != 0); + + if (copy_from_user(kernel_params->version, user->version, sizeof(kernel_params->version))) return -EFAULT; - if ((version[0] != DM_VERSION_MAJOR) || - (version[1] > DM_VERSION_MINOR)) { + if ((kernel_params->version[0] != DM_VERSION_MAJOR) || + (kernel_params->version[1] > DM_VERSION_MINOR)) { DMERR("ioctl interface mismatch: kernel(%u.%u.%u), user(%u.%u.%u), cmd(%d)", DM_VERSION_MAJOR, DM_VERSION_MINOR, DM_VERSION_PATCHLEVEL, - version[0], version[1], version[2], cmd); + kernel_params->version[0], + kernel_params->version[1], + kernel_params->version[2], + cmd); r = -EINVAL; } /* * Fill in the kernel version. */ - version[0] = DM_VERSION_MAJOR; - version[1] = DM_VERSION_MINOR; - version[2] = DM_VERSION_PATCHLEVEL; - if (copy_to_user(user->version, version, sizeof(version))) + kernel_params->version[0] = DM_VERSION_MAJOR; + kernel_params->version[1] = DM_VERSION_MINOR; + kernel_params->version[2] = DM_VERSION_PATCHLEVEL; + if (copy_to_user(user->version, kernel_params->version, sizeof(kernel_params->version))) return -EFAULT; return r; @@ -1921,7 +1927,10 @@ static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kern const size_t minimum_data_size = offsetof(struct dm_ioctl, data); unsigned int noio_flag; - if (copy_from_user(param_kernel, user, minimum_data_size)) + /* check_version() already copied version from userspace, avoid TOCTOU */ + if (copy_from_user((char *)param_kernel + sizeof(param_kernel->version), + (char __user *)user + sizeof(param_kernel->version), + minimum_data_size - sizeof(param_kernel->version))) return -EFAULT; if (param_kernel->data_size < minimum_data_size) { @@ -2033,7 +2042,7 @@ static int ctl_ioctl(struct file *file, uint command, struct dm_ioctl __user *us * Check the interface version passed in. This also * writes out the kernel's interface version. */ - r = check_version(cmd, user); + r = check_version(cmd, user, ¶m_kernel); if (r) return r; -- cgit v1.2.3 From a85f1a9de91a59cd9b12d60f631cbda9c56a1c3c Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 3 Jun 2023 10:52:43 -0400 Subject: dm ioctl: Refuse to create device named "control" Typical userspace setups create a symlink under /dev/mapper with the name of the device, but /dev/mapper/control is reserved for DM's control device. Therefore, trying to create such a device is almost certain to be a userspace bug. Signed-off-by: Demi Marie Obenour Signed-off-by: Mike Snitzer --- drivers/md/dm-ioctl.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) (limited to 'drivers/md/dm-ioctl.c') diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c index bfaebc02833a..e172a91e88dc 100644 --- a/drivers/md/dm-ioctl.c +++ b/drivers/md/dm-ioctl.c @@ -767,7 +767,12 @@ static int get_target_version(struct file *filp, struct dm_ioctl *param, size_t static int check_name(const char *name) { if (strchr(name, '/')) { - DMERR("invalid device name"); + DMERR("device name cannot contain '/'"); + return -EINVAL; + } + + if (strcmp(name, DM_CONTROL_NODE) == 0) { + DMERR("device name cannot be \"%s\"", DM_CONTROL_NODE); return -EINVAL; } -- cgit v1.2.3 From 81ca2dbefaabe1a2ca1c7cfc84dfd45c072c82a6 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 3 Jun 2023 10:52:44 -0400 Subject: dm ioctl: Refuse to create device named "." or ".." Using either of these is going to greatly confuse userspace, as they are not valid symlink names and so creating the usual /dev/mapper/NAME symlink will not be possible. As creating a device with either of these names is almost certainly a userspace bug, just error out. Signed-off-by: Demi Marie Obenour Signed-off-by: Mike Snitzer --- drivers/md/dm-ioctl.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'drivers/md/dm-ioctl.c') diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c index e172a91e88dc..16244a7b193c 100644 --- a/drivers/md/dm-ioctl.c +++ b/drivers/md/dm-ioctl.c @@ -771,8 +771,10 @@ static int check_name(const char *name) return -EINVAL; } - if (strcmp(name, DM_CONTROL_NODE) == 0) { - DMERR("device name cannot be \"%s\"", DM_CONTROL_NODE); + if (strcmp(name, DM_CONTROL_NODE) == 0 || + strcmp(name, ".") == 0 || + strcmp(name, "..") == 0) { + DMERR("device name cannot be \"%s\", \".\", or \"..\"", DM_CONTROL_NODE); return -EINVAL; } -- cgit v1.2.3 From e2c789cab60a493a72b42cb53eb5fbf96d5f1ae3 Mon Sep 17 00:00:00 2001 From: Mikulas Patocka Date: Mon, 26 Jun 2023 16:48:40 +0200 Subject: dm: get rid of GFP_NOIO workarounds for __vmalloc and kvmalloc In the past, the function __vmalloc didn't respect the GFP flags - it allocated memory with the provided gfp flags, but it allocated page tables with GFP_KERNEL. This was fixed in commit 451769ebb7e7 ("mm/vmalloc: alloc GFP_NO{FS,IO} for vmalloc") so the memalloc_noio_{save,restore} workaround is no longer needed. The function kvmalloc didn't like flags different from GFP_KERNEL. This was fixed in commit a421ef303008 ("mm: allow !GFP_KERNEL allocations for kvmalloc"), so kvmalloc can now be called with GFP_NOIO. Signed-off-by: Mikulas Patocka Signed-off-by: Mike Snitzer --- drivers/md/dm-bufio.c | 17 ----------------- drivers/md/dm-ioctl.c | 5 +---- 2 files changed, 1 insertion(+), 21 deletions(-) (limited to 'drivers/md/dm-ioctl.c') diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c index a7079b38756a..bc309e41d074 100644 --- a/drivers/md/dm-bufio.c +++ b/drivers/md/dm-bufio.c @@ -1157,23 +1157,6 @@ static void *alloc_buffer_data(struct dm_bufio_client *c, gfp_t gfp_mask, *data_mode = DATA_MODE_VMALLOC; - /* - * __vmalloc allocates the data pages and auxiliary structures with - * gfp_flags that were specified, but pagetables are always allocated - * with GFP_KERNEL, no matter what was specified as gfp_mask. - * - * Consequently, we must set per-process flag PF_MEMALLOC_NOIO so that - * all allocations done by this process (including pagetables) are done - * as if GFP_NOIO was specified. - */ - if (gfp_mask & __GFP_NORETRY) { - unsigned int noio_flag = memalloc_noio_save(); - void *ptr = __vmalloc(c->block_size, gfp_mask); - - memalloc_noio_restore(noio_flag); - return ptr; - } - return __vmalloc(c->block_size, gfp_mask); } diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c index 16244a7b193c..8e14a4a0996d 100644 --- a/drivers/md/dm-ioctl.c +++ b/drivers/md/dm-ioctl.c @@ -1932,7 +1932,6 @@ static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kern struct dm_ioctl *dmi; int secure_data; const size_t minimum_data_size = offsetof(struct dm_ioctl, data); - unsigned int noio_flag; /* check_version() already copied version from userspace, avoid TOCTOU */ if (copy_from_user((char *)param_kernel + sizeof(param_kernel->version), @@ -1962,9 +1961,7 @@ static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kern * Use kmalloc() rather than vmalloc() when we can. */ dmi = NULL; - noio_flag = memalloc_noio_save(); - dmi = kvmalloc(param_kernel->data_size, GFP_KERNEL | __GFP_HIGH); - memalloc_noio_restore(noio_flag); + dmi = kvmalloc(param_kernel->data_size, GFP_NOIO | __GFP_HIGH); if (!dmi) { if (secure_data && clear_user(user, param_kernel->data_size)) -- cgit v1.2.3