Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752259AbdIVM15 (ORCPT ); Fri, 22 Sep 2017 08:27:57 -0400 Received: from mail-it0-f65.google.com ([209.85.214.65]:38317 "EHLO mail-it0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751928AbdIVM1z (ORCPT ); Fri, 22 Sep 2017 08:27:55 -0400 X-Google-Smtp-Source: AOwi7QDYHp6IZWwGthy1EUkcmRxoWgdy/wB7fRmbQxBvSL5fH+9ce8Hh/+HnbmYT6rcBa6PdL7jO1ckpIOBL5mC4Sk0= MIME-Version: 1.0 In-Reply-To: References: <1505798497-22436-1-git-send-email-mengxu.gatech@gmail.com> <4703026b-a8ea-20f3-ac8c-93bee6cba8dd@gatech.edu> From: David Herrmann Date: Fri, 22 Sep 2017 14:27:53 +0200 Message-ID: Subject: Re: [PATCH] hid/uhid: fix a double-fetch bug when copying event from user To: Dmitry Torokhov Cc: Meng Xu , Meng Xu , David Herrmann , Jiri Kosina , Benjamin Tissoires , "linux-input@vger.kernel.org" , lkml , sanidhya@gatech.edu, taesoo@gatech.edu Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2571 Lines: 59 Hey On Wed, Sep 20, 2017 at 12:12 AM, Dmitry Torokhov wrote: > On Tue, Sep 19, 2017 at 2:54 PM, Meng Xu wrote: >> >> >> On 09/19/2017 05:31 PM, Dmitry Torokhov wrote: >>> >>> On Mon, Sep 18, 2017 at 10:21 PM, Meng Xu 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