[BUG] kernel stack corruption during/after Netlabel error

[BUG] kernel stack corruption during/after Netlabel error

[Casey Schaufler]

On 11/29/2017 2:26 AM, James Morris wrote:
> I'm seeing a kernel stack corruption bug (detected via gcc) when running 
> the SELinux testsuite on a 4.15-rc1 kernel, in the 2nd inet_socket test:
>
> https://github.com/SELinuxProject/selinux-testsuite/blob/master/tests/inet_socket/test
>
>   # Verify that unauthorized client cannot communicate with the server.
>   $result = system
>   "runcon -t test_inet_bad_client_t -- $basedir/client stream 127.0.0.1 65535 2>&1";
>
> This correctlly causes an access control error in the Netlabel code, and 
> the bug seems to be triggered during the ICMP send:
>
> ...<SNIP>...
>
> This is mostly reliable, and I'm only seeing it on bare metal (not in a 
> virtualbox vm).
>
> The SELinux skb parse error at the start only sometimes appears, and 
> looking at the code, I suspect some kind of memory corruption being the 
> cause at that point (basic packet header checks).
>
> I bisected the bug down to the following change:
>
> commit bffa72cf7f9df842f0016ba03586039296b4caaf
> Author: Eric Dumazet <edumazet@google.com>
> Date:   Tue Sep 19 05:14:24 2017 -0700
>
>     net: sk_buff rbnode reorg
>     ...
>
>
> Anyone else able to reproduce this, or have any ideas on what's happening?

I have also bisected a problem to this change. I do not have a trace
because the problem manifests as a hard system hang without a trace
being presented. The issue arises when Smack attempts to relabel a TCP
socket using netlbl_sock_setattr().

I see that there is a proposed fix later in the thread, but I don't see
the patch. Could you send it to me, so I can try it on my problem?

Thank you.

>
>
>
> - James

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[BUG] kernel stack corruption during/after Netlabel error

[James Morris]

On Wed, 29 Nov 2017, Casey Schaufler wrote:

> I see that there is a proposed fix later in the thread, but I don't see
> the patch. Could you send it to me, so I can try it on my problem?

Forwarded off-list.

Interestingly, I didn't see the KASAN output email from Stephen here.


-- 
James Morris
<james.l.morris@oracle.com>

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[BUG] kernel stack corruption during/after Netlabel error

[Casey Schaufler]

On 11/29/2017 4:31 PM, James Morris wrote:
> On Wed, 29 Nov 2017, Casey Schaufler wrote:
>
>> I see that there is a proposed fix later in the thread, but I don't see
>> the patch. Could you send it to me, so I can try it on my problem?
> Forwarded off-list.

The patch does fix the problem I was seeing in Smack.

>
> Interestingly, I didn't see the KASAN output email from Stephen here.
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[BUG] kernel stack corruption during/after Netlabel error

[Eric Dumazet]

On Wed, 2017-11-29 at 19:16 -0800, Casey Schaufler wrote:
> On 11/29/2017 4:31 PM, James Morris wrote:
> > On Wed, 29 Nov 2017, Casey Schaufler wrote:
> > 
> > > I see that there is a proposed fix later in the thread, but I
> > > don't see
> > > the patch. Could you send it to me, so I can try it on my
> > > problem?
> > 
> > Forwarded off-list.
> 
> The patch does fix the problem I was seeing in Smack.

Can you guys test the following more complete patch ?

It should cover IPv4 and IPv6, and also the corner cases.

( Note that I squashed ipv6 fix in https://patchwork.ozlabs.org/patch/8
42844/ that I spotted while cooking this patch )

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index c6bc0c4d19c624888b0d0b5a4246c7183edf63f5..77ea45da0fe9c746907a312989658af3ad3b198d 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1591,6 +1591,34 @@ int tcp_filter(struct sock *sk, struct sk_buff *skb)
 }
 EXPORT_SYMBOL(tcp_filter);
 
+static void tcp_v4_restore_cb(struct sk_buff *skb)
+{
+	memmove(IPCB(skb), &TCP_SKB_CB(skb)->header.h4,
+		sizeof(struct inet_skb_parm));
+}
+
+static void tcp_v4_fill_cb(struct sk_buff *skb, const struct iphdr *iph,
+			   const struct tcphdr *th)
+{
+	/* This is tricky : We move IPCB at its correct location into TCP_SKB_CB()
+	 * barrier() makes sure compiler wont play fool^Waliasing games.
+	 */
+	memmove(&TCP_SKB_CB(skb)->header.h4, IPCB(skb),
+		sizeof(struct inet_skb_parm));
+	barrier();
+
+	TCP_SKB_CB(skb)->seq = ntohl(th->seq);
+	TCP_SKB_CB(skb)->end_seq = (TCP_SKB_CB(skb)->seq + th->syn + th->fin +
+				    skb->len - th->doff * 4);
+	TCP_SKB_CB(skb)->ack_seq = ntohl(th->ack_seq);
+	TCP_SKB_CB(skb)->tcp_flags = tcp_flag_byte(th);
+	TCP_SKB_CB(skb)->tcp_tw_isn = 0;
+	TCP_SKB_CB(skb)->ip_dsfield = ipv4_get_dsfield(iph);
+	TCP_SKB_CB(skb)->sacked	 = 0;
+	TCP_SKB_CB(skb)->has_rxtstamp =
+			skb->tstamp || skb_hwtstamps(skb)->hwtstamp;
+}
+
 /*
  *	From tcp_input.c
  */
@@ -1631,24 +1659,6 @@ int tcp_v4_rcv(struct sk_buff *skb)
 
 	th = (const struct tcphdr *)skb->data;
 	iph = ip_hdr(skb);
-	/* This is tricky : We move IPCB at its correct location into TCP_SKB_CB()
-	 * barrier() makes sure compiler wont play fool^Waliasing games.
-	 */
-	memmove(&TCP_SKB_CB(skb)->header.h4, IPCB(skb),
-		sizeof(struct inet_skb_parm));
-	barrier();
-
-	TCP_SKB_CB(skb)->seq = ntohl(th->seq);
-	TCP_SKB_CB(skb)->end_seq = (TCP_SKB_CB(skb)->seq + th->syn + th->fin +
-				    skb->len - th->doff * 4);
-	TCP_SKB_CB(skb)->ack_seq = ntohl(th->ack_seq);
-	TCP_SKB_CB(skb)->tcp_flags = tcp_flag_byte(th);
-	TCP_SKB_CB(skb)->tcp_tw_isn = 0;
-	TCP_SKB_CB(skb)->ip_dsfield = ipv4_get_dsfield(iph);
-	TCP_SKB_CB(skb)->sacked	 = 0;
-	TCP_SKB_CB(skb)->has_rxtstamp =
-			skb->tstamp || skb_hwtstamps(skb)->hwtstamp;
-
 lookup:
 	sk = __inet_lookup_skb(&tcp_hashinfo, skb, __tcp_hdrlen(th), th->source,
 			       th->dest, sdif, &refcounted);
@@ -1679,14 +1689,19 @@ int tcp_v4_rcv(struct sk_buff *skb)
 		sock_hold(sk);
 		refcounted = true;
 		nsk = NULL;
-		if (!tcp_filter(sk, skb))
+		if (!tcp_filter(sk, skb)) {
+			th = (const struct tcphdr *)skb->data;
+			iph = ip_hdr(skb);
+			tcp_v4_fill_cb(skb, iph, th);
 			nsk = tcp_check_req(sk, skb, req, false);
+		}
 		if (!nsk) {
 			reqsk_put(req);
 			goto discard_and_relse;
 		}
 		if (nsk == sk) {
 			reqsk_put(req);
+			tcp_v4_restore_cb(skb);
 		} else if (tcp_child_process(sk, nsk, skb)) {
 			tcp_v4_send_reset(nsk, skb);
 			goto discard_and_relse;
@@ -1712,6 +1727,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
 		goto discard_and_relse;
 	th = (const struct tcphdr *)skb->data;
 	iph = ip_hdr(skb);
+	tcp_v4_fill_cb(skb, iph, th);
 
 	skb->dev = NULL;
 
@@ -1742,6 +1758,8 @@ int tcp_v4_rcv(struct sk_buff *skb)
 	if (!xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb))
 		goto discard_it;
 
+	tcp_v4_fill_cb(skb, iph, th);
+
 	if (tcp_checksum_complete(skb)) {
 csum_error:
 		__TCP_INC_STATS(net, TCP_MIB_CSUMERRORS);
@@ -1768,6 +1786,8 @@ int tcp_v4_rcv(struct sk_buff *skb)
 		goto discard_it;
 	}
 
+	tcp_v4_fill_cb(skb, iph, th);
+
 	if (tcp_checksum_complete(skb)) {
 		inet_twsk_put(inet_twsk(sk));
 		goto csum_error;
@@ -1784,6 +1804,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
 		if (sk2) {
 			inet_twsk_deschedule_put(inet_twsk(sk));
 			sk = sk2;
+			tcp_v4_restore_cb(skb);
 			refcounted = false;
 			goto process;
 		}
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 6bb98c93edfe2ed2f16fe5229605f8108cfc7f9a..1f04ec0e4a7aa2c11b8ee27cbdd4067b5bcf32e5 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1454,7 +1454,6 @@ static int tcp_v6_rcv(struct sk_buff *skb)
 		struct sock *nsk;
 
 		sk = req->rsk_listener;
-		tcp_v6_fill_cb(skb, hdr, th);
 		if (tcp_v6_inbound_md5_hash(sk, skb)) {
 			sk_drops_add(sk, skb);
 			reqsk_put(req);
@@ -1467,8 +1466,12 @@ static int tcp_v6_rcv(struct sk_buff *skb)
 		sock_hold(sk);
 		refcounted = true;
 		nsk = NULL;
-		if (!tcp_filter(sk, skb))
+		if (!tcp_filter(sk, skb)) {
+			th = (const struct tcphdr *)skb->data;
+			hdr = ipv6_hdr(skb);
+			tcp_v6_fill_cb(skb, hdr, th);
 			nsk = tcp_check_req(sk, skb, req, false);
+		}
 		if (!nsk) {
 			reqsk_put(req);
 			goto discard_and_relse;
@@ -1492,8 +1495,6 @@ static int tcp_v6_rcv(struct sk_buff *skb)
 	if (!xfrm6_policy_check(sk, XFRM_POLICY_IN, skb))
 		goto discard_and_relse;
 
-	tcp_v6_fill_cb(skb, hdr, th);
-
 	if (tcp_v6_inbound_md5_hash(sk, skb))
 		goto discard_and_relse;
 
@@ -1501,6 +1502,7 @@ static int tcp_v6_rcv(struct sk_buff *skb)
 		goto discard_and_relse;
 	th = (const struct tcphdr *)skb->data;
 	hdr = ipv6_hdr(skb);
+	tcp_v6_fill_cb(skb, hdr, th);
 
 	skb->dev = NULL;
 
@@ -1590,7 +1592,6 @@ static int tcp_v6_rcv(struct sk_buff *skb)
 		tcp_v6_timewait_ack(sk, skb);
 		break;
 	case TCP_TW_RST:
-		tcp_v6_restore_cb(skb);
 		tcp_v6_send_reset(sk, skb);
 		inet_twsk_deschedule_put(inet_twsk(sk));
 		goto discard_it;



--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[BUG] kernel stack corruption during/after Netlabel error

[Paul Moore]

On Thu, Nov 30, 2017 at 5:50 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2017-11-29 at 19:16 -0800, Casey Schaufler wrote:
>> On 11/29/2017 4:31 PM, James Morris wrote:
>> > On Wed, 29 Nov 2017, Casey Schaufler wrote:
>> >
>> > > I see that there is a proposed fix later in the thread, but I
>> > > don't see
>> > > the patch. Could you send it to me, so I can try it on my
>> > > problem?
>> >
>> > Forwarded off-list.
>>
>> The patch does fix the problem I was seeing in Smack.
>
> Can you guys test the following more complete patch ?
>
> It should cover IPv4 and IPv6, and also the corner cases.
>
> ( Note that I squashed ipv6 fix in https://patchwork.ozlabs.org/patch/8
> 42844/ that I spotted while cooking this patch )

Building a test kernel now, although it make take me a few hours to
test it due to some commitments this morning.

> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index c6bc0c4d19c624888b0d0b5a4246c7183edf63f5..77ea45da0fe9c746907a312989658af3ad3b198d 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -1591,6 +1591,34 @@ int tcp_filter(struct sock *sk, struct sk_buff *skb)
>  }
>  EXPORT_SYMBOL(tcp_filter);
>
> +static void tcp_v4_restore_cb(struct sk_buff *skb)
> +{
> +       memmove(IPCB(skb), &TCP_SKB_CB(skb)->header.h4,
> +               sizeof(struct inet_skb_parm));
> +}
> +
> +static void tcp_v4_fill_cb(struct sk_buff *skb, const struct iphdr *iph,
> +                          const struct tcphdr *th)
> +{
> +       /* This is tricky : We move IPCB at its correct location into TCP_SKB_CB()
> +        * barrier() makes sure compiler wont play fool^Waliasing games.
> +        */
> +       memmove(&TCP_SKB_CB(skb)->header.h4, IPCB(skb),
> +               sizeof(struct inet_skb_parm));
> +       barrier();
> +
> +       TCP_SKB_CB(skb)->seq = ntohl(th->seq);
> +       TCP_SKB_CB(skb)->end_seq = (TCP_SKB_CB(skb)->seq + th->syn + th->fin +
> +                                   skb->len - th->doff * 4);
> +       TCP_SKB_CB(skb)->ack_seq = ntohl(th->ack_seq);
> +       TCP_SKB_CB(skb)->tcp_flags = tcp_flag_byte(th);
> +       TCP_SKB_CB(skb)->tcp_tw_isn = 0;
> +       TCP_SKB_CB(skb)->ip_dsfield = ipv4_get_dsfield(iph);
> +       TCP_SKB_CB(skb)->sacked  = 0;
> +       TCP_SKB_CB(skb)->has_rxtstamp =
> +                       skb->tstamp || skb_hwtstamps(skb)->hwtstamp;
> +}
> +
>  /*
>   *     From tcp_input.c
>   */
> @@ -1631,24 +1659,6 @@ int tcp_v4_rcv(struct sk_buff *skb)
>
>         th = (const struct tcphdr *)skb->data;
>         iph = ip_hdr(skb);
> -       /* This is tricky : We move IPCB at its correct location into TCP_SKB_CB()
> -        * barrier() makes sure compiler wont play fool^Waliasing games.
> -        */
> -       memmove(&TCP_SKB_CB(skb)->header.h4, IPCB(skb),
> -               sizeof(struct inet_skb_parm));
> -       barrier();
> -
> -       TCP_SKB_CB(skb)->seq = ntohl(th->seq);
> -       TCP_SKB_CB(skb)->end_seq = (TCP_SKB_CB(skb)->seq + th->syn + th->fin +
> -                                   skb->len - th->doff * 4);
> -       TCP_SKB_CB(skb)->ack_seq = ntohl(th->ack_seq);
> -       TCP_SKB_CB(skb)->tcp_flags = tcp_flag_byte(th);
> -       TCP_SKB_CB(skb)->tcp_tw_isn = 0;
> -       TCP_SKB_CB(skb)->ip_dsfield = ipv4_get_dsfield(iph);
> -       TCP_SKB_CB(skb)->sacked  = 0;
> -       TCP_SKB_CB(skb)->has_rxtstamp =
> -                       skb->tstamp || skb_hwtstamps(skb)->hwtstamp;
> -
>  lookup:
>         sk = __inet_lookup_skb(&tcp_hashinfo, skb, __tcp_hdrlen(th), th->source,
>                                th->dest, sdif, &refcounted);
> @@ -1679,14 +1689,19 @@ int tcp_v4_rcv(struct sk_buff *skb)
>                 sock_hold(sk);
>                 refcounted = true;
>                 nsk = NULL;
> -               if (!tcp_filter(sk, skb))
> +               if (!tcp_filter(sk, skb)) {
> +                       th = (const struct tcphdr *)skb->data;
> +                       iph = ip_hdr(skb);
> +                       tcp_v4_fill_cb(skb, iph, th);
>                         nsk = tcp_check_req(sk, skb, req, false);
> +               }
>                 if (!nsk) {
>                         reqsk_put(req);
>                         goto discard_and_relse;
>                 }
>                 if (nsk == sk) {
>                         reqsk_put(req);
> +                       tcp_v4_restore_cb(skb);
>                 } else if (tcp_child_process(sk, nsk, skb)) {
>                         tcp_v4_send_reset(nsk, skb);
>                         goto discard_and_relse;
> @@ -1712,6 +1727,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
>                 goto discard_and_relse;
>         th = (const struct tcphdr *)skb->data;
>         iph = ip_hdr(skb);
> +       tcp_v4_fill_cb(skb, iph, th);
>
>         skb->dev = NULL;
>
> @@ -1742,6 +1758,8 @@ int tcp_v4_rcv(struct sk_buff *skb)
>         if (!xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb))
>                 goto discard_it;
>
> +       tcp_v4_fill_cb(skb, iph, th);
> +
>         if (tcp_checksum_complete(skb)) {
>  csum_error:
>                 __TCP_INC_STATS(net, TCP_MIB_CSUMERRORS);
> @@ -1768,6 +1786,8 @@ int tcp_v4_rcv(struct sk_buff *skb)
>                 goto discard_it;
>         }
>
> +       tcp_v4_fill_cb(skb, iph, th);
> +
>         if (tcp_checksum_complete(skb)) {
>                 inet_twsk_put(inet_twsk(sk));
>                 goto csum_error;
> @@ -1784,6 +1804,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
>                 if (sk2) {
>                         inet_twsk_deschedule_put(inet_twsk(sk));
>                         sk = sk2;
> +                       tcp_v4_restore_cb(skb);
>                         refcounted = false;
>                         goto process;
>                 }
> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index 6bb98c93edfe2ed2f16fe5229605f8108cfc7f9a..1f04ec0e4a7aa2c11b8ee27cbdd4067b5bcf32e5 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -1454,7 +1454,6 @@ static int tcp_v6_rcv(struct sk_buff *skb)
>                 struct sock *nsk;
>
>                 sk = req->rsk_listener;
> -               tcp_v6_fill_cb(skb, hdr, th);
>                 if (tcp_v6_inbound_md5_hash(sk, skb)) {
>                         sk_drops_add(sk, skb);
>                         reqsk_put(req);
> @@ -1467,8 +1466,12 @@ static int tcp_v6_rcv(struct sk_buff *skb)
>                 sock_hold(sk);
>                 refcounted = true;
>                 nsk = NULL;
> -               if (!tcp_filter(sk, skb))
> +               if (!tcp_filter(sk, skb)) {
> +                       th = (const struct tcphdr *)skb->data;
> +                       hdr = ipv6_hdr(skb);
> +                       tcp_v6_fill_cb(skb, hdr, th);
>                         nsk = tcp_check_req(sk, skb, req, false);
> +               }
>                 if (!nsk) {
>                         reqsk_put(req);
>                         goto discard_and_relse;
> @@ -1492,8 +1495,6 @@ static int tcp_v6_rcv(struct sk_buff *skb)
>         if (!xfrm6_policy_check(sk, XFRM_POLICY_IN, skb))
>                 goto discard_and_relse;
>
> -       tcp_v6_fill_cb(skb, hdr, th);
> -
>         if (tcp_v6_inbound_md5_hash(sk, skb))
>                 goto discard_and_relse;
>
> @@ -1501,6 +1502,7 @@ static int tcp_v6_rcv(struct sk_buff *skb)
>                 goto discard_and_relse;
>         th = (const struct tcphdr *)skb->data;
>         hdr = ipv6_hdr(skb);
> +       tcp_v6_fill_cb(skb, hdr, th);
>
>         skb->dev = NULL;
>
> @@ -1590,7 +1592,6 @@ static int tcp_v6_rcv(struct sk_buff *skb)
>                 tcp_v6_timewait_ack(sk, skb);
>                 break;
>         case TCP_TW_RST:
> -               tcp_v6_restore_cb(skb);
>                 tcp_v6_send_reset(sk, skb);
>                 inet_twsk_deschedule_put(inet_twsk(sk));
>                 goto discard_it;
>
>
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[BUG] kernel stack corruption during/after Netlabel error

[Casey Schaufler]

On 11/30/2017 2:50 AM, Eric Dumazet wrote:
> On Wed, 2017-11-29 at 19:16 -0800, Casey Schaufler wrote:
>> On 11/29/2017 4:31 PM, James Morris wrote:
>>> On Wed, 29 Nov 2017, Casey Schaufler wrote:
>>>
>>>> I see that there is a proposed fix later in the thread, but I
>>>> don't see
>>>> the patch. Could you send it to me, so I can try it on my
>>>> problem?
>>> Forwarded off-list.
>> The patch does fix the problem I was seeing in Smack.
> Can you guys test the following more complete patch ?

Building now. I should have results soon.

>
> It should cover IPv4 and IPv6, and also the corner cases.
>
> ( Note that I squashed ipv6 fix in https://patchwork.ozlabs.org/patch/8
> 42844/ that I spotted while cooking this patch )
>
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index c6bc0c4d19c624888b0d0b5a4246c7183edf63f5..77ea45da0fe9c746907a312989658af3ad3b198d 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -1591,6 +1591,34 @@ int tcp_filter(struct sock *sk, struct sk_buff *skb)
>  }
>  EXPORT_SYMBOL(tcp_filter);
>  
> +static void tcp_v4_restore_cb(struct sk_buff *skb)
> +{
> +	memmove(IPCB(skb), &TCP_SKB_CB(skb)->header.h4,
> +		sizeof(struct inet_skb_parm));
> +}
> +
> +static void tcp_v4_fill_cb(struct sk_buff *skb, const struct iphdr *iph,
> +			   const struct tcphdr *th)
> +{
> +	/* This is tricky : We move IPCB at its correct location into TCP_SKB_CB()
> +	 * barrier() makes sure compiler wont play fool^Waliasing games.
> +	 */
> +	memmove(&TCP_SKB_CB(skb)->header.h4, IPCB(skb),
> +		sizeof(struct inet_skb_parm));
> +	barrier();
> +
> +	TCP_SKB_CB(skb)->seq = ntohl(th->seq);
> +	TCP_SKB_CB(skb)->end_seq = (TCP_SKB_CB(skb)->seq + th->syn + th->fin +
> +				    skb->len - th->doff * 4);
> +	TCP_SKB_CB(skb)->ack_seq = ntohl(th->ack_seq);
> +	TCP_SKB_CB(skb)->tcp_flags = tcp_flag_byte(th);
> +	TCP_SKB_CB(skb)->tcp_tw_isn = 0;
> +	TCP_SKB_CB(skb)->ip_dsfield = ipv4_get_dsfield(iph);
> +	TCP_SKB_CB(skb)->sacked	 = 0;
> +	TCP_SKB_CB(skb)->has_rxtstamp =
> +			skb->tstamp || skb_hwtstamps(skb)->hwtstamp;
> +}
> +
>  /*
>   *	From tcp_input.c
>   */
> @@ -1631,24 +1659,6 @@ int tcp_v4_rcv(struct sk_buff *skb)
>  
>  	th = (const struct tcphdr *)skb->data;
>  	iph = ip_hdr(skb);
> -	/* This is tricky : We move IPCB at its correct location into TCP_SKB_CB()
> -	 * barrier() makes sure compiler wont play fool^Waliasing games.
> -	 */
> -	memmove(&TCP_SKB_CB(skb)->header.h4, IPCB(skb),
> -		sizeof(struct inet_skb_parm));
> -	barrier();
> -
> -	TCP_SKB_CB(skb)->seq = ntohl(th->seq);
> -	TCP_SKB_CB(skb)->end_seq = (TCP_SKB_CB(skb)->seq + th->syn + th->fin +
> -				    skb->len - th->doff * 4);
> -	TCP_SKB_CB(skb)->ack_seq = ntohl(th->ack_seq);
> -	TCP_SKB_CB(skb)->tcp_flags = tcp_flag_byte(th);
> -	TCP_SKB_CB(skb)->tcp_tw_isn = 0;
> -	TCP_SKB_CB(skb)->ip_dsfield = ipv4_get_dsfield(iph);
> -	TCP_SKB_CB(skb)->sacked	 = 0;
> -	TCP_SKB_CB(skb)->has_rxtstamp =
> -			skb->tstamp || skb_hwtstamps(skb)->hwtstamp;
> -
>  lookup:
>  	sk = __inet_lookup_skb(&tcp_hashinfo, skb, __tcp_hdrlen(th), th->source,
>  			       th->dest, sdif, &refcounted);
> @@ -1679,14 +1689,19 @@ int tcp_v4_rcv(struct sk_buff *skb)
>  		sock_hold(sk);
>  		refcounted = true;
>  		nsk = NULL;
> -		if (!tcp_filter(sk, skb))
> +		if (!tcp_filter(sk, skb)) {
> +			th = (const struct tcphdr *)skb->data;
> +			iph = ip_hdr(skb);
> +			tcp_v4_fill_cb(skb, iph, th);
>  			nsk = tcp_check_req(sk, skb, req, false);
> +		}
>  		if (!nsk) {
>  			reqsk_put(req);
>  			goto discard_and_relse;
>  		}
>  		if (nsk == sk) {
>  			reqsk_put(req);
> +			tcp_v4_restore_cb(skb);
>  		} else if (tcp_child_process(sk, nsk, skb)) {
>  			tcp_v4_send_reset(nsk, skb);
>  			goto discard_and_relse;
> @@ -1712,6 +1727,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
>  		goto discard_and_relse;
>  	th = (const struct tcphdr *)skb->data;
>  	iph = ip_hdr(skb);
> +	tcp_v4_fill_cb(skb, iph, th);
>  
>  	skb->dev = NULL;
>  
> @@ -1742,6 +1758,8 @@ int tcp_v4_rcv(struct sk_buff *skb)
>  	if (!xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb))
>  		goto discard_it;
>  
> +	tcp_v4_fill_cb(skb, iph, th);
> +
>  	if (tcp_checksum_complete(skb)) {
>  csum_error:
>  		__TCP_INC_STATS(net, TCP_MIB_CSUMERRORS);
> @@ -1768,6 +1786,8 @@ int tcp_v4_rcv(struct sk_buff *skb)
>  		goto discard_it;
>  	}
>  
> +	tcp_v4_fill_cb(skb, iph, th);
> +
>  	if (tcp_checksum_complete(skb)) {
>  		inet_twsk_put(inet_twsk(sk));
>  		goto csum_error;
> @@ -1784,6 +1804,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
>  		if (sk2) {
>  			inet_twsk_deschedule_put(inet_twsk(sk));
>  			sk = sk2;
> +			tcp_v4_restore_cb(skb);
>  			refcounted = false;
>  			goto process;
>  		}
> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index 6bb98c93edfe2ed2f16fe5229605f8108cfc7f9a..1f04ec0e4a7aa2c11b8ee27cbdd4067b5bcf32e5 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -1454,7 +1454,6 @@ static int tcp_v6_rcv(struct sk_buff *skb)
>  		struct sock *nsk;
>  
>  		sk = req->rsk_listener;
> -		tcp_v6_fill_cb(skb, hdr, th);
>  		if (tcp_v6_inbound_md5_hash(sk, skb)) {
>  			sk_drops_add(sk, skb);
>  			reqsk_put(req);
> @@ -1467,8 +1466,12 @@ static int tcp_v6_rcv(struct sk_buff *skb)
>  		sock_hold(sk);
>  		refcounted = true;
>  		nsk = NULL;
> -		if (!tcp_filter(sk, skb))
> +		if (!tcp_filter(sk, skb)) {
> +			th = (const struct tcphdr *)skb->data;
> +			hdr = ipv6_hdr(skb);
> +			tcp_v6_fill_cb(skb, hdr, th);
>  			nsk = tcp_check_req(sk, skb, req, false);
> +		}
>  		if (!nsk) {
>  			reqsk_put(req);
>  			goto discard_and_relse;
> @@ -1492,8 +1495,6 @@ static int tcp_v6_rcv(struct sk_buff *skb)
>  	if (!xfrm6_policy_check(sk, XFRM_POLICY_IN, skb))
>  		goto discard_and_relse;
>  
> -	tcp_v6_fill_cb(skb, hdr, th);
> -
>  	if (tcp_v6_inbound_md5_hash(sk, skb))
>  		goto discard_and_relse;
>  
> @@ -1501,6 +1502,7 @@ static int tcp_v6_rcv(struct sk_buff *skb)
>  		goto discard_and_relse;
>  	th = (const struct tcphdr *)skb->data;
>  	hdr = ipv6_hdr(skb);
> +	tcp_v6_fill_cb(skb, hdr, th);
>  
>  	skb->dev = NULL;
>  
> @@ -1590,7 +1592,6 @@ static int tcp_v6_rcv(struct sk_buff *skb)
>  		tcp_v6_timewait_ack(sk, skb);
>  		break;
>  	case TCP_TW_RST:
> -		tcp_v6_restore_cb(skb);
>  		tcp_v6_send_reset(sk, skb);
>  		inet_twsk_deschedule_put(inet_twsk(sk));
>  		goto discard_it;
>
>
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[BUG] kernel stack corruption during/after Netlabel error

[Casey Schaufler]

On 11/30/2017 2:50 AM, Eric Dumazet wrote:
> On Wed, 2017-11-29 at 19:16 -0800, Casey Schaufler wrote:
>> On 11/29/2017 4:31 PM, James Morris wrote:
>>> On Wed, 29 Nov 2017, Casey Schaufler wrote:
>>>
>>>> I see that there is a proposed fix later in the thread, but I
>>>> don't see
>>>> the patch. Could you send it to me, so I can try it on my
>>>> problem?
>>> Forwarded off-list.
>> The patch does fix the problem I was seeing in Smack.
> Can you guys test the following more complete patch ?

My tests are passing. Thank you.

Tested-by: Casey Schaufler <casey@schaufler-ca.com>

>
> It should cover IPv4 and IPv6, and also the corner cases.
>
> ( Note that I squashed ipv6 fix in https://patchwork.ozlabs.org/patch/8
> 42844/ that I spotted while cooking this patch )
>
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index c6bc0c4d19c624888b0d0b5a4246c7183edf63f5..77ea45da0fe9c746907a312989658af3ad3b198d 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -1591,6 +1591,34 @@ int tcp_filter(struct sock *sk, struct sk_buff *skb)
>  }
>  EXPORT_SYMBOL(tcp_filter);
>  
> +static void tcp_v4_restore_cb(struct sk_buff *skb)
> +{
> +	memmove(IPCB(skb), &TCP_SKB_CB(skb)->header.h4,
> +		sizeof(struct inet_skb_parm));
> +}
> +
> +static void tcp_v4_fill_cb(struct sk_buff *skb, const struct iphdr *iph,
> +			   const struct tcphdr *th)
> +{
> +	/* This is tricky : We move IPCB at its correct location into TCP_SKB_CB()
> +	 * barrier() makes sure compiler wont play fool^Waliasing games.
> +	 */
> +	memmove(&TCP_SKB_CB(skb)->header.h4, IPCB(skb),
> +		sizeof(struct inet_skb_parm));
> +	barrier();
> +
> +	TCP_SKB_CB(skb)->seq = ntohl(th->seq);
> +	TCP_SKB_CB(skb)->end_seq = (TCP_SKB_CB(skb)->seq + th->syn + th->fin +
> +				    skb->len - th->doff * 4);
> +	TCP_SKB_CB(skb)->ack_seq = ntohl(th->ack_seq);
> +	TCP_SKB_CB(skb)->tcp_flags = tcp_flag_byte(th);
> +	TCP_SKB_CB(skb)->tcp_tw_isn = 0;
> +	TCP_SKB_CB(skb)->ip_dsfield = ipv4_get_dsfield(iph);
> +	TCP_SKB_CB(skb)->sacked	 = 0;
> +	TCP_SKB_CB(skb)->has_rxtstamp =
> +			skb->tstamp || skb_hwtstamps(skb)->hwtstamp;
> +}
> +
>  /*
>   *	From tcp_input.c
>   */
> @@ -1631,24 +1659,6 @@ int tcp_v4_rcv(struct sk_buff *skb)
>  
>  	th = (const struct tcphdr *)skb->data;
>  	iph = ip_hdr(skb);
> -	/* This is tricky : We move IPCB at its correct location into TCP_SKB_CB()
> -	 * barrier() makes sure compiler wont play fool^Waliasing games.
> -	 */
> -	memmove(&TCP_SKB_CB(skb)->header.h4, IPCB(skb),
> -		sizeof(struct inet_skb_parm));
> -	barrier();
> -
> -	TCP_SKB_CB(skb)->seq = ntohl(th->seq);
> -	TCP_SKB_CB(skb)->end_seq = (TCP_SKB_CB(skb)->seq + th->syn + th->fin +
> -				    skb->len - th->doff * 4);
> -	TCP_SKB_CB(skb)->ack_seq = ntohl(th->ack_seq);
> -	TCP_SKB_CB(skb)->tcp_flags = tcp_flag_byte(th);
> -	TCP_SKB_CB(skb)->tcp_tw_isn = 0;
> -	TCP_SKB_CB(skb)->ip_dsfield = ipv4_get_dsfield(iph);
> -	TCP_SKB_CB(skb)->sacked	 = 0;
> -	TCP_SKB_CB(skb)->has_rxtstamp =
> -			skb->tstamp || skb_hwtstamps(skb)->hwtstamp;
> -
>  lookup:
>  	sk = __inet_lookup_skb(&tcp_hashinfo, skb, __tcp_hdrlen(th), th->source,
>  			       th->dest, sdif, &refcounted);
> @@ -1679,14 +1689,19 @@ int tcp_v4_rcv(struct sk_buff *skb)
>  		sock_hold(sk);
>  		refcounted = true;
>  		nsk = NULL;
> -		if (!tcp_filter(sk, skb))
> +		if (!tcp_filter(sk, skb)) {
> +			th = (const struct tcphdr *)skb->data;
> +			iph = ip_hdr(skb);
> +			tcp_v4_fill_cb(skb, iph, th);
>  			nsk = tcp_check_req(sk, skb, req, false);
> +		}
>  		if (!nsk) {
>  			reqsk_put(req);
>  			goto discard_and_relse;
>  		}
>  		if (nsk == sk) {
>  			reqsk_put(req);
> +			tcp_v4_restore_cb(skb);
>  		} else if (tcp_child_process(sk, nsk, skb)) {
>  			tcp_v4_send_reset(nsk, skb);
>  			goto discard_and_relse;
> @@ -1712,6 +1727,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
>  		goto discard_and_relse;
>  	th = (const struct tcphdr *)skb->data;
>  	iph = ip_hdr(skb);
> +	tcp_v4_fill_cb(skb, iph, th);
>  
>  	skb->dev = NULL;
>  
> @@ -1742,6 +1758,8 @@ int tcp_v4_rcv(struct sk_buff *skb)
>  	if (!xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb))
>  		goto discard_it;
>  
> +	tcp_v4_fill_cb(skb, iph, th);
> +
>  	if (tcp_checksum_complete(skb)) {
>  csum_error:
>  		__TCP_INC_STATS(net, TCP_MIB_CSUMERRORS);
> @@ -1768,6 +1786,8 @@ int tcp_v4_rcv(struct sk_buff *skb)
>  		goto discard_it;
>  	}
>  
> +	tcp_v4_fill_cb(skb, iph, th);
> +
>  	if (tcp_checksum_complete(skb)) {
>  		inet_twsk_put(inet_twsk(sk));
>  		goto csum_error;
> @@ -1784,6 +1804,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
>  		if (sk2) {
>  			inet_twsk_deschedule_put(inet_twsk(sk));
>  			sk = sk2;
> +			tcp_v4_restore_cb(skb);
>  			refcounted = false;
>  			goto process;
>  		}
> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index 6bb98c93edfe2ed2f16fe5229605f8108cfc7f9a..1f04ec0e4a7aa2c11b8ee27cbdd4067b5bcf32e5 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -1454,7 +1454,6 @@ static int tcp_v6_rcv(struct sk_buff *skb)
>  		struct sock *nsk;
>  
>  		sk = req->rsk_listener;
> -		tcp_v6_fill_cb(skb, hdr, th);
>  		if (tcp_v6_inbound_md5_hash(sk, skb)) {
>  			sk_drops_add(sk, skb);
>  			reqsk_put(req);
> @@ -1467,8 +1466,12 @@ static int tcp_v6_rcv(struct sk_buff *skb)
>  		sock_hold(sk);
>  		refcounted = true;
>  		nsk = NULL;
> -		if (!tcp_filter(sk, skb))
> +		if (!tcp_filter(sk, skb)) {
> +			th = (const struct tcphdr *)skb->data;
> +			hdr = ipv6_hdr(skb);
> +			tcp_v6_fill_cb(skb, hdr, th);
>  			nsk = tcp_check_req(sk, skb, req, false);
> +		}
>  		if (!nsk) {
>  			reqsk_put(req);
>  			goto discard_and_relse;
> @@ -1492,8 +1495,6 @@ static int tcp_v6_rcv(struct sk_buff *skb)
>  	if (!xfrm6_policy_check(sk, XFRM_POLICY_IN, skb))
>  		goto discard_and_relse;
>  
> -	tcp_v6_fill_cb(skb, hdr, th);
> -
>  	if (tcp_v6_inbound_md5_hash(sk, skb))
>  		goto discard_and_relse;
>  
> @@ -1501,6 +1502,7 @@ static int tcp_v6_rcv(struct sk_buff *skb)
>  		goto discard_and_relse;
>  	th = (const struct tcphdr *)skb->data;
>  	hdr = ipv6_hdr(skb);
> +	tcp_v6_fill_cb(skb, hdr, th);
>  
>  	skb->dev = NULL;
>  
> @@ -1590,7 +1592,6 @@ static int tcp_v6_rcv(struct sk_buff *skb)
>  		tcp_v6_timewait_ack(sk, skb);
>  		break;
>  	case TCP_TW_RST:
> -		tcp_v6_restore_cb(skb);
>  		tcp_v6_send_reset(sk, skb);
>  		inet_twsk_deschedule_put(inet_twsk(sk));
>  		goto discard_it;
>
>
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[BUG] kernel stack corruption during/after Netlabel error

[David Ahern]

On 11/30/17 3:50 AM, Eric Dumazet wrote:
> @@ -1631,24 +1659,6 @@ int tcp_v4_rcv(struct sk_buff *skb)
>  
>  	th = (const struct tcphdr *)skb->data;
>  	iph = ip_hdr(skb);
> -	/* This is tricky : We move IPCB at its correct location into TCP_SKB_CB()
> -	 * barrier() makes sure compiler wont play fool^Waliasing games.
> -	 */
> -	memmove(&TCP_SKB_CB(skb)->header.h4, IPCB(skb),
> -		sizeof(struct inet_skb_parm));
> -	barrier();
> -
> -	TCP_SKB_CB(skb)->seq = ntohl(th->seq);
> -	TCP_SKB_CB(skb)->end_seq = (TCP_SKB_CB(skb)->seq + th->syn + th->fin +
> -				    skb->len - th->doff * 4);
> -	TCP_SKB_CB(skb)->ack_seq = ntohl(th->ack_seq);
> -	TCP_SKB_CB(skb)->tcp_flags = tcp_flag_byte(th);
> -	TCP_SKB_CB(skb)->tcp_tw_isn = 0;
> -	TCP_SKB_CB(skb)->ip_dsfield = ipv4_get_dsfield(iph);
> -	TCP_SKB_CB(skb)->sacked	 = 0;
> -	TCP_SKB_CB(skb)->has_rxtstamp =
> -			skb->tstamp || skb_hwtstamps(skb)->hwtstamp;
> -
>  lookup:
>  	sk = __inet_lookup_skb(&tcp_hashinfo, skb, __tcp_hdrlen(th), th->source,
>  			       th->dest, sdif, &refcounted);

I believe moving the above is going to affect lookups with VRF. Let me
take a look before this gets committed.

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[BUG] kernel stack corruption during/after Netlabel error

[Paul Moore]

On Thu, Nov 30, 2017 at 7:47 AM, Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Nov 30, 2017 at 5:50 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> On Wed, 2017-11-29 at 19:16 -0800, Casey Schaufler wrote:
>>> On 11/29/2017 4:31 PM, James Morris wrote:
>>> > On Wed, 29 Nov 2017, Casey Schaufler wrote:
>>> >
>>> > > I see that there is a proposed fix later in the thread, but I
>>> > > don't see
>>> > > the patch. Could you send it to me, so I can try it on my
>>> > > problem?
>>> >
>>> > Forwarded off-list.
>>>
>>> The patch does fix the problem I was seeing in Smack.
>>
>> Can you guys test the following more complete patch ?
>>
>> It should cover IPv4 and IPv6, and also the corner cases.
>>
>> ( Note that I squashed ipv6 fix in https://patchwork.ozlabs.org/patch/8
>> 42844/ that I spotted while cooking this patch )
>
> Building a test kernel now, although it make take me a few hours to
> test it due to some commitments this morning.

I just realized I forgot to enable KASAN in the build, but I can
verify that the patch doesn't break anything in the selinux-testsuite.

Tested-by: Paul Moore <paul@paul-moore.com>

>> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
>> index c6bc0c4d19c624888b0d0b5a4246c7183edf63f5..77ea45da0fe9c746907a312989658af3ad3b198d 100644
>> --- a/net/ipv4/tcp_ipv4.c
>> +++ b/net/ipv4/tcp_ipv4.c
>> @@ -1591,6 +1591,34 @@ int tcp_filter(struct sock *sk, struct sk_buff *skb)
>>  }
>>  EXPORT_SYMBOL(tcp_filter);
>>
>> +static void tcp_v4_restore_cb(struct sk_buff *skb)
>> +{
>> +       memmove(IPCB(skb), &TCP_SKB_CB(skb)->header.h4,
>> +               sizeof(struct inet_skb_parm));
>> +}
>> +
>> +static void tcp_v4_fill_cb(struct sk_buff *skb, const struct iphdr *iph,
>> +                          const struct tcphdr *th)
>> +{
>> +       /* This is tricky : We move IPCB at its correct location into TCP_SKB_CB()
>> +        * barrier() makes sure compiler wont play fool^Waliasing games.
>> +        */
>> +       memmove(&TCP_SKB_CB(skb)->header.h4, IPCB(skb),
>> +               sizeof(struct inet_skb_parm));
>> +       barrier();
>> +
>> +       TCP_SKB_CB(skb)->seq = ntohl(th->seq);
>> +       TCP_SKB_CB(skb)->end_seq = (TCP_SKB_CB(skb)->seq + th->syn + th->fin +
>> +                                   skb->len - th->doff * 4);
>> +       TCP_SKB_CB(skb)->ack_seq = ntohl(th->ack_seq);
>> +       TCP_SKB_CB(skb)->tcp_flags = tcp_flag_byte(th);
>> +       TCP_SKB_CB(skb)->tcp_tw_isn = 0;
>> +       TCP_SKB_CB(skb)->ip_dsfield = ipv4_get_dsfield(iph);
>> +       TCP_SKB_CB(skb)->sacked  = 0;
>> +       TCP_SKB_CB(skb)->has_rxtstamp =
>> +                       skb->tstamp || skb_hwtstamps(skb)->hwtstamp;
>> +}
>> +
>>  /*
>>   *     From tcp_input.c
>>   */
>> @@ -1631,24 +1659,6 @@ int tcp_v4_rcv(struct sk_buff *skb)
>>
>>         th = (const struct tcphdr *)skb->data;
>>         iph = ip_hdr(skb);
>> -       /* This is tricky : We move IPCB at its correct location into TCP_SKB_CB()
>> -        * barrier() makes sure compiler wont play fool^Waliasing games.
>> -        */
>> -       memmove(&TCP_SKB_CB(skb)->header.h4, IPCB(skb),
>> -               sizeof(struct inet_skb_parm));
>> -       barrier();
>> -
>> -       TCP_SKB_CB(skb)->seq = ntohl(th->seq);
>> -       TCP_SKB_CB(skb)->end_seq = (TCP_SKB_CB(skb)->seq + th->syn + th->fin +
>> -                                   skb->len - th->doff * 4);
>> -       TCP_SKB_CB(skb)->ack_seq = ntohl(th->ack_seq);
>> -       TCP_SKB_CB(skb)->tcp_flags = tcp_flag_byte(th);
>> -       TCP_SKB_CB(skb)->tcp_tw_isn = 0;
>> -       TCP_SKB_CB(skb)->ip_dsfield = ipv4_get_dsfield(iph);
>> -       TCP_SKB_CB(skb)->sacked  = 0;
>> -       TCP_SKB_CB(skb)->has_rxtstamp =
>> -                       skb->tstamp || skb_hwtstamps(skb)->hwtstamp;
>> -
>>  lookup:
>>         sk = __inet_lookup_skb(&tcp_hashinfo, skb, __tcp_hdrlen(th), th->source,
>>                                th->dest, sdif, &refcounted);
>> @@ -1679,14 +1689,19 @@ int tcp_v4_rcv(struct sk_buff *skb)
>>                 sock_hold(sk);
>>                 refcounted = true;
>>                 nsk = NULL;
>> -               if (!tcp_filter(sk, skb))
>> +               if (!tcp_filter(sk, skb)) {
>> +                       th = (const struct tcphdr *)skb->data;
>> +                       iph = ip_hdr(skb);
>> +                       tcp_v4_fill_cb(skb, iph, th);
>>                         nsk = tcp_check_req(sk, skb, req, false);
>> +               }
>>                 if (!nsk) {
>>                         reqsk_put(req);
>>                         goto discard_and_relse;
>>                 }
>>                 if (nsk == sk) {
>>                         reqsk_put(req);
>> +                       tcp_v4_restore_cb(skb);
>>                 } else if (tcp_child_process(sk, nsk, skb)) {
>>                         tcp_v4_send_reset(nsk, skb);
>>                         goto discard_and_relse;
>> @@ -1712,6 +1727,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
>>                 goto discard_and_relse;
>>         th = (const struct tcphdr *)skb->data;
>>         iph = ip_hdr(skb);
>> +       tcp_v4_fill_cb(skb, iph, th);
>>
>>         skb->dev = NULL;
>>
>> @@ -1742,6 +1758,8 @@ int tcp_v4_rcv(struct sk_buff *skb)
>>         if (!xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb))
>>                 goto discard_it;
>>
>> +       tcp_v4_fill_cb(skb, iph, th);
>> +
>>         if (tcp_checksum_complete(skb)) {
>>  csum_error:
>>                 __TCP_INC_STATS(net, TCP_MIB_CSUMERRORS);
>> @@ -1768,6 +1786,8 @@ int tcp_v4_rcv(struct sk_buff *skb)
>>                 goto discard_it;
>>         }
>>
>> +       tcp_v4_fill_cb(skb, iph, th);
>> +
>>         if (tcp_checksum_complete(skb)) {
>>                 inet_twsk_put(inet_twsk(sk));
>>                 goto csum_error;
>> @@ -1784,6 +1804,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
>>                 if (sk2) {
>>                         inet_twsk_deschedule_put(inet_twsk(sk));
>>                         sk = sk2;
>> +                       tcp_v4_restore_cb(skb);
>>                         refcounted = false;
>>                         goto process;
>>                 }
>> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
>> index 6bb98c93edfe2ed2f16fe5229605f8108cfc7f9a..1f04ec0e4a7aa2c11b8ee27cbdd4067b5bcf32e5 100644
>> --- a/net/ipv6/tcp_ipv6.c
>> +++ b/net/ipv6/tcp_ipv6.c
>> @@ -1454,7 +1454,6 @@ static int tcp_v6_rcv(struct sk_buff *skb)
>>                 struct sock *nsk;
>>
>>                 sk = req->rsk_listener;
>> -               tcp_v6_fill_cb(skb, hdr, th);
>>                 if (tcp_v6_inbound_md5_hash(sk, skb)) {
>>                         sk_drops_add(sk, skb);
>>                         reqsk_put(req);
>> @@ -1467,8 +1466,12 @@ static int tcp_v6_rcv(struct sk_buff *skb)
>>                 sock_hold(sk);
>>                 refcounted = true;
>>                 nsk = NULL;
>> -               if (!tcp_filter(sk, skb))
>> +               if (!tcp_filter(sk, skb)) {
>> +                       th = (const struct tcphdr *)skb->data;
>> +                       hdr = ipv6_hdr(skb);
>> +                       tcp_v6_fill_cb(skb, hdr, th);
>>                         nsk = tcp_check_req(sk, skb, req, false);
>> +               }
>>                 if (!nsk) {
>>                         reqsk_put(req);
>>                         goto discard_and_relse;
>> @@ -1492,8 +1495,6 @@ static int tcp_v6_rcv(struct sk_buff *skb)
>>         if (!xfrm6_policy_check(sk, XFRM_POLICY_IN, skb))
>>                 goto discard_and_relse;
>>
>> -       tcp_v6_fill_cb(skb, hdr, th);
>> -
>>         if (tcp_v6_inbound_md5_hash(sk, skb))
>>                 goto discard_and_relse;
>>
>> @@ -1501,6 +1502,7 @@ static int tcp_v6_rcv(struct sk_buff *skb)
>>                 goto discard_and_relse;
>>         th = (const struct tcphdr *)skb->data;
>>         hdr = ipv6_hdr(skb);
>> +       tcp_v6_fill_cb(skb, hdr, th);
>>
>>         skb->dev = NULL;
>>
>> @@ -1590,7 +1592,6 @@ static int tcp_v6_rcv(struct sk_buff *skb)
>>                 tcp_v6_timewait_ack(sk, skb);
>>                 break;
>>         case TCP_TW_RST:
>> -               tcp_v6_restore_cb(skb);
>>                 tcp_v6_send_reset(sk, skb);
>>                 inet_twsk_deschedule_put(inet_twsk(sk));
>>                 goto discard_it;
>>
>>
>>
>
>
>
> --
> paul moore
> www.paul-moore.com



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[BUG] kernel stack corruption during/after Netlabel error

[David Ahern]

On 11/30/17 8:44 AM, David Ahern wrote:
> On 11/30/17 3:50 AM, Eric Dumazet wrote:
>> @@ -1631,24 +1659,6 @@ int tcp_v4_rcv(struct sk_buff *skb)
>>  
>>  	th = (const struct tcphdr *)skb->data;
>>  	iph = ip_hdr(skb);
>> -	/* This is tricky : We move IPCB at its correct location into TCP_SKB_CB()
>> -	 * barrier() makes sure compiler wont play fool^Waliasing games.
>> -	 */
>> -	memmove(&TCP_SKB_CB(skb)->header.h4, IPCB(skb),
>> -		sizeof(struct inet_skb_parm));
>> -	barrier();
>> -
>> -	TCP_SKB_CB(skb)->seq = ntohl(th->seq);
>> -	TCP_SKB_CB(skb)->end_seq = (TCP_SKB_CB(skb)->seq + th->syn + th->fin +
>> -				    skb->len - th->doff * 4);
>> -	TCP_SKB_CB(skb)->ack_seq = ntohl(th->ack_seq);
>> -	TCP_SKB_CB(skb)->tcp_flags = tcp_flag_byte(th);
>> -	TCP_SKB_CB(skb)->tcp_tw_isn = 0;
>> -	TCP_SKB_CB(skb)->ip_dsfield = ipv4_get_dsfield(iph);
>> -	TCP_SKB_CB(skb)->sacked	 = 0;
>> -	TCP_SKB_CB(skb)->has_rxtstamp =
>> -			skb->tstamp || skb_hwtstamps(skb)->hwtstamp;
>> -
>>  lookup:
>>  	sk = __inet_lookup_skb(&tcp_hashinfo, skb, __tcp_hdrlen(th), th->source,
>>  			       th->dest, sdif, &refcounted);
> 
> I believe moving the above is going to affect lookups with VRF. Let me
> take a look before this gets committed.
> 

Eric:

Can you add this to the patch? Fixes socket lookups with VRF which
stashes a flag in the cb.

Thanks,

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 4e09398009c1..6c020015d556 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -849,7 +849,7 @@ static inline bool inet_exact_dif_match(struct net
*net, struct sk_buff *skb)
 {
 #if IS_ENABLED(CONFIG_NET_L3_MASTER_DEV)
        if (!net->ipv4.sysctl_tcp_l3mdev_accept &&
-           skb && ipv4_l3mdev_skb(TCP_SKB_CB(skb)->header.h4.flags))
+           skb && ipv4_l3mdev_skb(IPCB(skb)->flags))
                return true;
 #endif
        return false;
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[BUG] kernel stack corruption during/after Netlabel error

[Eric Dumazet]

On Thu, 2017-11-30 at 10:30 -0700, David Ahern wrote:
> On 11/30/17 8:44 AM, David Ahern wrote:
> > On 11/30/17 3:50 AM, Eric Dumazet wrote:
> > > @@ -1631,24 +1659,6 @@ int tcp_v4_rcv(struct sk_buff *skb)
> > > ?
> > > ?	th = (const struct tcphdr *)skb->data;
> > > ?	iph = ip_hdr(skb);
> > > -	/* This is tricky : We move IPCB at its correct location
> > > into TCP_SKB_CB()
> > > -	?* barrier() makes sure compiler wont play
> > > fool^Waliasing games.
> > > -	?*/
> > > -	memmove(&TCP_SKB_CB(skb)->header.h4, IPCB(skb),
> > > -		sizeof(struct inet_skb_parm));
> > > -	barrier();
> > > -
> > > -	TCP_SKB_CB(skb)->seq = ntohl(th->seq);
> > > -	TCP_SKB_CB(skb)->end_seq = (TCP_SKB_CB(skb)->seq + th-
> > > >syn + th->fin +
> > > -				????skb->len - th->doff * 4);
> > > -	TCP_SKB_CB(skb)->ack_seq = ntohl(th->ack_seq);
> > > -	TCP_SKB_CB(skb)->tcp_flags = tcp_flag_byte(th);
> > > -	TCP_SKB_CB(skb)->tcp_tw_isn = 0;
> > > -	TCP_SKB_CB(skb)->ip_dsfield = ipv4_get_dsfield(iph);
> > > -	TCP_SKB_CB(skb)->sacked	?= 0;
> > > -	TCP_SKB_CB(skb)->has_rxtstamp =
> > > -			skb->tstamp || skb_hwtstamps(skb)-
> > > >hwtstamp;
> > > -
> > > ?lookup:
> > > ?	sk = __inet_lookup_skb(&tcp_hashinfo, skb,
> > > __tcp_hdrlen(th), th->source,
> > > ?			???????th->dest, sdif, &refcounted);
> > 
> > I believe moving the above is going to affect lookups with VRF. Let
> > me
> > take a look before this gets committed.
> > 
> 
> Eric:
> 
> Can you add this to the patch? Fixes socket lookups with VRF which
> stashes a flag in the cb.
> 
> Thanks,
> 
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 4e09398009c1..6c020015d556 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -849,7 +849,7 @@ static inline bool inet_exact_dif_match(struct
> net
> *net, struct sk_buff *skb)
> ?{
> ?#if IS_ENABLED(CONFIG_NET_L3_MASTER_DEV)
> ????????if (!net->ipv4.sysctl_tcp_l3mdev_accept &&
> -???????????skb && ipv4_l3mdev_skb(TCP_SKB_CB(skb)->header.h4.flags))
> +???????????skb && ipv4_l3mdev_skb(IPCB(skb)->flags))
> ????????????????return true;
> ?#endif
> ????????return false;


I wonder if this should not be in a separate patch ?

Bug was added in 971f10eca186cab238c49daa91f703c5a001b0b1 ("tcp: better
TCP_SKB_CB layout to reduce cache line misses")  in linux 3.18

While VRF was added later.

If you agree, I will prepare a patch series, with different Fixes tag
so that David can decide which path needs to be backported into each
stable version.

Thanks.

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[BUG] kernel stack corruption during/after Netlabel error

[David Ahern]

On 11/30/17 10:57 AM, Eric Dumazet wrote:
> I wonder if this should not be in a separate patch ?
> 
> Bug was added in 971f10eca186cab238c49daa91f703c5a001b0b1 ("tcp: better
> TCP_SKB_CB layout to reduce cache line misses")  in linux 3.18
> 
> While VRF was added later.
> 
> If you agree, I will prepare a patch series, with different Fixes tag
> so that David can decide which path needs to be backported into each
> stable version.
> 

That's sound fine to me.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[BUG] kernel stack corruption during/after Netlabel error

[Casey Schaufler]

On 11/30/2017 9:57 AM, Eric Dumazet wrote:
> On Thu, 2017-11-30 at 10:30 -0700, David Ahern wrote:
>> On 11/30/17 8:44 AM, David Ahern wrote:
>>> On 11/30/17 3:50 AM, Eric Dumazet wrote:
>>>> @@ -1631,24 +1659,6 @@ int tcp_v4_rcv(struct sk_buff *skb)
>>>> ?
>>>> ?	th = (const struct tcphdr *)skb->data;
>>>> ?	iph = ip_hdr(skb);
>>>> -	/* This is tricky : We move IPCB at its correct location
>>>> into TCP_SKB_CB()
>>>> -	?* barrier() makes sure compiler wont play
>>>> fool^Waliasing games.
>>>> -	?*/
>>>> -	memmove(&TCP_SKB_CB(skb)->header.h4, IPCB(skb),
>>>> -		sizeof(struct inet_skb_parm));
>>>> -	barrier();
>>>> -
>>>> -	TCP_SKB_CB(skb)->seq = ntohl(th->seq);
>>>> -	TCP_SKB_CB(skb)->end_seq = (TCP_SKB_CB(skb)->seq + th-
>>>>> syn + th->fin +
>>>> -				????skb->len - th->doff * 4);
>>>> -	TCP_SKB_CB(skb)->ack_seq = ntohl(th->ack_seq);
>>>> -	TCP_SKB_CB(skb)->tcp_flags = tcp_flag_byte(th);
>>>> -	TCP_SKB_CB(skb)->tcp_tw_isn = 0;
>>>> -	TCP_SKB_CB(skb)->ip_dsfield = ipv4_get_dsfield(iph);
>>>> -	TCP_SKB_CB(skb)->sacked	?= 0;
>>>> -	TCP_SKB_CB(skb)->has_rxtstamp =
>>>> -			skb->tstamp || skb_hwtstamps(skb)-
>>>>> hwtstamp;
>>>> -
>>>> ?lookup:
>>>> ?	sk = __inet_lookup_skb(&tcp_hashinfo, skb,
>>>> __tcp_hdrlen(th), th->source,
>>>> ?			???????th->dest, sdif, &refcounted);
>>> I believe moving the above is going to affect lookups with VRF. Let
>>> me
>>> take a look before this gets committed.
>>>
>> Eric:
>>
>> Can you add this to the patch? Fixes socket lookups with VRF which
>> stashes a flag in the cb.

I've done my testing and it works both ways for Smack.


>>
>> Thanks,
>>
>> diff --git a/include/net/tcp.h b/include/net/tcp.h
>> index 4e09398009c1..6c020015d556 100644
>> --- a/include/net/tcp.h
>> +++ b/include/net/tcp.h
>> @@ -849,7 +849,7 @@ static inline bool inet_exact_dif_match(struct
>> net
>> *net, struct sk_buff *skb)
>> ?{
>> ?#if IS_ENABLED(CONFIG_NET_L3_MASTER_DEV)
>> ????????if (!net->ipv4.sysctl_tcp_l3mdev_accept &&
>> -???????????skb && ipv4_l3mdev_skb(TCP_SKB_CB(skb)->header.h4.flags))
>> +???????????skb && ipv4_l3mdev_skb(IPCB(skb)->flags))
>> ????????????????return true;
>> ?#endif
>> ????????return false;
>
> I wonder if this should not be in a separate patch ?
>
> Bug was added in 971f10eca186cab238c49daa91f703c5a001b0b1 ("tcp: better
> TCP_SKB_CB layout to reduce cache line misses")  in linux 3.18
>
> While VRF was added later.
>
> If you agree, I will prepare a patch series, with different Fixes tag
> so that David can decide which path needs to be backported into each
> stable version.
>
> Thanks.
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[BUG] kernel stack corruption during/after Netlabel error

[James Morris]

On Thu, 30 Nov 2017, Eric Dumazet wrote:

> On Wed, 2017-11-29 at 19:16 -0800, Casey Schaufler wrote:
> > On 11/29/2017 4:31 PM, James Morris wrote:
> > > On Wed, 29 Nov 2017, Casey Schaufler wrote:
> > > 
> > > > I see that there is a proposed fix later in the thread, but I
> > > > don't see
> > > > the patch. Could you send it to me, so I can try it on my
> > > > problem?
> > > 
> > > Forwarded off-list.
> > 
> > The patch does fix the problem I was seeing in Smack.
> 
> Can you guys test the following more complete patch ?
> 
> It should cover IPv4 and IPv6, and also the corner cases.


Tested-by: James Morris <james.l.morris@oracle.com>



-- 
James Morris
<james.l.morris@oracle.com>

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html