netdev - [PATCH net] net: dsa: sja1105: Fix double delivery of TX timestamps to socket error queue

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: <20191216223344.2261-1-olteanv@gmail.com>
Date:   Tue, 17 Dec 2019 00:33:44 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     davem@...emloft.net, jakub.kicinski@...ronome.com
Cc:     richardcochran@...il.com, f.fainelli@...il.com,
        vivien.didelot@...il.com, andrew@...n.ch, netdev@...r.kernel.org,
        Vladimir Oltean <olteanv@...il.com>
Subject: [PATCH net] net: dsa: sja1105: Fix double delivery of TX timestamps to socket error queue

On the LS1021A-TSN board, it can be seen that under rare conditions,
ptp4l gets unexpected extra data in the event socket error queue.

This is because the DSA master driver (gianfar) has TX timestamping
logic along the lines of:

1. In gfar_start_xmit:
	do_tstamp = (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
		    priv->hwts_tx_en;
	(...)
	if (unlikely(do_tstamp))
		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
2. Later in gfar_clean_tx_ring:
	if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
		(...)
		skb_tstamp_tx(skb, &shhwtstamps);

That is to say, between the first and second check, it drops
priv->hwts_tx_en (it assumes that it is the only one who can set
SKBTX_IN_PROGRESS, disregarding stacked net devices such as DSA switches
or PTP-capable PHY drivers). Any such driver (like sja1105 in this case)
that would set SKBTX_IN_PROGRESS would trip up this check and gianfar
would deliver a garbage TX timestamp for this skb too, which can in turn
have unpredictable and surprising effects to user space.

In fact gianfar is by far not the only driver which uses
SKBTX_IN_PROGRESS to identify skb's that need special handling. The flag
used to have a historical purpose and is now evaluated in the networking
stack in a single place: in __skb_tstamp_tx, only on the software
timestamping path (hwtstamps == NULL) which is not relevant for us.

So do the wise thing and drop the unneeded assignment. Even though this
patch alone will not protect against all classes of Ethernet driver TX
timestamping bugs, it will circumvent those related to the incorrect
interpretation of this skb tx flag.

Fixes: 47ed985e97f5 ("net: dsa: sja1105: Add logic for TX timestamping")
Suggested-by: Richard Cochran <richardcochran@...il.com>
Signed-off-by: Vladimir Oltean <olteanv@...il.com>
---
 drivers/net/dsa/sja1105/sja1105_ptp.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.c b/drivers/net/dsa/sja1105/sja1105_ptp.c
index 54258a25031d..038c83fbd9e8 100644
--- a/drivers/net/dsa/sja1105/sja1105_ptp.c
+++ b/drivers/net/dsa/sja1105/sja1105_ptp.c
@@ -668,8 +668,6 @@ void sja1105_ptp_txtstamp_skb(struct dsa_switch *ds, int slot,
 	u64 ticks, ts;
 	int rc;

-	skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
-
 	mutex_lock(&ptp_data->lock);

 	rc = sja1105_ptpclkval_read(priv, &ticks, NULL);
-- 
2.17.1