linux-kernel - [RFT PATCH 8/9] drivers: qcom: rpmh-rsc: spin_lock_irqsave() for tcs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20200306155707.RFT.8.I07c1f70e0e8f2dc0004bd38970b4e258acdc773e@changeid>
Date:   Fri,  6 Mar 2020 15:59:50 -0800
From:   Douglas Anderson <dianders@...omium.org>
To:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Maulik Shah <mkshah@...eaurora.org>
Cc:     Rajendra Nayak <rnayak@...eaurora.org>, mka@...omium.org,
        evgreen@...omium.org, swboyd@...omium.org,
        Lina Iyer <ilina@...eaurora.org>,
        Douglas Anderson <dianders@...omium.org>,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: [RFT PATCH 8/9] drivers: qcom: rpmh-rsc: spin_lock_irqsave() for tcs_invalidate()

Auditing tcs_invalidate() made me worried.  Specifically I saw that it
used spin_lock(), not spin_lock_irqsave().  That always worries me.

As I understand it, using spin_lock() is only valid in these
situations:

a) You know you are running in the interrupt handler (and all other
   users of the lock use the "irqsave" variant).
b) You know that nobody using the lock is ever running in the
   interrupt handler.
c) You know that someone else has always disabled interrupts before
   your code runs and thus the "irqsave" variant is pointless.

>From auditing the driver we look OK.  ...except that there is one
further corner case.  If sometimes your code is called with IRQs
disabled and sometimes it's not you will get in trouble if someone
ever boots your board with "nosmp" (AKA in uniprocessor mode).  In
such a case if someone else has the lock (without disabling
interrupts) and they get swapped out then your code (with interrupts
disabled) might loop forever waiting for the spinlock.

It's just safer to use the irqsave version, so let's do that.  In
future patches I believe tcs_invalidate() will always be called with
interrupts off anyway.

Signed-off-by: Douglas Anderson <dianders@...omium.org>
---

 drivers/soc/qcom/rpmh-rsc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 693873fce895..0297da5ceeaa 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -210,9 +210,10 @@ static bool tcs_is_free(struct rsc_drv *drv, int tcs_id)
 static int tcs_invalidate(struct rsc_drv *drv, int type)
 {
 	int m;
+	unsigned long flags;
 	struct tcs_group *tcs = &drv->tcs[type];
 
-	spin_lock(&tcs->lock);
+	spin_lock_irqsave(&tcs->lock, flags);
 	if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
 		spin_unlock(&tcs->lock);
 		return 0;
@@ -227,7 +228,7 @@ static int tcs_invalidate(struct rsc_drv *drv, int type)
 		write_tcs_reg_sync(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0);
 	}
 	bitmap_zero(tcs->slots, MAX_TCS_SLOTS);
-	spin_unlock(&tcs->lock);
+	spin_unlock_irqrestore(&tcs->lock, flags);
 
 	return 0;
 }
-- 
2.25.1.481.gfbce0eb801-goog