| Message ID | 20220420235659.830155EC021C@us226.sjc.aristanetworks.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | tcp: md5: incorrect tcp_header_len for incoming connections | expand |
On Wed, Apr 20, 2022 at 4:57 PM Francesco Ruggeri <fruggeri@arista.com> wrote: > > In tcp_create_openreq_child we adjust tcp_header_len for md5 using the > remote address in newsk. But that address is still 0 in newsk at this > point, and it is only set later by the callers (tcp_v[46]_syn_recv_sock). > Use the address from the request socket instead. > Nice catch. This seems like a day-0 bug, right ? Do you agree on adding Fixes: cfb6eeb4c860 ("[TCP]: MD5 Signature Option (RFC2385) support.") Thanks. > Signed-off-by: Francesco Ruggeri <fruggeri@arista.com> > --- > net/ipv4/tcp_minisocks.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c > index 6366df7aaf2a..6854bb1fb32b 100644 > --- a/net/ipv4/tcp_minisocks.c > +++ b/net/ipv4/tcp_minisocks.c > @@ -531,7 +531,7 @@ struct sock *tcp_create_openreq_child(const struct sock *sk, > newtp->tsoffset = treq->ts_off; > #ifdef CONFIG_TCP_MD5SIG > newtp->md5sig_info = NULL; /*XXX*/ > - if (newtp->af_specific->md5_lookup(sk, newsk)) > + if (treq->af_specific->req_md5_lookup(sk, req_to_sk(req))) > newtp->tcp_header_len += TCPOLEN_MD5SIG_ALIGNED; > #endif > if (skb->len >= TCP_MSS_DEFAULT + newtp->tcp_header_len) > -- > 2.28.0 > >
On Wed, Apr 20, 2022 at 5:20 PM Eric Dumazet <edumazet@google.com> wrote: > On Wed, Apr 20, 2022 at 4:57 PM Francesco Ruggeri <fruggeri@arista.com> wrote: > This seems like a day-0 bug, right ? > > Do you agree on adding > > Fixes: cfb6eeb4c860 ("[TCP]: MD5 Signature Option (RFC2385) support.") > > Thanks. > I also think it is a day-0 bug. Should I resubmit with "Fixes:" ?
On Wed, Apr 20, 2022 at 5:32 PM Francesco Ruggeri <fruggeri@arista.com> wrote: > > On Wed, Apr 20, 2022 at 5:20 PM Eric Dumazet <edumazet@google.com> wrote: > > On Wed, Apr 20, 2022 at 4:57 PM Francesco Ruggeri <fruggeri@arista.com> wrote: > > This seems like a day-0 bug, right ? > > > > Do you agree on adding > > > > Fixes: cfb6eeb4c860 ("[TCP]: MD5 Signature Option (RFC2385) support.") > > > > Thanks. > > > I also think it is a day-0 bug. Should I resubmit with "Fixes:" ? I think so. It will make things a bit easier for network maintainers, because I do not think patchwork catches Fixes: tags added during code review. Also please add the net prefix, as in [PATCH v2 net] title (look at the warning at https://patchwork.kernel.org/project/netdevbpf/patch/20220420235659.830155EC021C@us226.sjc.aristanetworks.com/ ) Thanks.
diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c index 6366df7aaf2a..6854bb1fb32b 100644 --- a/net/ipv4/tcp_minisocks.c +++ b/net/ipv4/tcp_minisocks.c @@ -531,7 +531,7 @@ struct sock *tcp_create_openreq_child(const struct sock *sk, newtp->tsoffset = treq->ts_off; #ifdef CONFIG_TCP_MD5SIG newtp->md5sig_info = NULL; /*XXX*/ - if (newtp->af_specific->md5_lookup(sk, newsk)) + if (treq->af_specific->req_md5_lookup(sk, req_to_sk(req))) newtp->tcp_header_len += TCPOLEN_MD5SIG_ALIGNED; #endif if (skb->len >= TCP_MSS_DEFAULT + newtp->tcp_header_len)
In tcp_create_openreq_child we adjust tcp_header_len for md5 using the remote address in newsk. But that address is still 0 in newsk at this point, and it is only set later by the callers (tcp_v[46]_syn_recv_sock). Use the address from the request socket instead. Signed-off-by: Francesco Ruggeri <fruggeri@arista.com> --- net/ipv4/tcp_minisocks.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)