tcp: fix SCM_TIMESTAMPING_OPT_STATS for normal skbs

PATCH In review Soheil Hassas Yeganeh 2 participants 4 messages Mar 18, 2017 – Mar 22, 2017

Subsystems (3)

Networking Network drivers Sockets

lore

Share & tools

lore

Soheil Hassas Yeganeh

author

2017-03-18 21:12 UTC #0 lore raw

PATCH 1/2 tcp: fix SCM_TIMESTAMPING_OPT_STATS for normal skbs

From: Soheil Hassas Yeganeh <[email protected]>

__sock_recv_timestamp can be called for both normal skbs (for receive timestamps) and for skbs on the error queue (for transmit timestamps).

Commit 1c885808e456
(tcp: SOF_TIMESTAMPING_OPT_STATS option for SO_TIMESTAMPING) assumes any skb passed to __sock_recv_timestamp are from the error queue, containing OPT_STATS in the content of the skb. This results in accessing invalid memory or generating junk data.

To fix this, set skb->pkt_type to PACKET_OUTGOING for packets on the error queue. This is safe because on the receive path on local sockets skb->pkt_type is never set to PACKET_OUTGOING. With that, copy OPT_STATS from a packet, only if its pkt_type is PACKET_OUTGOING.

Fixes 1c885808e456 ("tcp: SOF_TIMESTAMPING_OPT_STATS option for SO_TIMESTAMPING")

Reported-by JongHwan Kim <[email protected]>

Signed-off-by Soheil Hassas Yeganeh <[email protected]>

Signed-off-by Eric Dumazet <[email protected]>

Signed-off-by Willem de Bruijn <[email protected]>

---
 net/core/skbuff.c | 10 ++++++++++
 net/socket.c      | 13 ++++++++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

net/core/skbuff.c +10 -0

index cd4ba8c6b609..b1fbd1958eb6 100644
@@ -3694,6 +3694,15 @@ static void sock_rmem_free(struct sk_buff *skb)
 	atomic_sub(skb->truesize, &sk->sk_rmem_alloc);
 }
 
+static void skb_set_err_queue(struct sk_buff *skb)
+{
+	/* pkt_type of skbs received on local sockets is never PACKET_OUTGOING.
+	 * So, it is safe to (mis)use it to mark skbs on the error queue.
+	 */
+	skb->pkt_type = PACKET_OUTGOING;
+	BUILD_BUG_ON(PACKET_OUTGOING == 0);
+}
+
 /*
  * Note: We dont mem charge error packets (no sk_forward_alloc changes)
  */

@@ -3707,6 +3716,7 @@ int sock_queue_err_skb(struct sock *sk, struct sk_buff *skb)
 	skb->sk = sk;
 	skb->destructor = sock_rmem_free;
 	atomic_add(skb->truesize, &sk->sk_rmem_alloc);
+	skb_set_err_queue(skb);
 
 	/* before exiting rcu section, make sure dst is refcounted */
 	skb_dst_force(skb);

net/socket.c +12 -1

index e034fe4164be..692d6989d2c2 100644
@@ -652,6 +652,16 @@ int kernel_sendmsg(struct socket *sock, struct msghdr *msg,
 }
 EXPORT_SYMBOL(kernel_sendmsg);
 
+static bool skb_is_err_queue(const struct sk_buff *skb)
+{
+	/* pkt_type of skbs enqueued on the error queue are set to
+	 * PACKET_OUTGOING in skb_set_err_queue(). This is only safe to do
+	 * in recvmsg, since skbs received on a local socket will never
+	 * have a pkt_type of PACKET_OUTGOING.
+	 */
+	return skb->pkt_type == PACKET_OUTGOING;
+}
+
 /*
  * called from sock_recv_timestamp() if sock_flag(sk, SOCK_RCVTSTAMP)
  */

@@ -695,7 +705,8 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
 		put_cmsg(msg, SOL_SOCKET,
 			 SCM_TIMESTAMPING, sizeof(tss), &tss);
 
-		if (skb->len && (sk->sk_tsflags & SOF_TIMESTAMPING_OPT_STATS))
+		if (skb_is_err_queue(skb) && skb->len &&
+		    (sk->sk_tsflags & SOF_TIMESTAMPING_OPT_STATS))
 			put_cmsg(msg, SOL_SOCKET, SCM_TIMESTAMPING_OPT_STATS,
 				 skb->len, skb->data);
 	}

Signature · 1 line

2.12.0.367.g23dc2f6d3c-goog

Soheil Hassas Yeganeh

author

2017-03-18 23:00 UTC #1 lore raw

PATCH 2/2 tcp: mark skbs with SCM_TIMESTAMPING_OPT_STATS

From: Soheil Hassas Yeganeh <[email protected]>

SOF_TIMESTAMPING_OPT_STATS can be enabled and disabled while packets are collected on the error queue. So, checking SOF_TIMESTAMPING_OPT_STATS in sk->sk_tsflags is not enough to safely assume that the skb contains OPT_STATS data.

Add a bit in sock_exterr_skb to indicate whether the skb contains opt_stats data.

Fixes 1c885808e456 ("tcp: SOF_TIMESTAMPING_OPT_STATS option for SO_TIMESTAMPING")

Reported-by JongHwan Kim <[email protected]>

Signed-off-by Soheil Hassas Yeganeh <[email protected]>

Signed-off-by Eric Dumazet <[email protected]>

Signed-off-by Willem de Bruijn <[email protected]>

---
 include/linux/errqueue.h |  2 ++
 net/core/skbuff.c        | 17 +++++++++++------
 net/socket.c             |  2 +-
 3 files changed, 14 insertions(+), 7 deletions(-)

include/linux/errqueue.h +2 -0

index 9ca23fcfb5d7..6fdfc884fdeb 100644
@@ -20,6 +20,8 @@ struct sock_exterr_skb {
 	struct sock_extended_err	ee;
 	u16				addr_offset;
 	__be16				port;
+	u8				opt_stats:1,
+					unused:7;
 };
 
 #endif

net/core/skbuff.c +11 -6

index b1fbd1958eb6..9f781092fda9 100644
@@ -3793,16 +3793,20 @@ EXPORT_SYMBOL(skb_clone_sk);
 
 static void __skb_complete_tx_timestamp(struct sk_buff *skb,
 					struct sock *sk,
-					int tstype)
+					int tstype,
+					bool opt_stats)
 {
 	struct sock_exterr_skb *serr;
 	int err;
 
+	BUILD_BUG_ON(sizeof(struct sock_exterr_skb) > sizeof(skb->cb));
+
 	serr = SKB_EXT_ERR(skb);
 	memset(serr, 0, sizeof(*serr));
 	serr->ee.ee_errno = ENOMSG;
 	serr->ee.ee_origin = SO_EE_ORIGIN_TIMESTAMPING;
 	serr->ee.ee_info = tstype;
+	serr->opt_stats = opt_stats;
 	if (sk->sk_tsflags & SOF_TIMESTAMPING_OPT_ID) {
 		serr->ee.ee_data = skb_shinfo(skb)->tskey;
 		if (sk->sk_protocol == IPPROTO_TCP &&

@@ -3843,7 +3847,7 @@ void skb_complete_tx_timestamp(struct sk_buff *skb,
 	 */
 	if (likely(atomic_inc_not_zero(&sk->sk_refcnt))) {
 		*skb_hwtstamps(skb) = *hwtstamps;
-		__skb_complete_tx_timestamp(skb, sk, SCM_TSTAMP_SND);
+		__skb_complete_tx_timestamp(skb, sk, SCM_TSTAMP_SND, false);
 		sock_put(sk);
 	}
 }

@@ -3854,7 +3858,7 @@ void __skb_tstamp_tx(struct sk_buff *orig_skb,
 		     struct sock *sk, int tstype)
 {
 	struct sk_buff *skb;
-	bool tsonly;
+	bool tsonly, opt_stats = false;
 
 	if (!sk)
 		return;

@@ -3867,9 +3871,10 @@ void __skb_tstamp_tx(struct sk_buff *orig_skb,
 #ifdef CONFIG_INET
 		if ((sk->sk_tsflags & SOF_TIMESTAMPING_OPT_STATS) &&
 		    sk->sk_protocol == IPPROTO_TCP &&
-		    sk->sk_type == SOCK_STREAM)
+		    sk->sk_type == SOCK_STREAM) {
 			skb = tcp_get_timestamping_opt_stats(sk);
-		else
+			opt_stats = true;
+		} else
 #endif
 			skb = alloc_skb(0, GFP_ATOMIC);
 	} else {

@@ -3888,7 +3893,7 @@ void __skb_tstamp_tx(struct sk_buff *orig_skb,
 	else
 		skb->tstamp = ktime_get_real();
 
-	__skb_complete_tx_timestamp(skb, sk, tstype);
+	__skb_complete_tx_timestamp(skb, sk, tstype, opt_stats);
 }
 EXPORT_SYMBOL_GPL(__skb_tstamp_tx);

net/socket.c +1 -1

index 692d6989d2c2..985ef06792d6 100644
@@ -706,7 +706,7 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
 			 SCM_TIMESTAMPING, sizeof(tss), &tss);
 
 		if (skb_is_err_queue(skb) && skb->len &&
-		    (sk->sk_tsflags & SOF_TIMESTAMPING_OPT_STATS))
+		    SKB_EXT_ERR(skb)->opt_stats)
 			put_cmsg(msg, SOL_SOCKET, SCM_TIMESTAMPING_OPT_STATS,
 				 skb->len, skb->data);
 	}

Signature · 1 line

2.12.0.367.g23dc2f6d3c-goog

David Miller

maintainer

2017-03-22 01:45 UTC #2 lore raw

Re: [PATCH net 2/2] tcp: mark skbs with SCM_TIMESTAMPING_OPT_STATS

From: Soheil Hassas Yeganeh <[email protected]>
Date: Sat, 18 Mar 2017 17:03:00 -0400

Quoted 8 lines

From: Soheil Hassas Yeganeh <[email protected]>
SOF_TIMESTAMPING_OPT_STATS can be enabled and disabled while packets are collected on the error queue. So, checking SOF_TIMESTAMPING_OPT_STATS in sk->sk_tsflags is not enough to safely assume that the skb contains OPT_STATS data.

Quoted 8 lines

Add a bit in sock_exterr_skb to indicate whether the skb contains opt_stats data.
Fixes 1c885808e456 ("tcp: SOF_TIMESTAMPING_OPT_STATS option for SO_TIMESTAMPING")
Reported-by JongHwan Kim <[email protected]>
Signed-off-by Soheil Hassas Yeganeh <[email protected]>
Signed-off-by Eric Dumazet <[email protected]>
Signed-off-by Willem de Bruijn <[email protected]>

Also applied and queued up for -stable.

David Miller

maintainer

2017-03-22 01:44 UTC #3 lore raw

In reply to Soheil Hassas Yeganeh

From: Soheil Hassas Yeganeh <[email protected]>
Date: Sat, 18 Mar 2017 17:02:59 -0400

Quoted 19 lines

From: Soheil Hassas Yeganeh <[email protected]>
__sock_recv_timestamp can be called for both normal skbs (for receive timestamps) and for skbs on the error queue (for transmit timestamps).
Commit 1c885808e456
(tcp: SOF_TIMESTAMPING_OPT_STATS option for SO_TIMESTAMPING) assumes any skb passed to __sock_recv_timestamp are from the error queue, containing OPT_STATS in the content of the skb. This results in accessing invalid memory or generating junk data.
To fix this, set skb->pkt_type to PACKET_OUTGOING for packets on the error queue. This is safe because on the receive path on local sockets skb->pkt_type is never set to PACKET_OUTGOING. With that, copy OPT_STATS from a packet, only if its pkt_type is PACKET_OUTGOING.

Quoted 5 lines

Fixes 1c885808e456 ("tcp: SOF_TIMESTAMPING_OPT_STATS option for SO_TIMESTAMPING")
Reported-by JongHwan Kim <[email protected]>
Signed-off-by Soheil Hassas Yeganeh <[email protected]>
Signed-off-by Eric Dumazet <[email protected]>
Signed-off-by Willem de Bruijn <[email protected]>

Applied and queued up for -stable.