From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2513BC433EF for ; Mon, 24 Jan 2022 19:13:23 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1345857AbiAXTNV (ORCPT ); Mon, 24 Jan 2022 14:13:21 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48004 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1343924AbiAXTLO (ORCPT ); Mon, 24 Jan 2022 14:11:14 -0500 Received: from ams.source.kernel.org (ams.source.kernel.org [IPv6:2604:1380:4601:e00::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 93D85C061366; Mon, 24 Jan 2022 11:03:01 -0800 (PST) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 536C1B8121F; Mon, 24 Jan 2022 19:03:00 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 82543C340EC; Mon, 24 Jan 2022 19:02:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1643050979; bh=Px9XtuqngjMe5hcYvLzLAWify+wY9K8GVy1dG9DMJ6w=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=0kDEpa7hTX/KxiXdH6V33d3oKSJKdl8LX82UZ/8QNbzew3yWQNlSSbyPu0wgOoOxe WHQH4eo2ZuFeyJiywD0Mi7IjUPWHgt+A5CmCpxVltzg/VnOQYzKgaq5bDxwIdjiDEN l1XYYVlZvT7xSQX1U7/yxKeG7uU2Ao1zmqAsMGMI= From: Greg Kroah-Hartman To: linux-kernel@vger.kernel.org Cc: Greg Kroah-Hartman , stable@vger.kernel.org, Jann Horn , Jiri Kosina Subject: [PATCH 4.14 016/186] HID: uhid: Fix worker destroying device without any protection Date: Mon, 24 Jan 2022 19:41:31 +0100 Message-Id: <20220124183937.631344024@linuxfoundation.org> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20220124183937.101330125@linuxfoundation.org> References: <20220124183937.101330125@linuxfoundation.org> User-Agent: quilt/0.66 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: stable@vger.kernel.org From: Jann Horn commit 4ea5763fb79ed89b3bdad455ebf3f33416a81624 upstream. uhid has to run hid_add_device() from workqueue context while allowing parallel use of the userspace API (which is protected with ->devlock). But hid_add_device() can fail. Currently, that is handled by immediately destroying the associated HID device, without using ->devlock - but if there are concurrent requests from userspace, that's wrong and leads to NULL dereferences and/or memory corruption (via use-after-free). Fix it by leaving the HID device as-is in the worker. We can clean it up later, either in the UHID_DESTROY command handler or in the ->release() handler. Cc: stable@vger.kernel.org Fixes: 67f8ecc550b5 ("HID: uhid: fix timeout when probe races with IO") Signed-off-by: Jann Horn Signed-off-by: Jiri Kosina Signed-off-by: Greg Kroah-Hartman --- drivers/hid/uhid.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) --- a/drivers/hid/uhid.c +++ b/drivers/hid/uhid.c @@ -32,11 +32,22 @@ struct uhid_device { struct mutex devlock; + + /* This flag tracks whether the HID device is usable for commands from + * userspace. The flag is already set before hid_add_device(), which + * runs in workqueue context, to allow hid_add_device() to communicate + * with userspace. + * However, if hid_add_device() fails, the flag is cleared without + * holding devlock. + * We guarantee that if @running changes from true to false while you're + * holding @devlock, it's still fine to access @hid. + */ bool running; __u8 *rd_data; uint rd_size; + /* When this is NULL, userspace may use UHID_CREATE/UHID_CREATE2. */ struct hid_device *hid; struct uhid_event input_buf; @@ -67,9 +78,18 @@ static void uhid_device_add_worker(struc if (ret) { hid_err(uhid->hid, "Cannot register HID device: error %d\n", ret); - hid_destroy_device(uhid->hid); - uhid->hid = NULL; + /* We used to call hid_destroy_device() here, but that's really + * messy to get right because we have to coordinate with + * concurrent writes from userspace that might be in the middle + * of using uhid->hid. + * Just leave uhid->hid as-is for now, and clean it up when + * userspace tries to close or reinitialize the uhid instance. + * + * However, we do have to clear the ->running flag and do a + * wakeup to make sure userspace knows that the device is gone. + */ uhid->running = false; + wake_up_interruptible(&uhid->report_wait); } } @@ -478,7 +498,7 @@ static int uhid_dev_create2(struct uhid_ void *rd_data; int ret; - if (uhid->running) + if (uhid->hid) return -EALREADY; rd_size = ev->u.create2.rd_size; @@ -559,7 +579,7 @@ static int uhid_dev_create(struct uhid_d static int uhid_dev_destroy(struct uhid_device *uhid) { - if (!uhid->running) + if (!uhid->hid) return -EINVAL; uhid->running = false; @@ -568,6 +588,7 @@ static int uhid_dev_destroy(struct uhid_ cancel_work_sync(&uhid->worker); hid_destroy_device(uhid->hid); + uhid->hid = NULL; kfree(uhid->rd_data); return 0;