| 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
| ||
|
Message-ID: <Z2BgQ54+QOOib/Fe@lzaremba-mobl.ger.corp.intel.com>
Date: Mon, 16 Dec 2024 18:15:47 +0100
From: Larysa Zaremba <larysa.zaremba@...el.com>
To: Parthiban Veerasooran <parthiban.veerasooran@...rochip.com>
CC: <andrew+netdev@...n.ch>, <davem@...emloft.net>, <edumazet@...gle.com>,
<kuba@...nel.org>, <pabeni@...hat.com>, <netdev@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <UNGLinuxDriver@...rochip.com>,
<jacob.e.keller@...el.com>
Subject: Re: [PATCH net v4 2/2] net: ethernet: oa_tc6: fix tx skb race
condition between reference pointers
On Fri, Dec 13, 2024 at 06:01:59PM +0530, Parthiban Veerasooran wrote:
> There are two skb pointers to manage tx skb's enqueued from n/w stack.
> waiting_tx_skb pointer points to the tx skb which needs to be processed
> and ongoing_tx_skb pointer points to the tx skb which is being processed.
>
> SPI thread prepares the tx data chunks from the tx skb pointed by the
> ongoing_tx_skb pointer. When the tx skb pointed by the ongoing_tx_skb is
> processed, the tx skb pointed by the waiting_tx_skb is assigned to
> ongoing_tx_skb and the waiting_tx_skb pointer is assigned with NULL.
> Whenever there is a new tx skb from n/w stack, it will be assigned to
> waiting_tx_skb pointer if it is NULL. Enqueuing and processing of a tx skb
> handled in two different threads.
>
> Consider a scenario where the SPI thread processed an ongoing_tx_skb and
> it moves next tx skb from waiting_tx_skb pointer to ongoing_tx_skb pointer
> without doing any NULL check. At this time, if the waiting_tx_skb pointer
> is NULL then ongoing_tx_skb pointer is also assigned with NULL. After
> that, if a new tx skb is assigned to waiting_tx_skb pointer by the n/w
> stack and there is a chance to overwrite the tx skb pointer with NULL in
> the SPI thread. Finally one of the tx skb will be left as unhandled,
> resulting packet missing and memory leak.
>
> - Consider the below scenario where the TXC reported from the previous
> transfer is 10 and ongoing_tx_skb holds an tx ethernet frame which can be
> transported in 20 TXCs and waiting_tx_skb is still NULL.
> tx_credits = 10; /* 21 are filled in the previous transfer */
> ongoing_tx_skb = 20;
> waiting_tx_skb = NULL; /* Still NULL */
> - So, (tc6->ongoing_tx_skb || tc6->waiting_tx_skb) becomes true.
> - After oa_tc6_prepare_spi_tx_buf_for_tx_skbs()
> ongoing_tx_skb = 10;
> waiting_tx_skb = NULL; /* Still NULL */
> - Perform SPI transfer.
> - Process SPI rx buffer to get the TXC from footers.
> - Now let's assume previously filled 21 TXCs are freed so we are good to
> transport the next remaining 10 tx chunks from ongoing_tx_skb.
> tx_credits = 21;
> ongoing_tx_skb = 10;
> waiting_tx_skb = NULL;
> - So, (tc6->ongoing_tx_skb || tc6->waiting_tx_skb) becomes true again.
> - In the oa_tc6_prepare_spi_tx_buf_for_tx_skbs()
> ongoing_tx_skb = NULL;
> waiting_tx_skb = NULL;
>
> - Now the below bad case might happen,
>
> Thread1 (oa_tc6_start_xmit) Thread2 (oa_tc6_spi_thread_handler)
> --------------------------- -----------------------------------
> - if waiting_tx_skb is NULL
> - if ongoing_tx_skb is NULL
> - ongoing_tx_skb = waiting_tx_skb
> - waiting_tx_skb = skb
> - waiting_tx_skb = NULL
> ...
> - ongoing_tx_skb = NULL
> - if waiting_tx_skb is NULL
> - waiting_tx_skb = skb
>
> To overcome the above issue, protect the moving of tx skb reference from
> waiting_tx_skb pointer to ongoing_tx_skb pointer and assigning new tx skb
> to waiting_tx_skb pointer, so that the other thread can't access the
> waiting_tx_skb pointer until the current thread completes moving the tx
> skb reference safely.
>
This fixes the above race condition and spin_lock_bh() is appropriate.
Reviewed-by: Larysa Zaremba <larysa.zaremba@...el.com>
> Fixes: 53fbde8ab21e ("net: ethernet: oa_tc6: implement transmit path to transfer tx ethernet frames")
> Signed-off-by: Parthiban Veerasooran <parthiban.veerasooran@...rochip.com>
> ---
> drivers/net/ethernet/oa_tc6.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
> index 4c8b0ca922b7..db200e4ec284 100644
> --- a/drivers/net/ethernet/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6.c
> @@ -113,6 +113,7 @@ struct oa_tc6 {
> struct mii_bus *mdiobus;
> struct spi_device *spi;
> struct mutex spi_ctrl_lock; /* Protects spi control transfer */
> + spinlock_t tx_skb_lock; /* Protects tx skb handling */
> void *spi_ctrl_tx_buf;
> void *spi_ctrl_rx_buf;
> void *spi_data_tx_buf;
> @@ -1004,8 +1005,10 @@ 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++) {
> 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);
> }
> if (!tc6->ongoing_tx_skb)
> break;
> @@ -1210,7 +1213,9 @@ netdev_tx_t oa_tc6_start_xmit(struct oa_tc6 *tc6, struct sk_buff *skb)
> return NETDEV_TX_OK;
> }
>
> + spin_lock_bh(&tc6->tx_skb_lock);
> tc6->waiting_tx_skb = skb;
> + spin_unlock_bh(&tc6->tx_skb_lock);
>
> /* Wake spi kthread to perform spi transfer */
> wake_up_interruptible(&tc6->spi_wq);
> @@ -1240,6 +1245,7 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev)
> tc6->netdev = netdev;
> SET_NETDEV_DEV(netdev, &spi->dev);
> mutex_init(&tc6->spi_ctrl_lock);
> + spin_lock_init(&tc6->tx_skb_lock);
>
> /* Set the SPI controller to pump at realtime priority */
> tc6->spi->rt = true;
> --
> 2.34.1
>
>
Powered by blists - more mailing lists