Received: by 2002:a05:6a10:9848:0:0:0:0 with SMTP id x8csp3528387pxf; Mon, 15 Mar 2021 11:33:55 -0700 (PDT) X-Google-Smtp-Source: ABdhPJz+BdhIp9EljB8p390mxoxcP/SzmhHU9qo0bkI6sCeH7kxmDuTF/SRWN/mHdkKPElVOKzeC X-Received: by 2002:a05:6402:30a5:: with SMTP id df5mr31860425edb.24.1615833235593; Mon, 15 Mar 2021 11:33:55 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1615833235; cv=none; d=google.com; s=arc-20160816; b=maLrj6wXz/8HzVp0eA16m50sUI8VuvAlM87QWzl08Ytv4Ecv1CnDo0OritVcJB7XWh EZ0KrrS7RckC8UCzyAU/6IzyhEz+TPJUlCd4BaVQpjNCqLd2z+wKHgJuD2VcjE4F8zEV BuXnN8rQI4hKWgBUiIoKL43tWI7N4734Yv9hPyLdCN1zNImYO7FAoaKryF60BZZynfy0 pCDJ+nArH3t+FIZPABBWKL5j6eCsk5dIlK50j8ZjPokGHen+EqYcNCwUmPYg7sbJxjIl KCKA0Q5wvTGWtxu5EVAjSRsfBWSrVr6ksXQSomYgaI4t2hgpwIz4UZOqvaMzNCyuvanT CHow== 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=VCjOT+2gteimtlaNwH7E/V0IDdr4W1ov4YDhzvmTkQI=; b=WnEpGV4G3Ghdub92GfAh5r7TFJCT6mbHXMDBauZErSSu3PKCk88zS0oqpr24fJprSY 7XGL7bvFy98EIPF1kCAFw7sUZ93Kk0IoXi0loEgftlBtig9ie7i3nUR2/o38ArXa67Tc 3v0E6d/kIcupkZLAgBRZssDEZnxaGVjCT8Y7hXovWYZwhf2Bf0/IgHhcM2QKOlpVnR3p WGO3io+oepaMWHzT6fqgXUwusVxXDfJYHQkmuPToz9kMk4lDJJaOKLzyl6inrviSY4Zg d4iICsTbN+6KZmOoBBRCcYv3+VZi/7+ogrn+Xvuq9n+aUHP5o7pmXTgIbUBx1qV61eXL LhcQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linuxfoundation.org header.s=korg header.b=pDH9fAEt; 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=pass (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 a8si11890107ejt.44.2021.03.15.11.33.33; Mon, 15 Mar 2021 11:33:55 -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=@linuxfoundation.org header.s=korg header.b=pDH9fAEt; 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=pass (p=NONE sp=NONE dis=NONE) header.from=linuxfoundation.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237943AbhCOOln (ORCPT + 99 others); Mon, 15 Mar 2021 10:41:43 -0400 Received: from mail.kernel.org ([198.145.29.99]:49340 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234055AbhCOOCw (ORCPT ); Mon, 15 Mar 2021 10:02:52 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id 0B15464EEE; Mon, 15 Mar 2021 14:02:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1615816971; bh=Fhgwe28ATRz/2WBldTQblfj5onW69dG5dekJ9gMPgiE=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=pDH9fAEtu+lscKC1dOnKyE6HVIUwtfLdO2sMidR6JIM/90IdU9HQWurref+55CDSw G0Kl7IiqwqhJZrUWnFjajuX05emTUl4prOBSqLgJFT7lMMp9DzT7lulTG4QTHbWqRE hDPg+eDc/NLWRZbWM9/5wbq8kqYyRkmzzx/RVNTw= From: gregkh@linuxfoundation.org To: linux-kernel@vger.kernel.org Cc: Greg Kroah-Hartman , stable@vger.kernel.org, syzbot , syzbot , syzbot , Tetsuo Handa , Shuah Khan Subject: [PATCH 5.10 220/290] usbip: fix vhci_hcd attach_store() races leading to gpf Date: Mon, 15 Mar 2021 14:55:13 +0100 Message-Id: <20210315135549.402933956@linuxfoundation.org> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20210315135541.921894249@linuxfoundation.org> References: <20210315135541.921894249@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: Greg Kroah-Hartman From: Shuah Khan commit 718ad9693e3656120064b715fe931f43a6201e67 upstream. attach_store() is invoked when user requests import (attach) a device from usbip host. Attach and detach are governed by local state and shared state - Shared state (usbip device status) - Device status is used to manage the attach and detach operations on import-able devices. - Local state (tcp_socket, rx and tx thread task_struct ptrs) A valid tcp_socket controls rx and tx thread operations while the device is in exported state. - Device has to be in the right state to be attached and detached. Attach sequence includes validating the socket and creating receive (rx) and transmit (tx) threads to talk to the host to get access to the imported device. rx and tx threads depends on local and shared state to be correct and in sync. Detach sequence shuts the socket down and stops the rx and tx threads. Detach sequence relies on local and shared states to be in sync. There are races in updating the local and shared status in the current attach sequence resulting in crashes. These stem from starting rx and tx threads before local and global state is updated correctly to be in sync. 1. Doesn't handle kthread_create() error and saves invalid ptr in local state that drives rx and tx threads. 2. Updates tcp_socket and sockfd, starts stub_rx and stub_tx threads before updating usbip_device status to VDEV_ST_NOTASSIGNED. This opens up a race condition between the threads, port connect, and detach handling. Fix the above problems: - Stop using kthread_get_run() macro to create/start threads. - Create threads and get task struct reference. - Add kthread_create() failure handling and bail out. - Hold vhci and usbip_device locks to update local and shared states after creating rx and tx threads. - Update usbip_device status to VDEV_ST_NOTASSIGNED. - Update usbip_device tcp_socket, sockfd, tcp_rx, and tcp_tx - Start threads after usbip_device (tcp_socket, sockfd, tcp_rx, tcp_tx, and status) is complete. Credit goes to syzbot and Tetsuo Handa for finding and root-causing the kthread_get_run() improper error handling problem and others. This is hard problem to find and debug since the races aren't seen in a normal case. Fuzzing forces the race window to be small enough for the kthread_get_run() error path bug and starting threads before updating the local and shared state bug in the attach sequence. - Update usbip_device tcp_rx and tcp_tx pointers holding vhci and usbip_device locks. Tested with syzbot reproducer: - https://syzkaller.appspot.com/text?tag=ReproC&x=14801034d00000 Fixes: 9720b4bc76a83807 ("staging/usbip: convert to kthread") Cc: stable@vger.kernel.org Reported-by: syzbot Reported-by: syzbot Reported-by: syzbot Reported-by: Tetsuo Handa Signed-off-by: Shuah Khan Link: https://lore.kernel.org/r/bb434bd5d7a64fbec38b5ecfb838a6baef6eb12b.1615171203.git.skhan@linuxfoundation.org Signed-off-by: Greg Kroah-Hartman --- drivers/usb/usbip/vhci_sysfs.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) --- a/drivers/usb/usbip/vhci_sysfs.c +++ b/drivers/usb/usbip/vhci_sysfs.c @@ -312,6 +312,8 @@ static ssize_t attach_store(struct devic struct vhci *vhci; int err; unsigned long flags; + struct task_struct *tcp_rx = NULL; + struct task_struct *tcp_tx = NULL; /* * @rhport: port number of vhci_hcd @@ -360,9 +362,24 @@ static ssize_t attach_store(struct devic return -EINVAL; } - /* now need lock until setting vdev status as used */ + /* create threads before locking */ + tcp_rx = kthread_create(vhci_rx_loop, &vdev->ud, "vhci_rx"); + if (IS_ERR(tcp_rx)) { + sockfd_put(socket); + return -EINVAL; + } + tcp_tx = kthread_create(vhci_tx_loop, &vdev->ud, "vhci_tx"); + if (IS_ERR(tcp_tx)) { + kthread_stop(tcp_rx); + sockfd_put(socket); + return -EINVAL; + } + + /* get task structs now */ + get_task_struct(tcp_rx); + get_task_struct(tcp_tx); - /* begin a lock */ + /* now begin lock until setting vdev status set */ spin_lock_irqsave(&vhci->lock, flags); spin_lock(&vdev->ud.lock); @@ -372,6 +389,8 @@ static ssize_t attach_store(struct devic spin_unlock_irqrestore(&vhci->lock, flags); sockfd_put(socket); + kthread_stop_put(tcp_rx); + kthread_stop_put(tcp_tx); dev_err(dev, "port %d already used\n", rhport); /* @@ -390,14 +409,16 @@ static ssize_t attach_store(struct devic vdev->speed = speed; vdev->ud.sockfd = sockfd; vdev->ud.tcp_socket = socket; + vdev->ud.tcp_rx = tcp_rx; + vdev->ud.tcp_tx = tcp_tx; vdev->ud.status = VDEV_ST_NOTASSIGNED; spin_unlock(&vdev->ud.lock); spin_unlock_irqrestore(&vhci->lock, flags); /* end the lock */ - vdev->ud.tcp_rx = kthread_get_run(vhci_rx_loop, &vdev->ud, "vhci_rx"); - vdev->ud.tcp_tx = kthread_get_run(vhci_tx_loop, &vdev->ud, "vhci_tx"); + wake_up_process(vdev->ud.tcp_rx); + wake_up_process(vdev->ud.tcp_tx); rh_port_connect(vdev, speed);