| Message ID | 1333336250-4110-1-git-send-email-xiaosuo@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
On Mon, 2012-04-02 at 11:10 +0800, Changli Gao wrote: > We should check the length of the data before dereferencing it when parsing > the TCP options. > > Signed-off-by: Changli Gao <xiaosuo@gmail.com> > --- > net/ipv4/tcp_input.c | 2 ++ > 1 file changed, 2 insertions(+) > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > index e886e2f..5099f08 100644 > --- a/net/ipv4/tcp_input.c > +++ b/net/ipv4/tcp_input.c > @@ -3845,6 +3845,8 @@ void tcp_parse_options(const struct sk_buff *skb, struct tcp_options_received *o > length--; > continue; > default: > + if (length < 2) > + return; > opsize = *ptr++; > if (opsize < 2) /* "silly options" */ > return; Acked-by: Eric Dumazet <eric.dumazet@gmail.com> -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Changli Gao <xiaosuo@gmail.com> Date: Mon, 2 Apr 2012 11:10:50 +0800 > We should check the length of the data before dereferencing it when parsing > the TCP options. > > Signed-off-by: Changli Gao <xiaosuo@gmail.com> Proper Subject prefix here would be "tcp: ", not "net: " and maybe adjust the subject line to also mention the specific function being fixed, which in this case would be tcp_parse_options(). So: tcp: Validate length of data before dereference in tcp_parse_options(). and then you can make the commit message just be your signoff. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Mon, 02 Apr 2012 05:19:33 +0200 >> @@ -3845,6 +3845,8 @@ void tcp_parse_options(const struct sk_buff *skb, struct tcp_options_received *o >> length--; >> continue; >> default: >> + if (length < 2) >> + return; >> opsize = *ptr++; >> if (opsize < 2) /* "silly options" */ >> return; > > Acked-by: Eric Dumazet <eric.dumazet@gmail.com> Tag Eric, you're it. You ACK'd this patch, so you get to show how this is actually able to cause some kind of problem. I assert that this is adding a useless test, that doesn't fix any kind of possible crash or misbehavior. If length == 1 at the default:, the code will absolutely do the right thing. Prove me wrong. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 2, 2012 at 11:29 AM, David Miller <davem@davemloft.net> wrote: > > Tag Eric, you're it. > > You ACK'd this patch, so you get to show how this is actually able > to cause some kind of problem. > > I assert that this is adding a useless test, that doesn't fix any kind > of possible crash or misbehavior. If length == 1 at the default:, the > code will absolutely do the right thing. > > Prove me wrong. Thinking about a malformed tcp segment, which has no data but silly options, and whose last byte is neither TCPOPT_EOL or TCPOPT_NOP, we will try to dereference one byte over the boundary when parsing the options. I know we have skb_shared_info at the end and it won't cause any crash, but should we rely on this fact?
On Sun, 2012-04-01 at 23:29 -0400, David Miller wrote: > From: Eric Dumazet <eric.dumazet@gmail.com> > Date: Mon, 02 Apr 2012 05:19:33 +0200 > > >> @@ -3845,6 +3845,8 @@ void tcp_parse_options(const struct sk_buff *skb, struct tcp_options_received *o > >> length--; > >> continue; > >> default: > >> + if (length < 2) > >> + return; > >> opsize = *ptr++; > >> if (opsize < 2) /* "silly options" */ > >> return; > > > > Acked-by: Eric Dumazet <eric.dumazet@gmail.com> > > Tag Eric, you're it. > > You ACK'd this patch, so you get to show how this is actually able > to cause some kind of problem. > > I assert that this is adding a useless test, that doesn't fix any kind > of possible crash or misbehavior. If length == 1 at the default:, the > code will absolutely do the right thing. > > Prove me wrong. No problem. You can have NOP,NOP,NOP,EVIL-OPTION initial length=4 (multiple of 4) We can read 5 bytes, and access 'out of bound' memory. Usually not a problem since we have many bytes after our head. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2012-04-02 at 11:45 +0800, Changli Gao wrote: > Thinking about a malformed tcp segment, which has no data but silly > options, and whose last byte is neither TCPOPT_EOL or TCPOPT_NOP, we > will try to dereference one byte over the boundary when parsing the > options. I know we have skb_shared_info at the end and it won't cause > any crash, but should we rely on this fact? > No we cant rely on this, kmemcheck might barf on us. Your patch (and the netfilter one) is fine. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Mon, 02 Apr 2012 05:45:45 +0200 > Usually not a problem since we have many bytes after our head. We always have bytes after the head, it's guarenteed, and whether it's garbage bytes or skb_shared_info() it simply doesn't matter. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Mon, 02 Apr 2012 05:53:17 +0200 > On Mon, 2012-04-02 at 11:45 +0800, Changli Gao wrote: > >> Thinking about a malformed tcp segment, which has no data but silly >> options, and whose last byte is neither TCPOPT_EOL or TCPOPT_NOP, we >> will try to dereference one byte over the boundary when parsing the >> options. I know we have skb_shared_info at the end and it won't cause >> any crash, but should we rely on this fact? >> > > No we cant rely on this, kmemcheck might barf on us. Give me a break. The code does the right thing, in every possible case, and in every possible valid state of an SKB. If we can't make kmemcheck handle that, tough, I'm not adding useless tests to a function, specifically tests which are always there and that don't fix anything. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2012-04-01 at 23:55 -0400, David Miller wrote: > From: Eric Dumazet <eric.dumazet@gmail.com> > Date: Mon, 02 Apr 2012 05:45:45 +0200 > > > Usually not a problem since we have many bytes after our head. > > We always have bytes after the head, it's guarenteed, and whether it's > garbage bytes or skb_shared_info() it simply doesn't matter. Then you have to add a kmemcheck_something() to make this clear and avoid possible warnings. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2012-04-01 at 23:57 -0400, David Miller wrote: > From: Eric Dumazet <eric.dumazet@gmail.com> > Date: Mon, 02 Apr 2012 05:53:17 +0200 > > > On Mon, 2012-04-02 at 11:45 +0800, Changli Gao wrote: > > > >> Thinking about a malformed tcp segment, which has no data but silly > >> options, and whose last byte is neither TCPOPT_EOL or TCPOPT_NOP, we > >> will try to dereference one byte over the boundary when parsing the > >> options. I know we have skb_shared_info at the end and it won't cause > >> any crash, but should we rely on this fact? > >> > > > > No we cant rely on this, kmemcheck might barf on us. > > Give me a break. Sure. End of discussion. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Mon, 02 Apr 2012 05:58:25 +0200 > On Sun, 2012-04-01 at 23:55 -0400, David Miller wrote: >> From: Eric Dumazet <eric.dumazet@gmail.com> >> Date: Mon, 02 Apr 2012 05:45:45 +0200 >> >> > Usually not a problem since we have many bytes after our head. >> >> We always have bytes after the head, it's guarenteed, and whether it's >> garbage bytes or skb_shared_info() it simply doesn't matter. > > Then you have to add a kmemcheck_something() to make this clear and > avoid possible warnings. That's perfectly fine and would document the situation. And we can add a similar annotation to the two other nearly identical pieces of code in net/netfilter/nf_conntrack_proto_tcp.c -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 2, 2012 at 11:53 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > No we cant rely on this, kmemcheck might barf on us. > > Your patch (and the netfilter one) is fine. > > Got it. Thanks. FYI, the tcp options are copied to the stack before being parsed.
On Mon, 2012-04-02 at 12:47 +0800, Changli Gao wrote: > Got it. Thanks. FYI, the tcp options are copied to the stack before > being parsed. > What do you mean ? code looks like : const struct tcphdr *th = tcp_hdr(skb); int length = (th->doff * 4) - sizeof(struct tcphdr); ptr = (const unsigned char *)(th + 1); Therefore ptr points somewhere in skb->head ... -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 2, 2012 at 12:54 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Mon, 2012-04-02 at 12:47 +0800, Changli Gao wrote: > >> Got it. Thanks. FYI, the tcp options are copied to the stack before >> being parsed. >> > > What do you mean ? > > code looks like : > > const struct tcphdr *th = tcp_hdr(skb); > int length = (th->doff * 4) - sizeof(struct tcphdr); > > ptr = (const unsigned char *)(th + 1); > > > > Therefore ptr points somewhere in skb->head ... > > > Oh, sorry. I forgot to add the condition when I wrote it down. I mean the code in netfilter. unsigned char buff[(15 * 4) - sizeof(struct tcphdr)]; const unsigned char *ptr; int length = (tcph->doff*4) - sizeof(struct tcphdr); if (!length) return; ptr = skb_header_pointer(skb, dataoff + sizeof(struct tcphdr), length, buff); BUG_ON(ptr == NULL);
On Mon, 2012-04-02 at 14:27 +0800, Changli Gao wrote: > On Mon, Apr 2, 2012 at 12:54 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > On Mon, 2012-04-02 at 12:47 +0800, Changli Gao wrote: > > > >> Got it. Thanks. FYI, the tcp options are copied to the stack before > >> being parsed. > >> > > > > What do you mean ? > > > > code looks like : > > > > const struct tcphdr *th = tcp_hdr(skb); > > int length = (th->doff * 4) - sizeof(struct tcphdr); > > > > ptr = (const unsigned char *)(th + 1); > > > > > > > > Therefore ptr points somewhere in skb->head ... > > > > > > > > Oh, sorry. I forgot to add the condition when I wrote it down. I mean > the code in netfilter. > > unsigned char buff[(15 * 4) - sizeof(struct tcphdr)]; > const unsigned char *ptr; > int length = (tcph->doff*4) - sizeof(struct tcphdr); > > if (!length) > return; > > ptr = skb_header_pointer(skb, dataoff + sizeof(struct tcphdr), > length, buff); > BUG_ON(ptr == NULL); > Doesnt really save us, skb_header_pointer() copies only if block not directly and fully accessible in skb->head So if skb->head contains exactly the tcp options, we still can read one uninit byte. So potential problem in netfilter too. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index e886e2f..5099f08 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -3845,6 +3845,8 @@ void tcp_parse_options(const struct sk_buff *skb, struct tcp_options_received *o length--; continue; default: + if (length < 2) + return; opsize = *ptr++; if (opsize < 2) /* "silly options" */ return;
We should check the length of the data before dereferencing it when parsing the TCP options. Signed-off-by: Changli Gao <xiaosuo@gmail.com> --- net/ipv4/tcp_input.c | 2 ++ 1 file changed, 2 insertions(+) -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html