From patchwork Thu Jan 5 12:40:59 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bartosz Golaszewski X-Patchwork-Id: 639663 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 E335DC3DA7A for ; Thu, 5 Jan 2023 12:41:28 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232344AbjAEMlN (ORCPT ); Thu, 5 Jan 2023 07:41:13 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35320 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231757AbjAEMlL (ORCPT ); Thu, 5 Jan 2023 07:41:11 -0500 Received: from mail-wm1-x32b.google.com (mail-wm1-x32b.google.com [IPv6:2a00:1450:4864:20::32b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4DCF2338 for ; Thu, 5 Jan 2023 04:41:08 -0800 (PST) Received: by mail-wm1-x32b.google.com with SMTP id ja17so27904282wmb.3 for ; Thu, 05 Jan 2023 04:41:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bgdev-pl.20210112.gappssmtp.com; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=mE6v31ZZ0E7oy+OxwP/3qLKCpkH38/LEX5bi/JWcPpw=; b=ISRv+h7juXHTqYp3y1UgeXJZQePwb4ygyPPIZJjxYN/vKNjuRkuStN+CksM+Mew8Zr SX8Y89KF1G5f4H5gtA/a3Z4Prd3ODlKqNKCUsOcLnCd66P5M4d5v4ulwxfrihH8Xjrw5 iSPfS2FmvtO7C9csfEpEfBTZBatTq18clmfZeieQFgCD51/4CzK20MCMiek5kBY5b7sq aKvIhEDnvKMeiPQQZREJ2eKWb/rgb48nMCjDUQ7ZdR4w7G5pXpq6IN2b6BpumpHWtZgK 4X50varBbauB6A0InRc/Y/AN3Uwiq6nV97q5hT2UjPa3/Exk4nuJcvarMFgOWyGNW2ta htCQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=mE6v31ZZ0E7oy+OxwP/3qLKCpkH38/LEX5bi/JWcPpw=; b=2Hs22kTwgNjEdG2O7sD6xiSqHp8tFdBd0DVccPChvQhrYaaaEf2vNe1jMz6P76ZbCL lZmrdlijX8ghXJnMXtVhJ/ORhjzR1DG57bPOb7fMpiD+TlJ9k5dlHqIKdzCVebF9T6cY sdFFdkDl0E41vrf6Z/czIdcWXqnua9AjPF6jH7DbwOzp7Rf7UtNhwxShmsNi3TkSe5Rn DSdKKo9Mb0yY1HQaoanzdVtYi09ParXaF4dBa5X4YWU5GppvwnH2kpYanHwsSL1HvKQq VE/lUW//wTEeQuTN1eytJYWPt0ZUL3TdfsoN4JdC+bKFzPfoILPpE2B37YwngpDkapaA M7xA== X-Gm-Message-State: AFqh2koye9132AqzW2lJ3cVRzbjq808nnlvk3HimioK4uPbYsNxoaEj/ QcLvFtqFEIq/C+lluWQJPlgXrw== X-Google-Smtp-Source: AMrXdXuAxloSp3gFtgCQktpymTTqK+iVZhI0eQWrCdYEZ5d15gm+znCn2Gd59w7mIdU7uothWv9ohg== X-Received: by 2002:a05:600c:4da0:b0:3d2:3a53:2cd6 with SMTP id v32-20020a05600c4da000b003d23a532cd6mr36715161wmp.9.1672922466802; Thu, 05 Jan 2023 04:41:06 -0800 (PST) Received: from brgl-uxlite.home ([2a01:cb1d:334:ac00:101:3196:3a40:6416]) by smtp.gmail.com with ESMTPSA id m6-20020a1c2606000000b003b47e75b401sm2338153wmm.37.2023.01.05.04.41.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 05 Jan 2023 04:41:06 -0800 (PST) From: Bartosz Golaszewski To: Mark Brown Cc: linux-spi@vger.kernel.org, linux-kernel@vger.kernel.org, Bartosz Golaszewski Subject: [PATCH 1/2] spi: spidev: fix a race condition when accessing spidev->spi Date: Thu, 5 Jan 2023 13:40:59 +0100 Message-Id: <20230105124100.185542-1-brgl@bgdev.pl> X-Mailer: git-send-email 2.37.2 MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: linux-spi@vger.kernel.org From: Bartosz Golaszewski There's a spinlock in place that is taken in file_operations callbacks whenever we check if spidev->spi is still alive (not null). It's also taken when spidev->spi is set to NULL in remove(). This however doesn't protect the code against driver unbind event while one of the syscalls is still in progress. To that end we need a lock taken continuously as long as we may still access spidev->spi. As both the file ops and the remove callback are never called from interrupt context, we can replace the spinlock with a sleeping lock. Using an RW semaphore allows the syscalls to run concurrently unless protected otherwise. We take it for writing only when setting spidev->spi to null, while everywhere else it's only taken for reading. This assures that it will be dropped only once all currently executed syscalls have returned. Signed-off-by: Bartosz Golaszewski --- drivers/spi/spidev.c | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c index 6313e7d0cdf8..b71620f64ec9 100644 --- a/drivers/spi/spidev.c +++ b/drivers/spi/spidev.c @@ -18,6 +18,7 @@ #include #include #include +#include #include #include @@ -68,7 +69,7 @@ static_assert(N_SPI_MINORS > 0 && N_SPI_MINORS <= 256); struct spidev_data { dev_t devt; - spinlock_t spi_lock; + struct rw_semaphore sem; struct spi_device *spi; struct list_head device_entry; @@ -95,9 +96,8 @@ spidev_sync(struct spidev_data *spidev, struct spi_message *message) int status; struct spi_device *spi; - spin_lock_irq(&spidev->spi_lock); + down_read(&spidev->sem); spi = spidev->spi; - spin_unlock_irq(&spidev->spi_lock); if (spi == NULL) status = -ESHUTDOWN; @@ -107,6 +107,7 @@ spidev_sync(struct spidev_data *spidev, struct spi_message *message) if (status == 0) status = message->actual_length; + up_read(&spidev->sem); return status; } @@ -359,12 +360,12 @@ spidev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) * we issue this ioctl. */ spidev = filp->private_data; - spin_lock_irq(&spidev->spi_lock); + down_read(&spidev->sem); spi = spi_dev_get(spidev->spi); - spin_unlock_irq(&spidev->spi_lock); - - if (spi == NULL) + if (spi == NULL) { + up_read(&spidev->sem); return -ESHUTDOWN; + } /* use the buffer lock here for triple duty: * - prevent I/O (from us) so calling spi_setup() is safe; @@ -508,6 +509,7 @@ spidev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) mutex_unlock(&spidev->buf_lock); spi_dev_put(spi); + up_read(&spidev->sem); return retval; } @@ -529,12 +531,12 @@ spidev_compat_ioc_message(struct file *filp, unsigned int cmd, * we issue this ioctl. */ spidev = filp->private_data; - spin_lock_irq(&spidev->spi_lock); + down_read(&spidev->sem); spi = spi_dev_get(spidev->spi); - spin_unlock_irq(&spidev->spi_lock); - - if (spi == NULL) + if (spi == NULL) { + up_read(&spidev->sem); return -ESHUTDOWN; + } /* SPI_IOC_MESSAGE needs the buffer locked "normally" */ mutex_lock(&spidev->buf_lock); @@ -561,6 +563,7 @@ spidev_compat_ioc_message(struct file *filp, unsigned int cmd, done: mutex_unlock(&spidev->buf_lock); spi_dev_put(spi); + up_read(&spidev->sem); return retval; } @@ -640,10 +643,10 @@ static int spidev_release(struct inode *inode, struct file *filp) spidev = filp->private_data; filp->private_data = NULL; - spin_lock_irq(&spidev->spi_lock); + down_read(&spidev->sem); /* ... after we unbound from the underlying device? */ dofree = (spidev->spi == NULL); - spin_unlock_irq(&spidev->spi_lock); + up_read(&spidev->sem); /* last close? */ spidev->users--; @@ -776,7 +779,7 @@ static int spidev_probe(struct spi_device *spi) /* Initialize the driver data */ spidev->spi = spi; - spin_lock_init(&spidev->spi_lock); + init_rwsem(&spidev->sem); mutex_init(&spidev->buf_lock); INIT_LIST_HEAD(&spidev->device_entry); @@ -821,9 +824,9 @@ static void spidev_remove(struct spi_device *spi) /* prevent new opens */ mutex_lock(&device_list_lock); /* make sure ops on existing fds can abort cleanly */ - spin_lock_irq(&spidev->spi_lock); + down_write(&spidev->sem); spidev->spi = NULL; - spin_unlock_irq(&spidev->spi_lock); + up_write(&spidev->sem); list_del(&spidev->device_entry); device_destroy(spidev_class, spidev->devt); From patchwork Thu Jan 5 12:41:00 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bartosz Golaszewski X-Patchwork-Id: 640014 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 596F7C3DA7D for ; Thu, 5 Jan 2023 12:41:31 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232020AbjAEMl3 (ORCPT ); Thu, 5 Jan 2023 07:41:29 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35322 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232319AbjAEMlM (ORCPT ); Thu, 5 Jan 2023 07:41:12 -0500 Received: from mail-wm1-x332.google.com (mail-wm1-x332.google.com [IPv6:2a00:1450:4864:20::332]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 09A2E16481 for ; Thu, 5 Jan 2023 04:41:09 -0800 (PST) Received: by mail-wm1-x332.google.com with SMTP id m3so19011097wmq.0 for ; Thu, 05 Jan 2023 04:41:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bgdev-pl.20210112.gappssmtp.com; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=8mYBOZhut1McL5t8U9bUpMD48gPtdONvdoyZEUsQo0o=; b=Dlp6rSddGlTIAmt3GjVdQUjqvXqHbbIQ0Z6RpA/KuOjU8qPQC7bM+7h/KMe5zPJUqJ 9YM8XL/y6FGIAwWXjo+1x12QcywGZ2U4GotPozZ+GshXAFQjeWdGZmGFqRnLjaxgTKgL MxKtflXIebEXsjwadfP3APnkiJSyzJfZqfpMKXk+F4IiJ1wAWM17yrH/39EW3u5OJGMp UIGV2bJ7JrG+QgMkE+D5hS4FBxjagbznbfry/SG8c0cNLhMBy3JUC/TFHC+73g3SsVei m0R68KIaUbYhb+xTfIw/Y6rFGKYJ+WheQewYrxPEqhkli4qCMK0nq+hFy4lzXaHushal NisQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=8mYBOZhut1McL5t8U9bUpMD48gPtdONvdoyZEUsQo0o=; b=xwasZ8BGg2jtxjF98cKDP0K0HLLSxQubtMN6QUAEeETR8mTl3E/4C7KYdceAMiC4IE HL/nwwPNmH3NUOyaWYvyZkfPE6ZKK6JCDs+yRqxDPLhu0/jb2dGihzDqJynVCycxInZV ncIOqrPQ7B/t+53jSCgHQuQsmHOdHXryhKc1UXDZ0bFa/AUo6tAhIskEabX7POaiwrRJ RjX+jC8W7iDxXp4XfpBkNvPjNQ5O9WLPkuWNmtAP25OthHWp+EzW5zPq1nVSti3kHbds BGD2scOWlqaqtwDShYJOp+xu+b0ipSo7TyjULxZULm/yCODjtvjRrBLB77z0C1VKfiOu 6MtA== X-Gm-Message-State: AFqh2kowtOTlG8JyCGSbY26J457BoFGWzilqBK3SaPKWAA7TL6iYaqUb xv5uiLItP2y1Y/r3V/fIH+FDng== X-Google-Smtp-Source: AMrXdXuq1HF7/uhu/I0YpqGMQ8foibyaZRFMZfOLIr0fYPMW1O1rUDo7v6FWfImNpze9+pjju43llg== X-Received: by 2002:a05:600c:a4d:b0:3cf:6e78:e2ca with SMTP id c13-20020a05600c0a4d00b003cf6e78e2camr39888024wmq.5.1672922467613; Thu, 05 Jan 2023 04:41:07 -0800 (PST) Received: from brgl-uxlite.home ([2a01:cb1d:334:ac00:101:3196:3a40:6416]) by smtp.gmail.com with ESMTPSA id m6-20020a1c2606000000b003b47e75b401sm2338153wmm.37.2023.01.05.04.41.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 05 Jan 2023 04:41:07 -0800 (PST) From: Bartosz Golaszewski To: Mark Brown Cc: linux-spi@vger.kernel.org, linux-kernel@vger.kernel.org, Bartosz Golaszewski Subject: [PATCH 2/2] spi: spidev: remove debug messages that access spidev->spi without locking Date: Thu, 5 Jan 2023 13:41:00 +0100 Message-Id: <20230105124100.185542-2-brgl@bgdev.pl> X-Mailer: git-send-email 2.37.2 In-Reply-To: <20230105124100.185542-1-brgl@bgdev.pl> References: <20230105124100.185542-1-brgl@bgdev.pl> MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: linux-spi@vger.kernel.org From: Bartosz Golaszewski The two debug messages in spidev_open() dereference spidev->spi without taking the semaphore and without checking if it's not null. This can lead to a crash. Drop the messages as they're not needed - the user-space will get informed about ENOMEM with the syscall return value. Signed-off-by: Bartosz Golaszewski --- drivers/spi/spidev.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c index b71620f64ec9..29c6344ee8e8 100644 --- a/drivers/spi/spidev.c +++ b/drivers/spi/spidev.c @@ -604,7 +604,6 @@ static int spidev_open(struct inode *inode, struct file *filp) if (!spidev->tx_buffer) { spidev->tx_buffer = kmalloc(bufsiz, GFP_KERNEL); if (!spidev->tx_buffer) { - dev_dbg(&spidev->spi->dev, "open/ENOMEM\n"); status = -ENOMEM; goto err_find_dev; } @@ -613,7 +612,6 @@ static int spidev_open(struct inode *inode, struct file *filp) if (!spidev->rx_buffer) { spidev->rx_buffer = kmalloc(bufsiz, GFP_KERNEL); if (!spidev->rx_buffer) { - dev_dbg(&spidev->spi->dev, "open/ENOMEM\n"); status = -ENOMEM; goto err_alloc_rx_buf; }