| Message ID | 1559117459-27353-1-git-send-email-92siuyang@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | ipv4: tcp_input: fix stack out of bounds when parsing TCP options. | expand |
On Wed, May 29, 2019 at 1:10 AM Young Xiao <92siuyang@gmail.com> wrote: > > The TCP option parsing routines in tcp_parse_options function could > read one byte out of the buffer of the TCP options. > > 1 while (length > 0) { > 2 int opcode = *ptr++; > 3 int opsize; > 4 > 5 switch (opcode) { > 6 case TCPOPT_EOL: > 7 return; > 8 case TCPOPT_NOP: /* Ref: RFC 793 section 3.1 */ > 9 length--; > 10 continue; > 11 default: > 12 opsize = *ptr++; //out of bound access > > If length = 1, then there is an access in line2. > And another access is occurred in line 12. > This would lead to out-of-bound access. > > Therefore, in the patch we check that the available data length is > larger enough to pase both TCP option code and size. > > Signed-off-by: Young Xiao <92siuyang@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 20f6fac..9775825 100644 > --- a/net/ipv4/tcp_input.c > +++ b/net/ipv4/tcp_input.c > @@ -3791,6 +3791,8 @@ void tcp_parse_options(const struct net *net, > length--; > continue; > default: > + if (length < 2) > + return; > opsize = *ptr++; > if (opsize < 2) /* "silly options" */ > return; In practice we are good, since we have at least 320 bytes of room there, and the test done later catches silly options. if (opsize < 2) /* "silly options" */ return; if (opsize > length) /* remember, opsize >= 2 here */ return; /* don't parse partial options */ I guess adding yet another conditional will make this code obviously correct for all eyes and various tools. Thanks. Signed-off-by: Eric Dumazet <edumazet@google.com>
Indeed, condition opsize < 2 and opsize > length can deduce that length >= 2. However, before the condition (if opsize < 2), there may be one-byte out-of-bound access in line 12. I'm not sure whether I have put it very clearly. On Wed, May 29, 2019 at 10:20 PM Eric Dumazet <edumazet@google.com> wrote: > > On Wed, May 29, 2019 at 1:10 AM Young Xiao <92siuyang@gmail.com> wrote: > > > > The TCP option parsing routines in tcp_parse_options function could > > read one byte out of the buffer of the TCP options. > > > > 1 while (length > 0) { > > 2 int opcode = *ptr++; > > 3 int opsize; > > 4 > > 5 switch (opcode) { > > 6 case TCPOPT_EOL: > > 7 return; > > 8 case TCPOPT_NOP: /* Ref: RFC 793 section 3.1 */ > > 9 length--; > > 10 continue; > > 11 default: > > 12 opsize = *ptr++; //out of bound access > > > > If length = 1, then there is an access in line2. > > And another access is occurred in line 12. > > This would lead to out-of-bound access. > > > > Therefore, in the patch we check that the available data length is > > larger enough to pase both TCP option code and size. > > > > Signed-off-by: Young Xiao <92siuyang@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 20f6fac..9775825 100644 > > --- a/net/ipv4/tcp_input.c > > +++ b/net/ipv4/tcp_input.c > > @@ -3791,6 +3791,8 @@ void tcp_parse_options(const struct net *net, > > length--; > > continue; > > default: > > + if (length < 2) > > + return; > > opsize = *ptr++; > > if (opsize < 2) /* "silly options" */ > > return; > > In practice we are good, since we have at least 320 bytes of room there, > and the test done later catches silly options. > > if (opsize < 2) /* "silly options" */ > return; > if (opsize > length) /* remember, opsize >= 2 here */ > return; /* don't parse partial options */ > > I guess adding yet another conditional will make this code obviously > correct for all eyes > and various tools. > > Thanks. > > Signed-off-by: Eric Dumazet <edumazet@google.com>
On Wed, May 29, 2019 at 8:11 AM Yang Xiao <92siuyang@gmail.com> wrote: > > Indeed, condition opsize < 2 and opsize > length can deduce that length >= 2. > However, before the condition (if opsize < 2), there may be one-byte > out-of-bound access in line 12. > I'm not sure whether I have put it very clearly. Maybe I should have been clear about the 320 bytes we have at the end of skb->head This is the struct skb_shared_info So reading one byte, 'out-of-bound' here is harmless. Whatever value is read, we will return early without ever looking at a following byte. > > On Wed, May 29, 2019 at 10:20 PM Eric Dumazet <edumazet@google.com> wrote: > > > > On Wed, May 29, 2019 at 1:10 AM Young Xiao <92siuyang@gmail.com> wrote: > > > > > > The TCP option parsing routines in tcp_parse_options function could > > > read one byte out of the buffer of the TCP options. > > > > > > 1 while (length > 0) { > > > 2 int opcode = *ptr++; > > > 3 int opsize; > > > 4 > > > 5 switch (opcode) { > > > 6 case TCPOPT_EOL: > > > 7 return; > > > 8 case TCPOPT_NOP: /* Ref: RFC 793 section 3.1 */ > > > 9 length--; > > > 10 continue; > > > 11 default: > > > 12 opsize = *ptr++; //out of bound access > > > > > > If length = 1, then there is an access in line2. > > > And another access is occurred in line 12. > > > This would lead to out-of-bound access. > > > > > > Therefore, in the patch we check that the available data length is > > > larger enough to pase both TCP option code and size. > > > > > > Signed-off-by: Young Xiao <92siuyang@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 20f6fac..9775825 100644 > > > --- a/net/ipv4/tcp_input.c > > > +++ b/net/ipv4/tcp_input.c > > > @@ -3791,6 +3791,8 @@ void tcp_parse_options(const struct net *net, > > > length--; > > > continue; > > > default: > > > + if (length < 2) > > > + return; > > > opsize = *ptr++; > > > if (opsize < 2) /* "silly options" */ > > > return; > > > > In practice we are good, since we have at least 320 bytes of room there, > > and the test done later catches silly options. > > > > if (opsize < 2) /* "silly options" */ > > return; > > if (opsize > length) /* remember, opsize >= 2 here */ > > return; /* don't parse partial options */ > > > > I guess adding yet another conditional will make this code obviously > > correct for all eyes > > and various tools. > > > > Thanks. > > > > Signed-off-by: Eric Dumazet <edumazet@google.com> > > > > -- > Best regards! > > Young > -----------------------------------------------------------
From: Young Xiao <92siuyang@gmail.com> Date: Wed, 29 May 2019 16:10:59 +0800 > The TCP option parsing routines in tcp_parse_options function could > read one byte out of the buffer of the TCP options. > > 1 while (length > 0) { > 2 int opcode = *ptr++; > 3 int opsize; > 4 > 5 switch (opcode) { > 6 case TCPOPT_EOL: > 7 return; > 8 case TCPOPT_NOP: /* Ref: RFC 793 section 3.1 */ > 9 length--; > 10 continue; > 11 default: > 12 opsize = *ptr++; //out of bound access > > If length = 1, then there is an access in line2. > And another access is occurred in line 12. > This would lead to out-of-bound access. > > Therefore, in the patch we check that the available data length is > larger enough to pase both TCP option code and size. > > Signed-off-by: Young Xiao <92siuyang@gmail.com> Applied.
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index 20f6fac..9775825 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -3791,6 +3791,8 @@ void tcp_parse_options(const struct net *net, length--; continue; default: + if (length < 2) + return; opsize = *ptr++; if (opsize < 2) /* "silly options" */ return;
The TCP option parsing routines in tcp_parse_options function could read one byte out of the buffer of the TCP options. 1 while (length > 0) { 2 int opcode = *ptr++; 3 int opsize; 4 5 switch (opcode) { 6 case TCPOPT_EOL: 7 return; 8 case TCPOPT_NOP: /* Ref: RFC 793 section 3.1 */ 9 length--; 10 continue; 11 default: 12 opsize = *ptr++; //out of bound access If length = 1, then there is an access in line2. And another access is occurred in line 12. This would lead to out-of-bound access. Therefore, in the patch we check that the available data length is larger enough to pase both TCP option code and size. Signed-off-by: Young Xiao <92siuyang@gmail.com> --- net/ipv4/tcp_input.c | 2 ++ 1 file changed, 2 insertions(+)