| Message ID | 20231128205749.312759-6-dima@arista.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v3,1/7] Documentation/tcp: Fix an obvious typo | expand |
On 11/28/23 1:57 PM, Dmitry Safonov wrote: > If the connection was established, don't allow adding TCP-AO keys that > don't match the peer. Currently, there are checks for ip-address > matching, but L3 index check is missing. Add it to restrict userspace you say L3 index check is missing - add it. yet ... > shooting itself somewhere. > > Fixes: 248411b8cb89 ("net/tcp: Wire up l3index to TCP-AO") > Signed-off-by: Dmitry Safonov <dima@arista.com> > --- > net/ipv4/tcp_ao.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/net/ipv4/tcp_ao.c b/net/ipv4/tcp_ao.c > index bf41be6d4721..2d000e275ce7 100644 > --- a/net/ipv4/tcp_ao.c > +++ b/net/ipv4/tcp_ao.c > @@ -1608,6 +1608,9 @@ static int tcp_ao_add_cmd(struct sock *sk, unsigned short int family, > if (!dev || !l3index) > return -EINVAL; > > + if (!((1 << sk->sk_state) & (TCPF_LISTEN | TCPF_CLOSE))) > + return -EINVAL; ... this is checking socket state. > + > /* It's still possible to bind after adding keys or even > * re-bind to a different dev (with CAP_NET_RAW). > * So, no reason to return error here, rather try to be
Hi David, On 11/29/23 01:34, David Ahern wrote: > On 11/28/23 1:57 PM, Dmitry Safonov wrote: >> If the connection was established, don't allow adding TCP-AO keys that >> don't match the peer. Currently, there are checks for ip-address >> matching, but L3 index check is missing. Add it to restrict userspace > > you say L3 index check is missing - add it. yet ... > >> shooting itself somewhere. >> >> Fixes: 248411b8cb89 ("net/tcp: Wire up l3index to TCP-AO") >> Signed-off-by: Dmitry Safonov <dima@arista.com> >> --- >> net/ipv4/tcp_ao.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/net/ipv4/tcp_ao.c b/net/ipv4/tcp_ao.c >> index bf41be6d4721..2d000e275ce7 100644 >> --- a/net/ipv4/tcp_ao.c >> +++ b/net/ipv4/tcp_ao.c >> @@ -1608,6 +1608,9 @@ static int tcp_ao_add_cmd(struct sock *sk, unsigned short int family, >> if (!dev || !l3index) >> return -EINVAL; >> >> + if (!((1 << sk->sk_state) & (TCPF_LISTEN | TCPF_CLOSE))) >> + return -EINVAL; > > ... this is checking socket state. Right you are, it should have been under check for : if (bound_dev_if != cmd.ifindex) Currently it's warning for all sockets (which can be re-bound), but for sockets in the connected state it doesn't make sense as the key lookup is not expecting non peer-matching key post connect()/accept(). In this patch version the check will restrict adding a key on a connected socket with VRF regardless if it's matching the bound VRF. Will fix! Thanks for spotting this, Dmitry
diff --git a/net/ipv4/tcp_ao.c b/net/ipv4/tcp_ao.c index bf41be6d4721..2d000e275ce7 100644 --- a/net/ipv4/tcp_ao.c +++ b/net/ipv4/tcp_ao.c @@ -1608,6 +1608,9 @@ static int tcp_ao_add_cmd(struct sock *sk, unsigned short int family, if (!dev || !l3index) return -EINVAL; + if (!((1 << sk->sk_state) & (TCPF_LISTEN | TCPF_CLOSE))) + return -EINVAL; + /* It's still possible to bind after adding keys or even * re-bind to a different dev (with CAP_NET_RAW). * So, no reason to return error here, rather try to be
If the connection was established, don't allow adding TCP-AO keys that don't match the peer. Currently, there are checks for ip-address matching, but L3 index check is missing. Add it to restrict userspace shooting itself somewhere. Fixes: 248411b8cb89 ("net/tcp: Wire up l3index to TCP-AO") Signed-off-by: Dmitry Safonov <dima@arista.com> --- net/ipv4/tcp_ao.c | 3 +++ 1 file changed, 3 insertions(+)