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

[David Herrmann]

Hey

On Wed, Sep 20, 2017 at 12:12 AM, Dmitry Torokhov
<dmitry.torokhov@xxxxxxxxx> wrote:
> On Tue, Sep 19, 2017 at 2:54 PM, Meng Xu <meng.xu@xxxxxxxxxx> wrote:
>>
>>
>> On 09/19/2017 05:31 PM, Dmitry Torokhov wrote:
>>>
>>> On Mon, Sep 18, 2017 at 10:21 PM, Meng Xu <mengxu.gatech@xxxxxxxxx> 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.
>>
>> Yes, what you described is the root cause of this double-fetch situation
>> and that is why I am proposing this patch.
>>>
>>>
>>> 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.
>>
>> Yes, that is also true and we should never assume to get consistent
>> data from userspace. That is why in this case, we can have user
>> started with UHID_INPUT just to skip the large chunk of code in
>> if (type == UHID_CREATE) {} and then replace it with UHID_CREATE
>> and take the function uhid_dev_create(uhid, &uhid->input_buf).
>> This is exactly what this patch tries to mitigate.
>
> OK, so the driver should be able to withstand equivalent of "dd
> if=/dev/random of=/dev/uhid". The point of special handling of
> UHID_CREATE in uhid_event_from_user() is to convert the layout of the
> UHID event from 32 to 64 bit layout because we made a mistake and have
> a pointer data there. We do not really care about contents. We do not
> care it if changes. We do not care of the pointer is valid. If there
> was garbage, or there will be garbage after conversion, it does not
> care one bit. uhid_dev_create() has to validate the input and reject
> invalid input.

I agree.

With current code, worst case scenario is someone shortcutting the
compat-conversion by setting UHID_CREATE after uhid_event_from_user()
copied it. However, this does no harm, as Dmitry explained. If
user-space wants to shortcut the conversion, let them do so...

Thanks
David