From: eric.dumazet@gmail.com (Eric Dumazet)
To: linux-security-module@vger.kernel.org
Subject: [BUG] kernel stack corruption during/after Netlabel error
Date: Thu, 30 Nov 2017 09:57:09 -0800 [thread overview]
Message-ID: <1512064629.19682.21.camel@gmail.com> (raw)
In-Reply-To: <d29937fd-dd04-8568-f89c-505ff725761a@gmail.com>
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
WARNING: multiple messages have this Message-ID (diff)
From: Eric Dumazet <eric.dumazet@gmail.com>
To: David Ahern <dsahern@gmail.com>,
Casey Schaufler <casey@schaufler-ca.com>,
James Morris <james.l.morris@oracle.com>
Cc: Paul Moore <paul@paul-moore.com>,
netdev@vger.kernel.org, Stephen Smalley <sds@tycho.nsa.gov>,
selinux@tycho.nsa.gov,
LSM <linux-security-module@vger.kernel.org>
Subject: Re: [BUG] kernel stack corruption during/after Netlabel error
Date: Thu, 30 Nov 2017 09:57:09 -0800 [thread overview]
Message-ID: <1512064629.19682.21.camel@gmail.com> (raw)
In-Reply-To: <d29937fd-dd04-8568-f89c-505ff725761a@gmail.com>
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.
next prev parent reply other threads:[~2017-11-30 17:57 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-29 10:26 [BUG] kernel stack corruption during/after Netlabel error James Morris
2017-11-29 12:29 ` Eric Dumazet
2017-11-29 17:31 ` Stephen Smalley
2017-11-29 17:34 ` Eric Dumazet
2017-11-29 19:29 ` Paul Moore
2017-11-29 19:59 ` Stephen Smalley
2017-11-29 19:59 ` Stephen Smalley
2017-11-29 20:23 ` Eric Dumazet
2017-11-29 22:49 ` Eric Dumazet
2017-11-29 23:41 ` James Morris
2017-11-30 0:22 ` Casey Schaufler
2017-11-30 0:22 ` Casey Schaufler
2017-11-30 0:31 ` James Morris
2017-11-30 0:31 ` James Morris
2017-11-30 3:16 ` Casey Schaufler
2017-11-30 3:16 ` Casey Schaufler
2017-11-30 10:50 ` Eric Dumazet
2017-11-30 10:50 ` Eric Dumazet
2017-11-30 12:47 ` Paul Moore
2017-11-30 12:47 ` Paul Moore
2017-11-30 16:57 ` Paul Moore
2017-11-30 16:57 ` Paul Moore
2017-11-30 14:33 ` Casey Schaufler
2017-11-30 14:33 ` Casey Schaufler
2017-11-30 15:11 ` Casey Schaufler
2017-11-30 15:11 ` Casey Schaufler
2017-11-30 15:44 ` David Ahern
2017-11-30 15:44 ` David Ahern
2017-11-30 17:30 ` David Ahern
2017-11-30 17:30 ` David Ahern
2017-11-30 17:57 ` Eric Dumazet [this message]
2017-11-30 17:57 ` Eric Dumazet
2017-11-30 18:03 ` David Ahern
2017-11-30 18:03 ` David Ahern
2017-11-30 18:16 ` Casey Schaufler
2017-11-30 18:16 ` Casey Schaufler
2017-12-01 1:55 ` James Morris
2017-12-01 1:55 ` James Morris
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1512064629.19682.21.camel@gmail.com \
--to=eric.dumazet@gmail.com \
--cc=linux-security-module@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.