netdev - [PATCH net-next] net: ethernet: oa_tc6: fix race condition on ongoing_tx

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-next>] [day] [month] [year] [list]

Message-Id: <20241219065926.1051732-1-dheeraj.linuxdev@gmail.com>
Date: Thu, 19 Dec 2024 12:29:26 +0530
From: Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@...il.com>
To: parthiban.veerasooran@...rochip.com,
	netdev@...r.kernel.org
Cc: andrew+netdev@...n.ch,
	davem@...emloft.net,
	edumazet@...gle.com,
	kuba@...nel.org,
	pabeni@...hat.com,
	linux-kernel@...r.kernel.org,
	Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@...il.com>
Subject: [PATCH net-next] net: ethernet: oa_tc6: fix race condition on ongoing_tx_skb

A race condition exists in function oa_tc6_prepare_spi_tx_buf_for_tx_skbs
due to an unsynchronized access to shared variable tc6->ongoing_tx_skb.

The issue arises because the condition (!tc6->ongoing_tx_skb) is checked
outside the critical section. Two or more threads can simultaneously
evaluate this condition as true before acquiring the lock. This results
in both threads entering the critical section and modifying
tc6->ongoing_tx_skb, causing inconsistent state updates or overwriting
each other's changes.

Consider the following scenario. A race window exists in the sequence:

   Thread1 			 Thread2
   ------------------------      ------------------------
   - if ongoing_tx_skb is NULL
                                   - if ongoing_tx_skb is NULL
                                   - spin_lock_bh()
                                   - ongoing_tx_skb = waiting_tx_skb
                                   - waiting_tx_skb = NULL
                                   - spin_unlock_bh()
   - spin_lock_bh()
   - ongoing_tx_skb = waiting_tx_skb
   - waiting_tx_skb = NULL
   - spin_unlock_bh()

This leads to lost updates between ongoing_tx_skb and waiting_tx_skb
fields. Moving the NULL check inside the critical section ensures both
the NULL check and the assignment are protected by the same lock,
maintaining atomic check-and-set operations.

Fixes: e592b5110b3e ("net: ethernet: oa_tc6: fix tx skb race condition between reference pointers")
Closes: https://scan7.scan.coverity.com/#/project-view/52337/11354?selectedIssue=1602611
Signed-off-by: Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@...il.com>
---
 drivers/net/ethernet/oa_tc6.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
index db200e4ec284..66d55ec9bc88 100644
--- a/drivers/net/ethernet/oa_tc6.c
+++ b/drivers/net/ethernet/oa_tc6.c
@@ -1004,12 +1004,12 @@ static u16 oa_tc6_prepare_spi_tx_buf_for_tx_skbs(struct oa_tc6 *tc6)
 	 */
 	for (used_tx_credits = 0; used_tx_credits < tc6->tx_credits;
 	     used_tx_credits++) {
+		spin_lock_bh(&tc6->tx_skb_lock);
 		if (!tc6->ongoing_tx_skb) {
-			spin_lock_bh(&tc6->tx_skb_lock);
 			tc6->ongoing_tx_skb = tc6->waiting_tx_skb;
 			tc6->waiting_tx_skb = NULL;
-			spin_unlock_bh(&tc6->tx_skb_lock);
 		}
+		spin_unlock_bh(&tc6->tx_skb_lock);
 		if (!tc6->ongoing_tx_skb)
 			break;
 		oa_tc6_add_tx_skb_to_spi_buf(tc6);
-- 
2.34.1