| Message ID | 1491153972.10124.14.camel@edumazet-glaptop3.roam.corp.google.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
On 2017-04-02 20:26, Eric Dumazet wrote: > On Sun, 2017-04-02 at 10:14 -0700, Eric Dumazet wrote: > >> Could that be that netfilter does not abort earlier if TCP header is >> completely wrong ? >> > > Yes, I wonder if this patch would be better, unless we replicate the > th->doff sanity check in all netfilter modules dissecting TCP frames. > > diff --git a/net/netfilter/xt_tcpudp.c b/net/netfilter/xt_tcpudp.c > index > ade024c90f4f129a7c384e9e1cbfdb8ffe73065f..8cb4eadd5ba1c20e74bc27ee52a0bc36a5b26725 > 100644 > --- a/net/netfilter/xt_tcpudp.c > +++ b/net/netfilter/xt_tcpudp.c > @@ -103,11 +103,11 @@ static bool tcp_mt(const struct sk_buff *skb, > struct xt_action_param *par) > if (!NF_INVF(tcpinfo, XT_TCP_INV_FLAGS, > (((unsigned char *)th)[13] & tcpinfo->flg_mask) == > tcpinfo->flg_cmp)) > return false; > + if (th->doff * 4 < sizeof(_tcph)) { > + par->hotdrop = true; > + return false; > + } > if (tcpinfo->option) { > - if (th->doff * 4 < sizeof(_tcph)) { > - par->hotdrop = true; > - return false; > - } > if (!tcp_find_option(tcpinfo->option, skb, par->thoff, > th->doff*4 - sizeof(_tcph), > tcpinfo->invflags & XT_TCP_INV_OPTION, I modified patch a little as: if (th->doff * 4 < sizeof(_tcph)) { par->hotdrop = true; WARN_ON_ONCE(!tcpinfo->option); return false; } And it did triggered WARN once at morning, and didn't hit KASAN. I will run for a while more, to see if it is ok, and then if stable, will try to enable SFQ again.
On Mon, 2017-04-03 at 11:10 +0300, Denys Fedoryshchenko wrote: > I modified patch a little as: > if (th->doff * 4 < sizeof(_tcph)) { > par->hotdrop = true; > WARN_ON_ONCE(!tcpinfo->option); > return false; > } > > And it did triggered WARN once at morning, and didn't hit KASAN. I will > run for a while more, to see if it is ok, and then if stable, will try > to enable SFQ again. Excellent news ! We will post an official fix today, thanks a lot for this detective work Denys.
On 2017-04-03 15:09, Eric Dumazet wrote: > On Mon, 2017-04-03 at 11:10 +0300, Denys Fedoryshchenko wrote: > >> I modified patch a little as: >> if (th->doff * 4 < sizeof(_tcph)) { >> par->hotdrop = true; >> WARN_ON_ONCE(!tcpinfo->option); >> return false; >> } >> >> And it did triggered WARN once at morning, and didn't hit KASAN. I >> will >> run for a while more, to see if it is ok, and then if stable, will try >> to enable SFQ again. > > Excellent news ! > We will post an official fix today, thanks a lot for this detective > work > Denys. I am not sure it is finally fixed, maybe we need test more? I'm doing extensive tests today with identical configuration (i had to run fifo, because customer cannot afford anymore outages). I've dded sfq now different way, and identical config i will run after 3 hours approx.
On Mon, 2017-04-03 at 15:14 +0300, Denys Fedoryshchenko wrote: > On 2017-04-03 15:09, Eric Dumazet wrote: > > On Mon, 2017-04-03 at 11:10 +0300, Denys Fedoryshchenko wrote: > > > >> I modified patch a little as: > >> if (th->doff * 4 < sizeof(_tcph)) { > >> par->hotdrop = true; > >> WARN_ON_ONCE(!tcpinfo->option); > >> return false; > >> } > >> > >> And it did triggered WARN once at morning, and didn't hit KASAN. I > >> will > >> run for a while more, to see if it is ok, and then if stable, will try > >> to enable SFQ again. > > > > Excellent news ! > > We will post an official fix today, thanks a lot for this detective > > work > > Denys. > I am not sure it is finally fixed, maybe we need test more? > I'm doing extensive tests today with identical configuration (i had to > run fifo, because customer cannot afford anymore outages). I've dded sfq > now different way, and identical config i will run after 3 hours approx. I did not say this bug fix would be the last one. But this check is definitely needed, otherwise xt_TCPMSS can iterate whole memory, and either crash or scramble two bytes in memory, that do not belong to the frame at all. If tcp_hdrlen is 0 (can happen if tcph->doff is 0) Then : for (i = sizeof(struct tcphdr); i <= tcp_hdrlen - TCPOLEN_MSS; i += optlen(opt, i)) { Can look at 4 GB of memory, until it finds a 0x02, 0x04 sequence. It can also loop forever in some cases, depending on memory content.
diff --git a/net/netfilter/xt_tcpudp.c b/net/netfilter/xt_tcpudp.c index ade024c90f4f129a7c384e9e1cbfdb8ffe73065f..8cb4eadd5ba1c20e74bc27ee52a0bc36a5b26725 100644 --- a/net/netfilter/xt_tcpudp.c +++ b/net/netfilter/xt_tcpudp.c @@ -103,11 +103,11 @@ static bool tcp_mt(const struct sk_buff *skb, struct xt_action_param *par) if (!NF_INVF(tcpinfo, XT_TCP_INV_FLAGS, (((unsigned char *)th)[13] & tcpinfo->flg_mask) == tcpinfo->flg_cmp)) return false; + if (th->doff * 4 < sizeof(_tcph)) { + par->hotdrop = true; + return false; + } if (tcpinfo->option) { - if (th->doff * 4 < sizeof(_tcph)) { - par->hotdrop = true; - return false; - } if (!tcp_find_option(tcpinfo->option, skb, par->thoff, th->doff*4 - sizeof(_tcph), tcpinfo->invflags & XT_TCP_INV_OPTION,