Received: by 2002:a05:6a10:9e8c:0:0:0:0 with SMTP id y12csp387316pxx; Wed, 28 Oct 2020 07:14:51 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwJ1vNaOwxqe6GhxhghhOCMoJ7uin0EQI7tGMYN/K92HkFxR8bakrI2TtXye7biu27HcB5a X-Received: by 2002:a17:906:6a94:: with SMTP id p20mr7409533ejr.499.1603894490824; Wed, 28 Oct 2020 07:14:50 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1603894490; cv=none; d=google.com; s=arc-20160816; b=TpV6uJiJjemFztGc760QFdW9cV03pVgRUshn2La04WUlfiemOevWdRdesk6FQyQb/N KFJzfIc6HZ3/gMxARrQfjiRyfVa0Rz3yYBCpRo1ITAvVYSQwJ05KCr9NqoSlIYv1DuAs MSeoh/zHcYTtojAvGKHj5JbFm965srFiG6XKMU+JCF3rkQlt4iorSlNfVVsVV1xNm9NS qP9FhO2PQoGoHZGROL85ZaKORp31S6lMiaA7gQdoZFVGc4vsUd9jzA/VICEf8Kbhn0uh dpLVxCbKioJEcM5+vcDl9axrNm82efNDA9nCOjTwB140IIr4M2PWgcTnSzXbl6UfM6DU e+Pw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :user-agent:references:in-reply-to:message-id:date:subject:cc:to :from:dkim-signature; bh=4xF9AU10CzdIlDQ0cI/I8MjRXPOR4k2K//8YW21f6/M=; b=dUwPf9Gf3eHzkBp9ZxDDhvbnbQVWJJLRWg681deTAdm/3GLieC58bL8NGHCQ6xAGIP zpP3WFaFY79faCU7lRKxhh20pM91/DS9PVJEh3uumhzxZZARyhd6XGy/6sNbnE2dC8fR juRI9Se7kwaTsTQf3teJTVKCFCTnt35SzHssB6UawGVblw/D6cjZ16Yvvc7or7SWmykq zupTJSuSpSt+9a3Cu+vjlghW0Io5G71/55DtOzlW3kduZKRuSo8r7M2NP45l7ZZqT1GO 5xNDtjC72OluZ5OsyNvjC3CPMa5vQUuaaXDYy7MMcJ9RpQ2baRNSZtY0zlx8XHDVXcnW K8eg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=default header.b="DTMl83/U"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=linuxfoundation.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id q18si2524555edb.466.2020.10.28.07.14.28; Wed, 28 Oct 2020 07:14:50 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=default header.b="DTMl83/U"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=linuxfoundation.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1804882AbgJ0P7o (ORCPT + 99 others); Tue, 27 Oct 2020 11:59:44 -0400 Received: from mail.kernel.org ([198.145.29.99]:54348 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1796275AbgJ0PRF (ORCPT ); Tue, 27 Oct 2020 11:17:05 -0400 Received: from localhost (83-86-74-64.cable.dynamic.v4.ziggo.nl [83.86.74.64]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id E756B2224A; Tue, 27 Oct 2020 15:17:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1603811824; bh=iyMsPoeqfLD7Bze7bB14UtH5je5zdl3IIQwf2ExxCUc=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=DTMl83/UYs/8P6EsdM1bdb8ywZhBUk6AyxCyhsVRdumfpQnaY9D4K99dqrUHkD0Hf kKLGw9O4jZaKN4IFD3JK3Ku51cqLp3Z0IVz+KkWWrEF4rFzHSYJXP/9JS9qIDgcGoN jQ+benOW1hIsSwgK9ULFMFW9Z5VRLso/V9KwXdOc= From: Greg Kroah-Hartman To: linux-kernel@vger.kernel.org Cc: Greg Kroah-Hartman , stable@vger.kernel.org, syzbot , Tetsuo Handa , Oliver Neukum , Alan Stern Subject: [PATCH 5.8 630/633] USB: cdc-wdm: Make wdm_flush() interruptible and add wdm_fsync(). Date: Tue, 27 Oct 2020 14:56:13 +0100 Message-Id: <20201027135552.370888254@linuxfoundation.org> X-Mailer: git-send-email 2.29.1 In-Reply-To: <20201027135522.655719020@linuxfoundation.org> References: <20201027135522.655719020@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: linux-kernel@vger.kernel.org From: Oliver Neukum commit 37d2a36394d954413a495da61da1b2a51ecd28ab upstream. syzbot is reporting hung task at wdm_flush() [1], for there is a circular dependency that wdm_flush() from flip_close() for /dev/cdc-wdm0 forever waits for /dev/raw-gadget to be closed while close() for /dev/raw-gadget cannot be called unless close() for /dev/cdc-wdm0 completes. Tetsuo Handa considered that such circular dependency is an usage error [2] which corresponds to an unresponding broken hardware [3]. But Alan Stern responded that we should be prepared for such hardware [4]. Therefore, this patch changes wdm_flush() to use wait_event_interruptible_timeout() which gives up after 30 seconds, for hardware that remains silent must be ignored. The 30 seconds are coming out of thin air. Changing wait_event() to wait_event_interruptible_timeout() makes error reporting from close() syscall less reliable. To compensate it, this patch also implements wdm_fsync() which does not use timeout. Those who want to be very sure that data has gone out to the device are now advised to call fsync(), with a caveat that fsync() can return -EINVAL when running on older kernels which do not implement wdm_fsync(). This patch also fixes three more problems (listed below) found during exhaustive discussion and testing. Since multiple threads can concurrently call wdm_write()/wdm_flush(), we need to use wake_up_all() whenever clearing WDM_IN_USE in order to make sure that all waiters are woken up. Also, error reporting needs to use fetch-and-clear approach in order not to report same error for multiple times. Since wdm_flush() checks WDM_DISCONNECTING, wdm_write() should as well check WDM_DISCONNECTING. In wdm_flush(), since locks are not held, it is not safe to dereference desc->intf after checking that WDM_DISCONNECTING is not set [5]. Thus, remove dev_err() from wdm_flush(). [1] https://syzkaller.appspot.com/bug?id=e7b761593b23eb50855b9ea31e3be5472b711186 [2] https://lkml.kernel.org/r/27b7545e-8f41-10b8-7c02-e35a08eb1611@i-love.sakura.ne.jp [3] https://lkml.kernel.org/r/79ba410f-e0ef-2465-b94f-6b9a4a82adf5@i-love.sakura.ne.jp [4] https://lkml.kernel.org/r/20200530011040.GB12419@rowland.harvard.edu [5] https://lkml.kernel.org/r/c85331fc-874c-6e46-a77f-0ef1dc075308@i-love.sakura.ne.jp Reported-by: syzbot Cc: stable Co-developed-by: Tetsuo Handa Signed-off-by: Tetsuo Handa Signed-off-by: Oliver Neukum Cc: Alan Stern Link: https://lore.kernel.org/r/20200928141755.3476-1-penguin-kernel@I-love.SAKURA.ne.jp Signed-off-by: Greg Kroah-Hartman --- drivers/usb/class/cdc-wdm.c | 70 +++++++++++++++++++++++++++++++++----------- 1 file changed, 54 insertions(+), 16 deletions(-) --- a/drivers/usb/class/cdc-wdm.c +++ b/drivers/usb/class/cdc-wdm.c @@ -58,6 +58,9 @@ MODULE_DEVICE_TABLE (usb, wdm_ids); #define WDM_MAX 16 +/* we cannot wait forever at flush() */ +#define WDM_FLUSH_TIMEOUT (30 * HZ) + /* CDC-WMC r1.1 requires wMaxCommand to be "at least 256 decimal (0x100)" */ #define WDM_DEFAULT_BUFSIZE 256 @@ -151,7 +154,7 @@ static void wdm_out_callback(struct urb kfree(desc->outbuf); desc->outbuf = NULL; clear_bit(WDM_IN_USE, &desc->flags); - wake_up(&desc->wait); + wake_up_all(&desc->wait); } static void wdm_in_callback(struct urb *urb) @@ -393,6 +396,9 @@ static ssize_t wdm_write if (test_bit(WDM_RESETTING, &desc->flags)) r = -EIO; + if (test_bit(WDM_DISCONNECTING, &desc->flags)) + r = -ENODEV; + if (r < 0) { rv = r; goto out_free_mem_pm; @@ -424,6 +430,7 @@ static ssize_t wdm_write if (rv < 0) { desc->outbuf = NULL; clear_bit(WDM_IN_USE, &desc->flags); + wake_up_all(&desc->wait); /* for wdm_wait_for_response() */ dev_err(&desc->intf->dev, "Tx URB error: %d\n", rv); rv = usb_translate_errors(rv); goto out_free_mem_pm; @@ -583,28 +590,58 @@ err: return rv; } -static int wdm_flush(struct file *file, fl_owner_t id) +static int wdm_wait_for_response(struct file *file, long timeout) { struct wdm_device *desc = file->private_data; + long rv; /* Use long here because (int) MAX_SCHEDULE_TIMEOUT < 0. */ - wait_event(desc->wait, - /* - * needs both flags. We cannot do with one - * because resetting it would cause a race - * with write() yet we need to signal - * a disconnect - */ - !test_bit(WDM_IN_USE, &desc->flags) || - test_bit(WDM_DISCONNECTING, &desc->flags)); + /* + * Needs both flags. We cannot do with one because resetting it would + * cause a race with write() yet we need to signal a disconnect. + */ + rv = wait_event_interruptible_timeout(desc->wait, + !test_bit(WDM_IN_USE, &desc->flags) || + test_bit(WDM_DISCONNECTING, &desc->flags), + timeout); - /* cannot dereference desc->intf if WDM_DISCONNECTING */ + /* + * To report the correct error. This is best effort. + * We are inevitably racing with the hardware. + */ if (test_bit(WDM_DISCONNECTING, &desc->flags)) return -ENODEV; - if (desc->werr < 0) - dev_err(&desc->intf->dev, "Error in flush path: %d\n", - desc->werr); + if (!rv) + return -EIO; + if (rv < 0) + return -EINTR; + + spin_lock_irq(&desc->iuspin); + rv = desc->werr; + desc->werr = 0; + spin_unlock_irq(&desc->iuspin); - return usb_translate_errors(desc->werr); + return usb_translate_errors(rv); + +} + +/* + * You need to send a signal when you react to malicious or defective hardware. + * Also, don't abort when fsync() returned -EINVAL, for older kernels which do + * not implement wdm_flush() will return -EINVAL. + */ +static int wdm_fsync(struct file *file, loff_t start, loff_t end, int datasync) +{ + return wdm_wait_for_response(file, MAX_SCHEDULE_TIMEOUT); +} + +/* + * Same with wdm_fsync(), except it uses finite timeout in order to react to + * malicious or defective hardware which ceased communication after close() was + * implicitly called due to process termination. + */ +static int wdm_flush(struct file *file, fl_owner_t id) +{ + return wdm_wait_for_response(file, WDM_FLUSH_TIMEOUT); } static __poll_t wdm_poll(struct file *file, struct poll_table_struct *wait) @@ -729,6 +766,7 @@ static const struct file_operations wdm_ .owner = THIS_MODULE, .read = wdm_read, .write = wdm_write, + .fsync = wdm_fsync, .open = wdm_open, .flush = wdm_flush, .release = wdm_release,