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

Hey

On Wed, Sep 20, 2017 at 12:12 AM, Dmitry Torokhov
[off-list ref] wrote:

On Tue, Sep 19, 2017 at 2:54 PM, Meng Xu [off-list ref] wrote:

quoted

On 09/19/2017 05:31 PM, Dmitry Torokhov wrote:

quoted

On Mon, Sep 18, 2017 at 10:21 PM, Meng Xu [off-list ref] wrote:

quoted

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.

quoted

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