Hi Sabrina!
On 30/07/2026 16:39, Sabrina Dubroca wrote:
2026-07-28, 13:48:51 +0200, Antonio Quartulli wrote:
From: Antonio Quartulli <[email protected]>
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 <[email protected]>
---
drivers/net/ovpn/peer.c | 31 +++++++++++++++++++++++--------
1 file changed, 23 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c
index 3554da01e406..00971bbd3dcf 100644
--- a/drivers/net/ovpn/peer.c
+++ b/drivers/net/ovpn/peer.c
@@ -244,9 +244,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,
+ };
I'm running behind on reviews, so I missed this version of the patch
until it landed on netdev.
no worries - I just have a long backlog of fixes and I trying to send
them out at a reasonable pace...
I find this syntax really ugly. I get that
sashiko was upset about the useless memset [*], but a memset in that
branch would have been fine too.
yeah, moving the memset was also an option.
Honestly I don't have a strong opinion, either way.
I just wouldn't want to hold on the whole PR for this cosmetic detail
(both strategies are syntactically equivalent).
We could possibly do a re-styling in net-next later on.
What do you think?
[*] sometimes it's also ok to ignore it. a small memset, even in the
hotpath, probably has a non-measurable effect on throughput for a
codepath that also encrypts/decrypts full packets.
True, but sometimes you just want the code to be cleaner (for some
definition of clean..) :)
Cheers,
--
Antonio Quartulli
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
