| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 10 May 2024 15:39:33 +0200
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
Sergey Ryazanov <ryazanov.s.a@...il.com>, Paolo Abeni <pabeni@...hat.com>,
Eric Dumazet <edumazet@...gle.com>, Andrew Lunn <andrew@...n.ch>,
Esben Haabendal <esben@...nix.com>
Subject: Re: [PATCH net-next v3 09/24] ovpn: implement basic TX path (UDP)
On 10/05/2024 15:01, Sabrina Dubroca wrote:
> 2024-05-06, 03:16:22 +0200, Antonio Quartulli wrote:
>> diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
>> index a420bb45f25f..36cfb95edbf4 100644
>> --- a/drivers/net/ovpn/io.c
>> +++ b/drivers/net/ovpn/io.c
>> @@ -28,6 +30,12 @@ int ovpn_struct_init(struct net_device *dev)
>>
>> spin_lock_init(&ovpn->lock);
>>
>> + ovpn->crypto_wq = alloc_workqueue("ovpn-crypto-wq-%s",
>> + WQ_CPU_INTENSIVE | WQ_MEM_RECLAIM, 0,
>> + dev->name);
>> + if (!ovpn->crypto_wq)
>> + return -ENOMEM;
>> +
>> ovpn->events_wq = alloc_workqueue("ovpn-events-wq-%s", WQ_MEM_RECLAIM,
>> 0, dev->name);
>> if (!ovpn->events_wq)
>> return -ENOMEM;
>
> This will leak crypto_wq on failure. You need to roll back all
> previous changes when something fails (also if you move all this stuff
> into ndo_init).
ouch, good catch! thanks.
>
>> diff --git a/drivers/net/ovpn/peer.h b/drivers/net/ovpn/peer.h
>> index 659df320525c..f915afa260c3 100644
>> --- a/drivers/net/ovpn/peer.h
>> +++ b/drivers/net/ovpn/peer.h
>> @@ -22,9 +23,12 @@
>> * @id: unique identifier
>> * @vpn_addrs.ipv4: IPv4 assigned to peer on the tunnel
>> * @vpn_addrs.ipv6: IPv6 assigned to peer on the tunnel
>> + * @encrypt_work: work used to process outgoing packets
>> + * @decrypt_work: work used to process incoming packets
>
> nit: Only encrypt_work is used in this patch, decrypt_work is for RX
Right, same for tx_ring actually. will move both to the next patch
>
>
>> diff --git a/drivers/net/ovpn/udp.c b/drivers/net/ovpn/udp.c
>> index 4b7d96a13df0..f434da76dc0a 100644
>> --- a/drivers/net/ovpn/udp.c
>> +++ b/drivers/net/ovpn/udp.c
>> +/**
>> + * ovpn_udp4_output - send IPv4 packet over udp socket
>> + * @ovpn: the openvpn instance
>> + * @bind: the binding related to the destination peer
>> + * @cache: dst cache
>> + * @sk: the socket to send the packet over
>> + * @skb: the packet to send
>> + *
>> + * Return: 0 on success or a negative error code otherwise
>> + */
>> +static int ovpn_udp4_output(struct ovpn_struct *ovpn, struct ovpn_bind *bind,
>> + struct dst_cache *cache, struct sock *sk,
>> + struct sk_buff *skb)
>> +{
>> + struct rtable *rt;
>> + struct flowi4 fl = {
>> + .saddr = bind->local.ipv4.s_addr,
>> + .daddr = bind->sa.in4.sin_addr.s_addr,
>> + .fl4_sport = inet_sk(sk)->inet_sport,
>> + .fl4_dport = bind->sa.in4.sin_port,
>> + .flowi4_proto = sk->sk_protocol,
>> + .flowi4_mark = sk->sk_mark,
>> + };
>> + int ret;
>> +
>> + local_bh_disable();
>> + rt = dst_cache_get_ip4(cache, &fl.saddr);
>> + if (rt)
>> + goto transmit;
>> +
>> + if (unlikely(!inet_confirm_addr(sock_net(sk), NULL, 0, fl.saddr,
>> + RT_SCOPE_HOST))) {
>> + /* we may end up here when the cached address is not usable
>> + * anymore. In this case we reset address/cache and perform a
>> + * new look up
>
> What exactly are you trying to guard against here? The ipv4 address
> used for the last packet being removed from the device/host? I don't
> see other tunnels using dst_cache doing this kind of thing (except
> wireguard).
yes, that's the scenario being checked (which hopefully is what the
comment conveys).
>
>> + */
>> + fl.saddr = 0;
>> + bind->local.ipv4.s_addr = 0;
>> + dst_cache_reset(cache);
>> + }
>> +
>> + rt = ip_route_output_flow(sock_net(sk), &fl, sk);
>> + if (IS_ERR(rt) && PTR_ERR(rt) == -EINVAL) {
>> + fl.saddr = 0;
>> + bind->local.ipv4.s_addr = 0;
>> + dst_cache_reset(cache);
>> +
>> + rt = ip_route_output_flow(sock_net(sk), &fl, sk);
>
> Why do you need to repeat the lookup? And why only for ipv4, but not
> for ipv6?
We are repeating the lookup without the saddr.
The first lookup may have failed because the destination is not
reachable anymore from that specific source address, but it may be
reachable from another one (i.e. routing table change in a multi-homed
setup).
Why not for v6..that's a good question..I wonder if I should just do the
same and repeat the lookup with ip6addr_any as source..I think it would
make sense as we could end up in the same scenario as described for IPv4.
What do you think?
>
>> + }
>> +
>> + if (IS_ERR(rt)) {
>> + ret = PTR_ERR(rt);
>> + net_dbg_ratelimited("%s: no route to host %pISpc: %d\n",
>> + ovpn->dev->name, &bind->sa.in4, ret);
>> + goto err;
>> + }
>> + dst_cache_set_ip4(cache, &rt->dst, fl.saddr);
>
> Overall this looks a whole lot like udp_tunnel_dst_lookup, except for:
> - 2nd lookup
> - inet_confirm_addr/dst_cache_reset
but why doesn't udp_tunnel_dst_lookup account for cases where the source
address is not usable anymore? I think they are reasonable, no?
Maybe I could still use udp_tunnel_dst_lookup, but call it a second time
without saddr in case of failure?
>
> (and there's udp_tunnel6_dst_lookup for ipv6)
>
--
Antonio Quartulli
OpenVPN Inc.
Powered by blists - more mailing lists