| 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
| ||
|
Message-ID: <55196133-50dd-4c7d-915b-844dcff296b8@openvpn.net>
Date: Mon, 2 Sep 2024 14:24:07 +0200
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: netdev@...r.kernel.org, kuba@...nel.org, pabeni@...hat.com,
ryazanov.s.a@...il.com, edumazet@...gle.com, andrew@...n.ch
Subject: Re: [PATCH net-next v6 11/25] ovpn: implement basic RX path (UDP)
On 02/09/2024 13:22, Sabrina Dubroca wrote:
> 2024-08-27, 14:07:51 +0200, Antonio Quartulli wrote:
>> +static void ovpn_netdev_write(struct ovpn_peer *peer, struct sk_buff *skb)
>> +{
>> + /* we can't guarantee the packet wasn't corrupted before entering the
>> + * VPN, therefore we give other layers a chance to check that
>> + */
>> + skb->ip_summed = CHECKSUM_NONE;
>> +
>> + /* skb hash for transport packet no longer valid after decapsulation */
>> + skb_clear_hash(skb);
>> +
>> + /* post-decrypt scrub -- prepare to inject encapsulated packet onto the
>> + * interface, based on __skb_tunnel_rx() in dst.h
>> + */
>> + skb->dev = peer->ovpn->dev;
>> + skb_set_queue_mapping(skb, 0);
>> + skb_scrub_packet(skb, true);
>> +
>> + skb_reset_network_header(skb);
>> + skb_reset_transport_header(skb);
>> + skb_probe_transport_header(skb);
>> + skb_reset_inner_headers(skb);
>> +
>> + memset(skb->cb, 0, sizeof(skb->cb));
>> +
>> + /* cause packet to be "received" by the interface */
>> + if (likely(gro_cells_receive(&peer->ovpn->gro_cells,
>> + skb) == NET_RX_SUCCESS))
>> + /* update RX stats with the size of decrypted packet */
>> + dev_sw_netstats_rx_add(peer->ovpn->dev, skb->len);
>
> I don't think accessing skb->len after passing the skb to
> gro_cells_receive is safe, see
> c7cc9200e9b4 ("macsec: avoid use-after-free in macsec_handle_frame()")
Thanks for spotting this! It's basically the same issue (but symmetric)
as patch 10/25.
>
>
> [...]
>> static void ovpn_struct_free(struct net_device *net)
>> {
>> + struct ovpn_struct *ovpn = netdev_priv(net);
>> +
>> + gro_cells_destroy(&ovpn->gro_cells);
>> + rcu_barrier();
>
> What's the purpose of this rcu_barrier? I expect it in module_exit,
> not when removing one netdevice.
Good question.
I presume it's a leftover from previous tests.
I think it's harmless, but it should not be needed at all.
I will remove it.
>
>
>> diff --git a/drivers/net/ovpn/skb.h b/drivers/net/ovpn/skb.h
>> index 7966a10d915f..e070fe6f448c 100644
>> --- a/drivers/net/ovpn/skb.h
>> +++ b/drivers/net/ovpn/skb.h
>> @@ -18,10 +18,7 @@
>> #include <linux/types.h>
>>
>> struct ovpn_cb {
>> - struct aead_request *req;
>> struct ovpn_peer *peer;
>> - struct ovpn_crypto_key_slot *ks;
>> - unsigned int payload_offset;
>
> Squashed into the wrong patch?
Darn yes. Sorry for this.
>
>
> [...]
>> +struct ovpn_struct *ovpn_from_udp_sock(struct sock *sk)
>> +{
>> + struct ovpn_socket *ovpn_sock;
>> +
>> + if (unlikely(READ_ONCE(udp_sk(sk)->encap_type) != UDP_ENCAP_OVPNINUDP))
>> + return NULL;
>> +
>> + ovpn_sock = rcu_dereference_sk_user_data(sk);
>
> [1]
>
>> + if (unlikely(!ovpn_sock))
>> + return NULL;
>> +
>> + /* make sure that sk matches our stored transport socket */
>> + if (unlikely(!ovpn_sock->sock || sk != ovpn_sock->sock->sk))
>> + return NULL;
>> +
>> + return ovpn_sock->ovpn;
>> +}
>
>
>> +static int ovpn_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
>> +{
>> + struct ovpn_peer *peer = NULL;
>> + struct ovpn_struct *ovpn;
>> + u32 peer_id;
>> + u8 opcode;
>> +
>> + ovpn = ovpn_from_udp_sock(sk);
>> + if (unlikely(!ovpn)) {
>> + net_err_ratelimited("%s: cannot obtain ovpn object from UDP socket\n",
>> + __func__);
>> + goto drop;
>> + }
> [...]
>> + /* pop off outer UDP header */
>> + __skb_pull(skb, sizeof(struct udphdr));
>> + ovpn_recv(peer, skb);
>> + return 0;
>> +
>> +drop:
>> + if (peer)
>> + ovpn_peer_put(peer);
>> + dev_core_stats_rx_dropped_inc(ovpn->dev);
>
> If we get here from the first goto, ovpn is NULL. You could add a
> drop_noovpn label right here to just do the free+return.
Right.
Weird though that no static analysis tool complained about ovpn possibly
being NULL.
Will add the extra label.
>
>> + kfree_skb(skb);
>> + return 0;
>> +}
>> +
>> /**
>> * ovpn_udp4_output - send IPv4 packet over udp socket
>> * @ovpn: the openvpn instance
>> @@ -257,8 +342,13 @@ void ovpn_udp_send_skb(struct ovpn_struct *ovpn, struct ovpn_peer *peer,
>> */
>> int ovpn_udp_socket_attach(struct socket *sock, struct ovpn_struct *ovpn)
>> {
>> + struct udp_tunnel_sock_cfg cfg = {
>> + .sk_user_data = ovpn,
>> + .encap_type = UDP_ENCAP_OVPNINUDP,
>> + .encap_rcv = ovpn_udp_encap_recv,
>> + };
>> struct ovpn_socket *old_data;
>> - int ret = 0;
>> + int ret;
>>
>> /* sanity check */
>> if (sock->sk->sk_protocol != IPPROTO_UDP) {
>> @@ -272,6 +362,7 @@ int ovpn_udp_socket_attach(struct socket *sock, struct ovpn_struct *ovpn)
>> if (!old_data) {
>> /* socket is currently unused - we can take it */
>> rcu_read_unlock();
>> + setup_udp_tunnel_sock(sock_net(sock->sk), sock, &cfg);
>
> This will set sk_user_data to the ovpn_struct, but ovpn_from_udp_sock
> expects the ovpn_socket [1], which is stored into sk_user_data a
> little bit later by ovpn_socket_new. If a packet reaches
> ovpn_udp_encap_recv -> ovpn_from_udp_sock before ovpn_socket_new
> overwrites sk_user_data, bad things probably happen.
Wow - this is a very nice catch.
I think this wrong cfg.sk_user_data initialization was there this since
the first prototype, but it just passed unnoticed.
I will drop the field initialization, so that the sk_user_data stays
NULL until it gets assigned the ovpn_sock.
Thanks a lot!
Cheers,
--
Antonio Quartulli
OpenVPN Inc.
Powered by blists - more mailing lists