mptcp.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH net v2 0/3] Fix out of bounds when parsing TCP options
@ 2021-06-10 16:40 Maxim Mikityanskiy
  2021-06-10 16:40 ` [PATCH net v2 1/3] netfilter: synproxy: " Maxim Mikityanskiy
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Maxim Mikityanskiy @ 2021-06-10 16:40 UTC (permalink / raw)
  To: Mat Martineau, Matthieu Baerts, Jakub Kicinski, David S. Miller,
	Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal,
	Toke Høiland-Jørgensen, Jamal Hadi Salim, Cong Wang,
	Jiri Pirko, Patrick McHardy, Jesper Dangaard Brouer, Paolo Abeni,
	Christoph Paasch, Peter Krystad
  Cc: Young Xiao, netdev, mptcp, Maxim Mikityanskiy

This series fixes out-of-bounds access in various places in the kernel
where parsing of TCP options takes place. Fortunately, many more
occurrences don't have this bug.

v2 changes:

synproxy: Added an early return when length < 0 to avoid calling
skb_header_pointer with negative length.

sch_cake: Added doff validation to avoid parsing garbage.

Maxim Mikityanskiy (3):
  netfilter: synproxy: Fix out of bounds when parsing TCP options
  mptcp: Fix out of bounds when parsing TCP options
  sch_cake: Fix out of bounds when parsing TCP options and header

 net/mptcp/options.c              | 2 ++
 net/netfilter/nf_synproxy_core.c | 5 +++++
 net/sched/sch_cake.c             | 6 +++++-
 3 files changed, 12 insertions(+), 1 deletion(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH net v2 1/3] netfilter: synproxy: Fix out of bounds when parsing TCP options
  2021-06-10 16:40 [PATCH net v2 0/3] Fix out of bounds when parsing TCP options Maxim Mikityanskiy
@ 2021-06-10 16:40 ` Maxim Mikityanskiy
  2021-06-10 16:43   ` Florian Westphal
  2021-06-10 16:40 ` [PATCH net v2 2/3] mptcp: " Maxim Mikityanskiy
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Maxim Mikityanskiy @ 2021-06-10 16:40 UTC (permalink / raw)
  To: Mat Martineau, Matthieu Baerts, Jakub Kicinski, David S. Miller,
	Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal,
	Toke Høiland-Jørgensen, Jamal Hadi Salim, Cong Wang,
	Jiri Pirko, Patrick McHardy, Jesper Dangaard Brouer, Paolo Abeni,
	Christoph Paasch, Peter Krystad
  Cc: Young Xiao, netdev, mptcp, Maxim Mikityanskiy

The TCP option parser in synproxy (synproxy_parse_options) could read
one byte out of bounds. When the length is 1, the execution flow gets
into the loop, reads one byte of the opcode, and if the opcode is
neither TCPOPT_EOL nor TCPOPT_NOP, it reads one more byte, which exceeds
the length of 1.

This fix is inspired by commit 9609dad263f8 ("ipv4: tcp_input: fix stack
out of bounds when parsing TCP options.").

v2 changes:

Added an early return when length < 0 to avoid calling
skb_header_pointer with negative length.

Cc: Young Xiao <92siuyang@gmail.com>
Fixes: 48b1de4c110a ("netfilter: add SYNPROXY core/target")
Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
---
 net/netfilter/nf_synproxy_core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/netfilter/nf_synproxy_core.c b/net/netfilter/nf_synproxy_core.c
index b100c04a0e43..3d6d49420db8 100644
--- a/net/netfilter/nf_synproxy_core.c
+++ b/net/netfilter/nf_synproxy_core.c
@@ -31,6 +31,9 @@ synproxy_parse_options(const struct sk_buff *skb, unsigned int doff,
 	int length = (th->doff * 4) - sizeof(*th);
 	u8 buf[40], *ptr;
 
+	if (unlikely(length < 0))
+		return false;
+
 	ptr = skb_header_pointer(skb, doff + sizeof(*th), length, buf);
 	if (ptr == NULL)
 		return false;
@@ -47,6 +50,8 @@ synproxy_parse_options(const struct sk_buff *skb, unsigned int doff,
 			length--;
 			continue;
 		default:
+			if (length < 2)
+				return true;
 			opsize = *ptr++;
 			if (opsize < 2)
 				return true;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH net v2 2/3] mptcp: Fix out of bounds when parsing TCP options
  2021-06-10 16:40 [PATCH net v2 0/3] Fix out of bounds when parsing TCP options Maxim Mikityanskiy
  2021-06-10 16:40 ` [PATCH net v2 1/3] netfilter: synproxy: " Maxim Mikityanskiy
@ 2021-06-10 16:40 ` Maxim Mikityanskiy
  2021-06-10 21:03   ` Mat Martineau
  2021-06-10 16:40 ` [PATCH net v2 3/3] sch_cake: Fix out of bounds when parsing TCP options and header Maxim Mikityanskiy
  2021-06-10 21:50 ` [PATCH net v2 0/3] Fix out of bounds when parsing TCP options patchwork-bot+netdevbpf
  3 siblings, 1 reply; 9+ messages in thread
From: Maxim Mikityanskiy @ 2021-06-10 16:40 UTC (permalink / raw)
  To: Mat Martineau, Matthieu Baerts, Jakub Kicinski, David S. Miller,
	Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal,
	Toke Høiland-Jørgensen, Jamal Hadi Salim, Cong Wang,
	Jiri Pirko, Patrick McHardy, Jesper Dangaard Brouer, Paolo Abeni,
	Christoph Paasch, Peter Krystad
  Cc: Young Xiao, netdev, mptcp, Maxim Mikityanskiy

The TCP option parser in mptcp (mptcp_get_options) could read one byte
out of bounds. When the length is 1, the execution flow gets into the
loop, reads one byte of the opcode, and if the opcode is neither
TCPOPT_EOL nor TCPOPT_NOP, it reads one more byte, which exceeds the
length of 1.

This fix is inspired by commit 9609dad263f8 ("ipv4: tcp_input: fix stack
out of bounds when parsing TCP options.").

Cc: Young Xiao <92siuyang@gmail.com>
Fixes: cec37a6e41aa ("mptcp: Handle MP_CAPABLE options for outgoing connections")
Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
---
 net/mptcp/options.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 6b825fb3fa83..9b263f27ce9b 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -356,6 +356,8 @@ void mptcp_get_options(const struct sk_buff *skb,
 			length--;
 			continue;
 		default:
+			if (length < 2)
+				return;
 			opsize = *ptr++;
 			if (opsize < 2) /* "silly options" */
 				return;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH net v2 3/3] sch_cake: Fix out of bounds when parsing TCP options and header
  2021-06-10 16:40 [PATCH net v2 0/3] Fix out of bounds when parsing TCP options Maxim Mikityanskiy
  2021-06-10 16:40 ` [PATCH net v2 1/3] netfilter: synproxy: " Maxim Mikityanskiy
  2021-06-10 16:40 ` [PATCH net v2 2/3] mptcp: " Maxim Mikityanskiy
@ 2021-06-10 16:40 ` Maxim Mikityanskiy
  2021-06-10 21:50 ` [PATCH net v2 0/3] Fix out of bounds when parsing TCP options patchwork-bot+netdevbpf
  3 siblings, 0 replies; 9+ messages in thread
From: Maxim Mikityanskiy @ 2021-06-10 16:40 UTC (permalink / raw)
  To: Mat Martineau, Matthieu Baerts, Jakub Kicinski, David S. Miller,
	Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal,
	Toke Høiland-Jørgensen, Jamal Hadi Salim, Cong Wang,
	Jiri Pirko, Patrick McHardy, Jesper Dangaard Brouer, Paolo Abeni,
	Christoph Paasch, Peter Krystad
  Cc: Young Xiao, netdev, mptcp, Maxim Mikityanskiy

The TCP option parser in cake qdisc (cake_get_tcpopt and
cake_tcph_may_drop) could read one byte out of bounds. When the length
is 1, the execution flow gets into the loop, reads one byte of the
opcode, and if the opcode is neither TCPOPT_EOL nor TCPOPT_NOP, it reads
one more byte, which exceeds the length of 1.

This fix is inspired by commit 9609dad263f8 ("ipv4: tcp_input: fix stack
out of bounds when parsing TCP options.").

v2 changes:

Added doff validation in cake_get_tcphdr to avoid parsing garbage as TCP
header. Although it wasn't strictly an out-of-bounds access (memory was
allocated), garbage values could be read where CAKE expected the TCP
header if doff was smaller than 5.

Cc: Young Xiao <92siuyang@gmail.com>
Fixes: 8b7138814f29 ("sch_cake: Add optional ACK filter")
Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
 net/sched/sch_cake.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index 7d37638ee1c7..5c15968b5155 100644
--- a/net/sched/sch_cake.c
+++ b/net/sched/sch_cake.c
@@ -943,7 +943,7 @@ static struct tcphdr *cake_get_tcphdr(const struct sk_buff *skb,
 	}
 
 	tcph = skb_header_pointer(skb, offset, sizeof(_tcph), &_tcph);
-	if (!tcph)
+	if (!tcph || tcph->doff < 5)
 		return NULL;
 
 	return skb_header_pointer(skb, offset,
@@ -967,6 +967,8 @@ static const void *cake_get_tcpopt(const struct tcphdr *tcph,
 			length--;
 			continue;
 		}
+		if (length < 2)
+			break;
 		opsize = *ptr++;
 		if (opsize < 2 || opsize > length)
 			break;
@@ -1104,6 +1106,8 @@ static bool cake_tcph_may_drop(const struct tcphdr *tcph,
 			length--;
 			continue;
 		}
+		if (length < 2)
+			break;
 		opsize = *ptr++;
 		if (opsize < 2 || opsize > length)
 			break;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net v2 1/3] netfilter: synproxy: Fix out of bounds when parsing TCP options
  2021-06-10 16:40 ` [PATCH net v2 1/3] netfilter: synproxy: " Maxim Mikityanskiy
@ 2021-06-10 16:43   ` Florian Westphal
  0 siblings, 0 replies; 9+ messages in thread
From: Florian Westphal @ 2021-06-10 16:43 UTC (permalink / raw)
  To: Maxim Mikityanskiy
  Cc: Mat Martineau, Matthieu Baerts, Jakub Kicinski, David S. Miller,
	Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal,
	Toke Høiland-Jørgensen, Jamal Hadi Salim, Cong Wang,
	Jiri Pirko, Patrick McHardy, Jesper Dangaard Brouer, Paolo Abeni,
	Christoph Paasch, Peter Krystad, Young Xiao, netdev, mptcp

Maxim Mikityanskiy <maximmi@nvidia.com> wrote:
> The TCP option parser in synproxy (synproxy_parse_options) could read
> one byte out of bounds. When the length is 1, the execution flow gets
> into the loop, reads one byte of the opcode, and if the opcode is
> neither TCPOPT_EOL nor TCPOPT_NOP, it reads one more byte, which exceeds
> the length of 1.
> 
> This fix is inspired by commit 9609dad263f8 ("ipv4: tcp_input: fix stack
> out of bounds when parsing TCP options.").

Reviewed-by: Florian Westphal <fw@strlen.de>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net v2 2/3] mptcp: Fix out of bounds when parsing TCP options
  2021-06-10 16:40 ` [PATCH net v2 2/3] mptcp: " Maxim Mikityanskiy
@ 2021-06-10 21:03   ` Mat Martineau
  2021-06-10 21:09     ` Mat Martineau
  2021-06-11 14:30     ` Matthieu Baerts
  0 siblings, 2 replies; 9+ messages in thread
From: Mat Martineau @ 2021-06-10 21:03 UTC (permalink / raw)
  To: Matthieu Baerts; +Cc: Maxim Mikityanskiy, mptcp

On Thu, 10 Jun 2021, Maxim Mikityanskiy wrote:

> The TCP option parser in mptcp (mptcp_get_options) could read one byte
> out of bounds. When the length is 1, the execution flow gets into the
> loop, reads one byte of the opcode, and if the opcode is neither
> TCPOPT_EOL nor TCPOPT_NOP, it reads one more byte, which exceeds the
> length of 1.
>
> This fix is inspired by commit 9609dad263f8 ("ipv4: tcp_input: fix stack
> out of bounds when parsing TCP options.").
>
> Cc: Young Xiao <92siuyang@gmail.com>
> Fixes: cec37a6e41aa ("mptcp: Handle MP_CAPABLE options for outgoing connections")
> Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> ---
> net/mptcp/options.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 6b825fb3fa83..9b263f27ce9b 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -356,6 +356,8 @@ void mptcp_get_options(const struct sk_buff *skb,
> 			length--;
> 			continue;
> 		default:
> +			if (length < 2)
> +				return;
> 			opsize = *ptr++;
> 			if (opsize < 2) /* "silly options" */
> 				return;
> -- 
> 2.25.1

Matthieu -

Could you apply this in mptcp_net-next so it's easier to track when the 
patch finds its way to net-next?

(MPTCP patchwork status set to "queued")

Thanks!

--
Mat Martineau
Intel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net v2 2/3] mptcp: Fix out of bounds when parsing TCP options
  2021-06-10 21:03   ` Mat Martineau
@ 2021-06-10 21:09     ` Mat Martineau
  2021-06-11 14:30     ` Matthieu Baerts
  1 sibling, 0 replies; 9+ messages in thread
From: Mat Martineau @ 2021-06-10 21:09 UTC (permalink / raw)
  To: Maxim Mikityanskiy; +Cc: Matthieu Baerts, mptcp

On Thu, 10 Jun 2021, Mat Martineau wrote:

> On Thu, 10 Jun 2021, Maxim Mikityanskiy wrote:
>
>> The TCP option parser in mptcp (mptcp_get_options) could read one byte
>> out of bounds. When the length is 1, the execution flow gets into the
>> loop, reads one byte of the opcode, and if the opcode is neither
>> TCPOPT_EOL nor TCPOPT_NOP, it reads one more byte, which exceeds the
>> length of 1.
>> 
>> This fix is inspired by commit 9609dad263f8 ("ipv4: tcp_input: fix stack
>> out of bounds when parsing TCP options.").
>> 
>> Cc: Young Xiao <92siuyang@gmail.com>
>> Fixes: cec37a6e41aa ("mptcp: Handle MP_CAPABLE options for outgoing 
>> connections")
>> Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
>> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
>> ---
>> net/mptcp/options.c | 2 ++
>> 1 file changed, 2 insertions(+)
>> 
>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>> index 6b825fb3fa83..9b263f27ce9b 100644
>> --- a/net/mptcp/options.c
>> +++ b/net/mptcp/options.c
>> @@ -356,6 +356,8 @@ void mptcp_get_options(const struct sk_buff *skb,
>> 			length--;
>> 			continue;
>> 		default:
>> +			if (length < 2)
>> +				return;
>> 			opsize = *ptr++;
>> 			if (opsize < 2) /* "silly options" */
>> 				return;
>> -- 
>> 2.25.1
>
> Matthieu -
>
> Could you apply this in mptcp_net-next so it's easier to track when the patch 
> finds its way to net-next?
>
> (MPTCP patchwork status set to "queued")

And to clarify for Maxim: we have a separate MPTCP git tree where we stage 
MPTCP changes before submitting to net- or net-next. For your patch set 
that has related changes across a few networking subsystems, sending 
directly to netdev is the best thing to do.

We do have other queued changes for netdev so I think it would work well 
to include this patch in our tree now, and it will automatically disappear 
once it is fully merged upstream. Sometimes it takes a couple of weeks for 
the net/master branch to get merged in to net-next/master.

--
Mat Martineau
Intel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net v2 0/3] Fix out of bounds when parsing TCP options
  2021-06-10 16:40 [PATCH net v2 0/3] Fix out of bounds when parsing TCP options Maxim Mikityanskiy
                   ` (2 preceding siblings ...)
  2021-06-10 16:40 ` [PATCH net v2 3/3] sch_cake: Fix out of bounds when parsing TCP options and header Maxim Mikityanskiy
@ 2021-06-10 21:50 ` patchwork-bot+netdevbpf
  3 siblings, 0 replies; 9+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-06-10 21:50 UTC (permalink / raw)
  To: Maxim Mikityanskiy
  Cc: mathew.j.martineau, matthieu.baerts, kuba, davem, pablo, kadlec,
	fw, toke, jhs, xiyou.wangcong, jiri, kaber, brouer, pabeni,
	cpaasch, peter.krystad, 92siuyang, netdev, mptcp

Hello:

This series was applied to netdev/net.git (refs/heads/master):

On Thu, 10 Jun 2021 19:40:28 +0300 you wrote:
> This series fixes out-of-bounds access in various places in the kernel
> where parsing of TCP options takes place. Fortunately, many more
> occurrences don't have this bug.
> 
> v2 changes:
> 
> synproxy: Added an early return when length < 0 to avoid calling
> skb_header_pointer with negative length.
> 
> [...]

Here is the summary with links:
  - [net,v2,1/3] netfilter: synproxy: Fix out of bounds when parsing TCP options
    https://git.kernel.org/netdev/net/c/5fc177ab7594
  - [net,v2,2/3] mptcp: Fix out of bounds when parsing TCP options
    https://git.kernel.org/netdev/net/c/07718be26568
  - [net,v2,3/3] sch_cake: Fix out of bounds when parsing TCP options and header
    https://git.kernel.org/netdev/net/c/ba91c49dedbd

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net v2 2/3] mptcp: Fix out of bounds when parsing TCP options
  2021-06-10 21:03   ` Mat Martineau
  2021-06-10 21:09     ` Mat Martineau
@ 2021-06-11 14:30     ` Matthieu Baerts
  1 sibling, 0 replies; 9+ messages in thread
From: Matthieu Baerts @ 2021-06-11 14:30 UTC (permalink / raw)
  To: Mat Martineau; +Cc: Maxim Mikityanskiy, mptcp

Hi Mat,

On 10/06/2021 23:03, Mat Martineau wrote:
> On Thu, 10 Jun 2021, Maxim Mikityanskiy wrote:
> 
>> The TCP option parser in mptcp (mptcp_get_options) could read one byte
>> out of bounds. When the length is 1, the execution flow gets into the
>> loop, reads one byte of the opcode, and if the opcode is neither
>> TCPOPT_EOL nor TCPOPT_NOP, it reads one more byte, which exceeds the
>> length of 1.
>>
>> This fix is inspired by commit 9609dad263f8 ("ipv4: tcp_input: fix stack
>> out of bounds when parsing TCP options.").
>>
>> Cc: Young Xiao <92siuyang@gmail.com>
>> Fixes: cec37a6e41aa ("mptcp: Handle MP_CAPABLE options for outgoing
>> connections")
>> Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
>> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> 
> Matthieu -
> 
> Could you apply this in mptcp_net-next so it's easier to track when the
> patch finds its way to net-next?

Sure, good idea!
Thank you for the patch and the review!

Just applied in our tree:

- fdd037a5564a: mptcp: Fix out of bounds when parsing TCP options
- Results: 083034593cec..1ba3eb627299

Builds and tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20210611T142646
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export/20210611T142646

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-06-11 14:30 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-10 16:40 [PATCH net v2 0/3] Fix out of bounds when parsing TCP options Maxim Mikityanskiy
2021-06-10 16:40 ` [PATCH net v2 1/3] netfilter: synproxy: " Maxim Mikityanskiy
2021-06-10 16:43   ` Florian Westphal
2021-06-10 16:40 ` [PATCH net v2 2/3] mptcp: " Maxim Mikityanskiy
2021-06-10 21:03   ` Mat Martineau
2021-06-10 21:09     ` Mat Martineau
2021-06-11 14:30     ` Matthieu Baerts
2021-06-10 16:40 ` [PATCH net v2 3/3] sch_cake: Fix out of bounds when parsing TCP options and header Maxim Mikityanskiy
2021-06-10 21:50 ` [PATCH net v2 0/3] Fix out of bounds when parsing TCP options patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).