linux-hardening - Re: [bug report] dm ioctl: harden copy_params()'s copy_from

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <202404280901.96E2E1AD9@keescook>
Date: Sun, 28 Apr 2024 09:20:35 -0700
From: Kees Cook <keescook@...omium.org>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: dm-devel@...ts.linux.dev, linux-hardening@...r.kernel.org
Subject: Re: [bug report] dm ioctl: harden copy_params()'s copy_from_user()
 from malicious users

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