ovpn: zero-initialize sockaddr before learning a floated endpoint

ovpn_peer_endpoints_update() builds the new remote endpoint in an
on-stack struct sockaddr_storage that is left uninitialized. For IPv4
only sin_family/sin_addr/sin_port are written, leaving the 8-byte
sin_zero padding as stack garbage (for IPv6, sin6_flowinfo is left
uninitialized likewise).

ovpn_peer_reset_sockaddr() -> ovpn_bind_from_sockaddr() then memcpy()s
sizeof(struct sockaddr_in)/sizeof(struct sockaddr_in6) bytes - padding
included - into bind->remote. That buffer is later hashed with jhash()
over the same length to place the peer in the by_transp_addr table, so
the garbage padding lands the floated peer in an essentially random
bucket. Lockless lookups in ovpn_peer_get_by_transp_addr() build their
key from a zero-initialized sockaddr_storage, compute a different bucket
and fail to find the peer.

This is also a plain use of uninitialized stack memory in jhash().

Build the floated endpoint with a designated initializer so the
padding (sin_zero for IPv4, sin6_flowinfo for IPv6) is zeroed as part
of the assignment. This keeps the padding out of the by_transp_addr
hash key without memset-ing the whole sockaddr_storage on every
received packet.

Fixes: f0281c1d3732 ("ovpn: add support for updating local or remote UDP endpoint")
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>

diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c
index a330892e82bf7f63ca9454c28eada7e7df50b98d..33fb0a75e60066b81876cc8af4ffcc5ba3eeebf5 100644 (file)
--- a/drivers/net/ovpn/peer.c
+++ b/drivers/net/ovpn/peer.c
@@ -222,9 +222,16 @@ void ovpn_peer_endpoints_update(struct ovpn_peer *peer, struct sk_buff *skb)
                         */
                        local_ip = &ip_hdr(skb)->daddr;
                        sa = (struct sockaddr_in *)&ss;
-                       sa->sin_family = AF_INET;
-                       sa->sin_addr.s_addr = ip_hdr(skb)->saddr;
-                       sa->sin_port = udp_hdr(skb)->source;
+                       /* use a designated initializer so the sin_zero padding
+                        * is zeroed (it ends up in the by_transp_addr hash key)
+                        * without memset-ing the whole sockaddr_storage on the
+                        * RX fast path
+                        */
+                       *sa = (struct sockaddr_in) {
+                               .sin_family = AF_INET,
+                               .sin_addr.s_addr = ip_hdr(skb)->saddr,
+                               .sin_port = udp_hdr(skb)->source,
+                       };
                        salen = sizeof(*sa);
                        reset_cache = true;
                        break;
@@ -250,11 +257,19 @@ void ovpn_peer_endpoints_update(struct ovpn_peer *peer, struct sk_buff *skb)
                         */
                        local_ip = &ipv6_hdr(skb)->daddr;
                        sa6 = (struct sockaddr_in6 *)&ss;
-                       sa6->sin6_family = AF_INET6;
-                       sa6->sin6_addr = ipv6_hdr(skb)->saddr;
-                       sa6->sin6_port = udp_hdr(skb)->source;
-                       sa6->sin6_scope_id = ipv6_iface_scope_id(&ipv6_hdr(skb)->saddr,
-                                                                skb->skb_iif);
+                       /* use a designated initializer so the sin6_flowinfo
+                        * padding is zeroed (it ends up in the by_transp_addr
+                        * hash key) without memset-ing the whole
+                        * sockaddr_storage on the RX fast path
+                        */
+                       *sa6 = (struct sockaddr_in6) {
+                               .sin6_family = AF_INET6,
+                               .sin6_addr = ipv6_hdr(skb)->saddr,
+                               .sin6_port = udp_hdr(skb)->source,
+                               .sin6_scope_id =
+                                       ipv6_iface_scope_id(&ipv6_hdr(skb)->saddr,
+                                                           skb->skb_iif),
+                       };
                        salen = sizeof(*sa6);
                        reset_cache = true;
                        break;