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] [day] [month] [year] [list]

Date: Mon, 29 Apr 2024 08:49:55 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Kees Cook <keescook@...omium.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 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