Re: [PATCH net 3/3] sch_cake: Fix out of bounds when parsing TCP options

["Toke Høiland-Jørgensen" <toke@toke.dk>]

Maxim Mikityanskiy <maximmi@nvidia.com> writes:

> On 2021-06-10 00:51, Toke Høiland-Jørgensen wrote:
>> Maxim Mikityanskiy <maximmi@nvidia.com> writes:
>> 
>>> 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.").
>>>
>>> Cc: Young Xiao <92siuyang@gmail.com>
>>> Fixes: 8b7138814f29 ("sch_cake: Add optional ACK filter")
>>> Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
>> 
>> Thanks for fixing this!
>> 
>> Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
>> 
>
> Could you also review whether Florian's comment on patch 1 is relevant 
> to this patch too? I have concerns about cake_get_tcphdr, which returns 
> `skb_header_pointer(skb, offset, min(__tcp_hdrlen(tcph), bufsize), 
> buf)`. Although I don't see a way for it to get out of bounds (it will 
> read garbage instead of TCP header in the worst case), such code doesn't 
> look robust.
>
> It's not possible for it to get out of bounds, because there is a call 
> to skb_header_pointer above with sizeof(_tcph), which ensures that the 
> SKB has at least 20 bytes after the beginning of the TCP header, which 
> means that the second skb_header_pointer will either point to SKB (where 
> we have at least 20 bytes) or to buf (which is allocated by the caller, 
> so the caller shouldn't overflow its own buffer).
>
> On the other hand, parsing garbage doesn't look like a valid behavior 
> compared to dropping/ignoring/whatever-cake-does-with-bad-packets, so we 
> may want to handle it, for example:
>
>           return skb_header_pointer(skb, offset,
> -                                  min(__tcp_hdrlen(tcph), bufsize), buf);
> +                                  min(max(sizeof(struct tcphdr), 
> __tcp_hdrlen(tcph)), bufsize), buf);
>
> What do you think? Or did I just miss some early check for doff?

No, I think your analysis is correct: It won't lead to any out-of-bounds
reads, but I suppose we could end up trying to parse garbage. However,
if we do get a packet that sets doff to an invalid value, and we try to
parse it, we're essentially parsing garbage anyway. So I think the fix
should rather be something like:

diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index 7d37638ee1c7..d312d75ab698 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,

> (I realize it's egress path and the packets produced by the system 
> itself are unlikely to have bad doff, but it's not impossible, for 
> example, with AF_PACKET, BPF hooks in tc, etc.)

Most CAKE deployments primarily handles forwarded packets, and I suppose
malformed TCP packets could make it through the forwarding path as
well...

-Toke