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
