* [bug report] dm ioctl: harden copy_params()'s copy_from_user() from malicious users
@ 2024-04-28 13:12 Dan Carpenter
2024-04-28 16:20 ` Kees Cook
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2024-04-28 13:12 UTC (permalink / raw)
To: dm-devel; +Cc: linux-hardening, Kees Cook
Hi DM Maintainers and kernel hardenning people,
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.
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?
1982 goto bad;
1983 data_copied:
1984 /* Wipe the user buffer so we do not return it to userspace */
1985 if (secure_data && clear_user(user, param_kernel->data_size))
1986 goto bad;
1987
1988 *param = dmi;
1989 return 0;
1990
1991 bad:
1992 free_params(dmi, param_kernel->data_size, *param_flags);
1993
1994 return -EFAULT;
1995 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] dm ioctl: harden copy_params()'s copy_from_user() from malicious users
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
2024-04-29 5:49 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Kees Cook @ 2024-04-28 16:20 UTC (permalink / raw)
To: Dan Carpenter; +Cc: dm-devel, linux-hardening
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [bug report] dm ioctl: harden copy_params()'s copy_from_user() from malicious users
2024-04-28 16:20 ` Kees Cook
@ 2024-04-29 5:49 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2024-04-29 5:49 UTC (permalink / raw)
To: Kees Cook; +Cc: dm-devel, linux-hardening
On Sun, Apr 28, 2024 at 09:20:35AM -0700, Kees Cook wrote:
> 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 */
> };
>
Ugh... Those comments are out of date.
What happened was that back in the day we added padding to make the
struct the same size on both 32 bit and 64 bit so that we could do
pointer math like "param + 1" and it would work on both arches.
But they couldn't call it "padding" because there was already a struct
member with that name so it was called "data".
And then people started using it for data and made it a variable length
array. When it's a variable length array obviously "param + 1" is not
going to work. So the original purpose of the padding is now gone.
So now to calculate the real, allocated size of the struct we take
the sizeof() of the struct, subtract 7, and add the number of bytes in
->data[]. #LOL
> The Subject in the email is "bug report", though. Is there something
> here that is breaking?
>
I was going to send an automated email but then I changed it.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-04-29 5:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
2024-04-29 5:49 ` Dan Carpenter
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.