linux-kernel - Re: [PATCH net v2 2/2] net: ethernet: oa_tc6: fix tx skb race condition between reference pointers

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 for Android: free password hash cracker in your pocket

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

Message-ID: <8f06872b-1c6f-47fb-a82f-7d66a6b1c49b@microchip.com>
Date: Wed, 27 Nov 2024 10:49:10 +0000
From: <Parthiban.Veerasooran@...rochip.com>
To: <pabeni@...hat.com>
CC: <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<UNGLinuxDriver@...rochip.com>, <jacob.e.keller@...el.com>,
	<andrew+netdev@...n.ch>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<kuba@...nel.org>
Subject: Re: [PATCH net v2 2/2] net: ethernet: oa_tc6: fix tx skb race
 condition between reference pointers

Hi Paolo,

On 26/11/24 4:11 pm, Paolo Abeni wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 11/22/24 11:21, 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.
>> To overcome the above issue, protect the moving of tx skb reference from
>> waiting_tx_skb pointer to ongoing_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.
> 
> A mutex looks overkill. Why don't you use a spinlock? why locking only
> one side (the writer) would be enough?
Ah my bad, missed to protect tc6->waiting_tx_skb = skb. So that it will 
become like below,

mutex_lock(&tc6->tx_skb_lock);
tc6->waiting_tx_skb = skb;
mutex_unlock(&tc6->tx_skb_lock);

As both are not called from atomic context and they are allowed to 
sleep, I used mutex rather than spinlock.
> 
> Could you please report the exact sequence of events in a time diagram
> leading to the bug, something alike the following?
> 
> CPU0                                    CPU1
> oa_tc6_start_xmit
>   ...
>                                          oa_tc6_spi_thread_handler
>                                           ...
Good case:
----------
Consider waiting_tx_skb is NULL.

Thread1 (oa_tc6_start_xmit)	Thread2 (oa_tc6_spi_thread_handler)
---------------------------	-----------------------------------
- if waiting_tx_skb is NULL
- waiting_tx_skb = skb
				- if ongoing_tx_skb is NULL
				- ongoing_tx_skb = waiting_tx_skb
				- waiting_tx_skb = NULL
				...
				- ongoing_tx_skb = NULL
- if waiting_tx_skb is NULL
- waiting_tx_skb = skb
				- if ongoing_tx_skb is NULL
				- ongoing_tx_skb = waiting_tx_skb
				- waiting_tx_skb = NULL
				...
				- ongoing_tx_skb = NULL
....

Bad case:
---------
Consider waiting_tx_skb is NULL.

Thread1 (oa_tc6_start_xmit)	Thread2 (oa_tc6_spi_thread_handler)
---------------------------	-----------------------------------
- if waiting_tx_skb is NULL
- waiting_tx_skb = skb
				- if ongoing_tx_skb is NULL
				- ongoing_tx_skb = waiting_tx_skb
				- waiting_tx_skb = NULL
				...
				- ongoing_tx_skb = NULL
- 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

So one tx skb is left as unhandled and the pointer is overwritten with 
NULL. Hope this clarifies the race condition.

Best regards,
Parthiban V
> 
> Thanks,
> 
> Paolo
>