[lvc-project] [PATCH 5.10 004/317] r8169: Fix possible ring buffer corruption on fragmented Tx packets.

[Ken Milmore]

On 13/06/2024 21:27, Ken Milmore wrote:
> On 13/06/2024 18:21, Alexey Khoroshilov wrote:
>> On 13.06.2024 14:30, Greg Kroah-Hartman wrote:
>>> 5.10-stable review patch.  If anyone has any objections, please let me know.
>>
>> The patch is cleanly applied to 5.10, but it leads to uninit value
>> access in rtl_tx_slots_avail().
>>
>>
>> 	unsigned int frags;
>> 	u32 opts[2];
>>
>> 	txd_first = tp->TxDescArray + entry;
>>
>> 	if (unlikely(!rtl_tx_slots_avail(tp, frags))) {
>>                                              ^^^^^ - USE OF UNINIT VALUE
>> 		if (net_ratelimit())
>> 			netdev_err(dev, "BUG! Tx Ring full when queue awake!\n");
>> 		goto err_stop_0;
>> 	}
>>
>> 	opts[1] = rtl8169_tx_vlan_tag(skb);
>> 	opts[0] = 0;
>>
>> 	if (!rtl_chip_supports_csum_v2(tp))
>> 		rtl8169_tso_csum_v1(skb, opts);
>> 	else if (!rtl8169_tso_csum_v2(tp, skb, opts))
>> 		goto err_dma_0;
>>
>> 	if (unlikely(rtl8169_tx_map(tp, opts, skb_headlen(skb), skb->data,
>> 				    entry, false)))
>> 		goto err_dma_0;
>>
>> 	txd_first = tp->TxDescArray + entry;
>>
>> 	frags = skb_shinfo(skb)->nr_frags;
>>         ^^^^^^   - INITIALIZATION IS HERE AFTER THE PATCH
>>
>> There is no such problem in upstream because rtl_tx_slots_avail() has no
>> nr_frags argument there.
>>
>>
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>
>> --
>> Alexey Khoroshilov
>> Linux Verification Center, ISPRAS
> 
> Looks like the frags argument was removed in commit 83c317d7b36bb (r8169: remove nr_frags argument from rtl_tx_slots_avail), which first appears in linux-5.11.
> 
> I dare say it would be safe to replace
>  	if (unlikely(!rtl_tx_slots_avail(tp, frags))) {
> with
>  	if (unlikely(!rtl_tx_slots_avail(tp, MAX_SKB_FRAGS))) {
> 
> Best wait for Heiner to confirm though.

Further to this, I have tested the following amended patch to 5.10 on the RTL8125b and it works without problems:


diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index c29d43c5f450..8aa0097bf887 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -4279,16 +4279,16 @@ static void rtl8169_doorbell(struct rtl8169_private *tp)
 static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
 				      struct net_device *dev)
 {
-	unsigned int frags = skb_shinfo(skb)->nr_frags;
 	struct rtl8169_private *tp = netdev_priv(dev);
 	unsigned int entry = tp->cur_tx % NUM_TX_DESC;
 	struct TxDesc *txd_first, *txd_last;
 	bool stop_queue, door_bell;
+	unsigned int frags;
 	u32 opts[2];
 
 	txd_first = tp->TxDescArray + entry;
 
-	if (unlikely(!rtl_tx_slots_avail(tp, frags))) {
+	if (unlikely(!rtl_tx_slots_avail(tp, MAX_SKB_FRAGS))) {
 		if (net_ratelimit())
 			netdev_err(dev, "BUG! Tx Ring full when queue awake!\n");
 		goto err_stop_0;
@@ -4309,6 +4309,7 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
 				    entry, false)))
 		goto err_dma_0;
 
+	frags = skb_shinfo(skb)->nr_frags;
 	if (frags) {
 		if (rtl8169_xmit_frags(tp, skb, opts, entry))
 			goto err_dma_1;


An alternative to this amendment would be to apply 83c317d7b36bb as a prerequisite, but that does not apply cleanly to 5.10 without some massaging.

Because of these complications, I would suggest it may be safest to omit this patch from 5.10 for now.

NAKed-by: Ken Milmore <ken.milmore at gmail.com>