linux-kernel - Re: [PATCH] hid/uhid: fix a double-fetch bug when copying event from user

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]

Date:   Tue, 19 Sep 2017 14:31:41 -0700
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     Meng Xu <mengxu.gatech@...il.com>
Cc:     dh.herrmann@...glemail.com, Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        "linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
        lkml <linux-kernel@...r.kernel.org>, meng.xu@...ech.edu,
        sanidhya@...ech.edu, taesoo@...ech.edu
Subject: Re: [PATCH] hid/uhid: fix a double-fetch bug when copying event from user

On Mon, Sep 18, 2017 at 10:21 PM, Meng Xu <mengxu.gatech@...il.com> wrote:
> When in_compat_syscall(), a user could make type != UHID_CREATE when
> get_user(type, buffer) [first fetch] and later make event->type ==
> UHID_CREATE in copy_from_user(event, buffer, ...) [second fetch].
>
> By doing so, an attacker might circumvent the specific logic to handle
> the type == UHID_CREATE case and later cause undefined behaviors.
>
> This patch enforces that event->type is overriden to the type value
> copied in the first fetch and thus, mitigate this race condition attack.

I do not believe this mitigates anything, as copy_form_user() is not
an atomic operation and we can have 2nd thread "scrambling" the memory
while 1st does the ioctl.

We also should not expect that we always get consistent data from
userspace and the rest of the driver should be able to cope with
(reject) such data.

>
> Signed-off-by: Meng Xu <mengxu.gatech@...il.com>
> ---
>  drivers/hid/uhid.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
> index 7f8ff39..4bbfd8a 100644
> --- a/drivers/hid/uhid.c
> +++ b/drivers/hid/uhid.c
> @@ -448,11 +448,20 @@ static int uhid_event_from_user(const char __user *buffer, size_t len,
>                         kfree(compat);
>                         return 0;
>                 }
> +
>                 /* All others can be copied directly */
> -       }
> +               if (copy_from_user(event, buffer, min(len, sizeof(*event))))
> +                       return -EFAULT;
>
> -       if (copy_from_user(event, buffer, min(len, sizeof(*event))))
> -               return -EFAULT;
> +               /*
> +                * Override type in case the user process rushes to change it
> +                * between two fetches
> +                * */
> +               event->type = type;

So if we started with UHID_INPUT, userspace replaced it with
UHID_FEATURE, we clobber the type UHID_INPUT leaving the rest of the
structure holding the new data and call it a day? That does not make
any sense to me.

> +       } else {
> +               if (copy_from_user(event, buffer, min(len, sizeof(*event))))
> +                       return -EFAULT;
> +       }
>
>         return 0;
>  }
> --
> 2.7.4
>

Thanks.

-- 
Dmitry