From: Dan Carpenter <dan.carpenter@linaro.org>
To: Kees Cook <keescook@chromium.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: Mon, 29 Apr 2024 08:49:55 +0300 [thread overview]
Message-ID: <5cde5d51-a8f2-4c30-a016-1fd8e34212e0@moroto.mountain> (raw)
In-Reply-To: <202404280901.96E2E1AD9@keescook>
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
prev parent reply other threads:[~2024-04-29 5:50 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
2024-04-29 5:49 ` Dan Carpenter [this message]
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=5cde5d51-a8f2-4c30-a016-1fd8e34212e0@moroto.mountain \
--to=dan.carpenter@linaro.org \
--cc=dm-devel@lists.linux.dev \
--cc=keescook@chromium.org \
--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.