From: Kees Cook <keescook@chromium.org>
To: Dan Carpenter <dan.carpenter@linaro.org>
Cc: dm-devel@lists.linux.dev, linux-hardening@vger.kernel.org
Subject: Re: [bug report] dm ioctl: harden copy_params()'s copy_from_user() from malicious users
Date: Sun, 28 Apr 2024 09:20:35 -0700 [thread overview]
Message-ID: <202404280901.96E2E1AD9@keescook> (raw)
In-Reply-To: <f84a96d7-2df4-4f79-8c27-83a1d9574412@moroto.mountain>
On Sun, Apr 28, 2024 at 04:12:07PM +0300, Dan Carpenter wrote:
> Hi DM Maintainers and kernel hardenning people,
Hello! :)
> drivers/md/dm-ioctl.c
> 1931 static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kernel,
> 1932 int ioctl_flags, struct dm_ioctl **param, int *param_flags)
> 1933 {
> 1934 struct dm_ioctl *dmi;
> 1935 int secure_data;
> 1936 const size_t minimum_data_size = offsetof(struct dm_ioctl, data);
> 1937
> 1938 /* check_version() already copied version from userspace, avoid TOCTOU */
> 1939 if (copy_from_user((char *)param_kernel + sizeof(param_kernel->version),
> 1940 (char __user *)user + sizeof(param_kernel->version),
> 1941 minimum_data_size - sizeof(param_kernel->version)))
> 1942 return -EFAULT;
> 1943
> 1944 if (unlikely(param_kernel->data_size < minimum_data_size) ||
> 1945 unlikely(param_kernel->data_size > DM_MAX_TARGETS * DM_MAX_TARGET_PARAMS)) {
>
> So what's happening here is that struct dm_ioctl->data[] is declared as
> a 7 byte array, but it's actually a variable size array which could be
> more or less than 7 bytes.
Repeating from include/uapi/linux/dm-ioctl.h:
struct dm_ioctl {
...
__u32 data_size; /* total size of data passed in
* including this struct */
__u32 data_start; /* offset to start of data
* relative to start of this struct */
...
char data[7]; /* padding or data */
};
>
> 1946 DMERR("Invalid data size in the ioctl structure: %u",
> 1947 param_kernel->data_size);
> 1948 return -EINVAL;
> 1949 }
> 1950
> 1951 secure_data = param_kernel->flags & DM_SECURE_DATA_FLAG;
> 1952
> 1953 *param_flags = secure_data ? DM_WIPE_BUFFER : 0;
> 1954
> 1955 if (ioctl_flags & IOCTL_FLAGS_NO_PARAMS) {
> 1956 dmi = param_kernel;
> 1957 dmi->data_size = minimum_data_size;
> 1958 goto data_copied;
> 1959 }
> 1960
> 1961 /*
> 1962 * Use __GFP_HIGH to avoid low memory issues when a device is
> 1963 * suspended and the ioctl is needed to resume it.
> 1964 * Use kmalloc() rather than vmalloc() when we can.
> 1965 */
> 1966 dmi = NULL;
> 1967 dmi = kvmalloc(param_kernel->data_size, GFP_NOIO | __GFP_HIGH);
>
> We allocate the correct size of the variable element array.
>
> 1968
> 1969 if (!dmi) {
> 1970 if (secure_data && clear_user(user, param_kernel->data_size))
> 1971 return -EFAULT;
> 1972 return -ENOMEM;
> 1973 }
> 1974
> 1975 *param_flags |= DM_PARAMS_MALLOC;
> 1976
> 1977 /* Copy from param_kernel (which was already copied from user) */
> 1978 memcpy(dmi, param_kernel, minimum_data_size);
> 1979
> --> 1980 if (copy_from_user(&dmi->data, (char __user *)user + minimum_data_size,
> 1981 param_kernel->data_size - minimum_data_size))
>
> Doesn't the kernel hardenning stuff have run time checks for if we
> write beyond the end of a 7 byte array? Why not just declare it as a
> zero element array?
The usercopy hardening was implemented before we had reliable array
bounds handling in the compilers, so it actually looks up the allocation
size when performing its checks. So, it'll only yell if
"param_kernel->data_size - minimum_data_size" is larger than
"param_kernel->data_size - offset-into-allocation-for &dmi->data" (which
is minimum_data_size).
Now, it sure would be nice to not be lying to the compiler about the
size of "data", so I would generally recommend this change to the UAPI:
diff --git a/include/uapi/linux/dm-ioctl.h b/include/uapi/linux/dm-ioctl.h
index 1990b5700f69..170465be55af 100644
--- a/include/uapi/linux/dm-ioctl.h
+++ b/include/uapi/linux/dm-ioctl.h
@@ -143,7 +143,10 @@ struct dm_ioctl {
char name[DM_NAME_LEN]; /* device name */
char uuid[DM_UUID_LEN]; /* unique identifier for
* the block device */
- char data[7]; /* padding or data */
+ union {
+ char padding[7];/* minimum structure padding */
+ __DECLARE_FLEX_ARRAY(char, data);
+ };
};
/*
On the other hand, if nothing is actively broken, we could just leave it
as-is? (But if we ever try to memcpy() out of dmi->data, we're going to
run into trouble.)
The Subject in the email is "bug report", though. Is there something
here that is breaking?
Also on a related note, the validation for the "data_start" member seems
a bit fragile. It does get checked everywhere that uses if FWICT, but
it feels like it'd be better in validate_params(). *shrug*
-Kees
--
Kees Cook
next prev parent reply other threads:[~2024-04-28 16:20 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-28 13:12 [bug report] dm ioctl: harden copy_params()'s copy_from_user() from malicious users Dan Carpenter
2024-04-28 16:20 ` Kees Cook [this message]
2024-04-29 5:49 ` Dan Carpenter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=202404280901.96E2E1AD9@keescook \
--to=keescook@chromium.org \
--cc=dan.carpenter@linaro.org \
--cc=dm-devel@lists.linux.dev \
--cc=linux-hardening@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.