Commit 4fe1ec99 authored by Mikulas Patocka's avatar Mikulas Patocka Committed by Mike Snitzer

dm ioctl: fix misbehavior if list_versions races with module loading

__list_versions will first estimate the required space using the
"dm_target_iterate(list_version_get_needed, &needed)" call and then will
fill the space using the "dm_target_iterate(list_version_get_info,
&iter_info)" call. Each of these calls locks the targets using the
"down_read(&_lock)" and "up_read(&_lock)" calls, however between the first
and second "dm_target_iterate" there is no lock held and the target
modules can be loaded at this point, so the second "dm_target_iterate"
call may need more space than what was the first "dm_target_iterate"
returned.

The code tries to handle this overflow (see the beginning of
list_version_get_info), however this handling is incorrect.

The code sets "param->data_size = param->data_start + needed" and
"iter_info.end = (char *)vers+len" - "needed" is the size returned by the
first dm_target_iterate call; "len" is the size of the buffer allocated by
userspace.

"len" may be greater than "needed"; in this case, the code will write up
to "len" bytes into the buffer, however param->data_size is set to
"needed", so it may write data past the param->data_size value. The ioctl
interface copies only up to param->data_size into userspace, thus part of
the result will be truncated.

Fix this bug by setting "iter_info.end = (char *)vers + needed;" - this
guarantees that the second "dm_target_iterate" call will write only up to
the "needed" buffer and it will exit with "DM_BUFFER_FULL_FLAG" if it
overflows the "needed" space - in this case, userspace will allocate a
larger buffer and retry.

Note that there is also a bug in list_version_get_needed - we need to add
"strlen(tt->name) + 1" to the needed size, not "strlen(tt->name)".

Cc: stable@vger.kernel.org
Signed-off-by: default avatarMikulas Patocka <mpatocka@redhat.com>
Signed-off-by: default avatarMike Snitzer <snitzer@kernel.org>
parent 094226ad
...@@ -655,7 +655,7 @@ static void list_version_get_needed(struct target_type *tt, void *needed_param) ...@@ -655,7 +655,7 @@ static void list_version_get_needed(struct target_type *tt, void *needed_param)
size_t *needed = needed_param; size_t *needed = needed_param;
*needed += sizeof(struct dm_target_versions); *needed += sizeof(struct dm_target_versions);
*needed += strlen(tt->name); *needed += strlen(tt->name) + 1;
*needed += ALIGN_MASK; *needed += ALIGN_MASK;
} }
...@@ -720,7 +720,7 @@ static int __list_versions(struct dm_ioctl *param, size_t param_size, const char ...@@ -720,7 +720,7 @@ static int __list_versions(struct dm_ioctl *param, size_t param_size, const char
iter_info.old_vers = NULL; iter_info.old_vers = NULL;
iter_info.vers = vers; iter_info.vers = vers;
iter_info.flags = 0; iter_info.flags = 0;
iter_info.end = (char *)vers+len; iter_info.end = (char *)vers + needed;
/* /*
* Now loop through filling out the names & versions. * Now loop through filling out the names & versions.
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment