diff mbox series

[Openvpn-devel,net,v4,5/9] ovpn: zero-initialize sockaddr before learning a floated endpoint

Message ID 20260728114855.1323861-6-a@unstable.cc
State Accepted
Headers
Series ovpn: assorted net fixes |

Commit Message

Antonio Quartulli July 28, 2026, 11:48 a.m. UTC 
  From: Antonio Quartulli <antonio@openvpn.net>

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>
---
 drivers/net/ovpn/peer.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

Comments

Sabrina Dubroca July 30, 2026, 2:39 p.m. UTC | #1 
2026-07-28, 13:48:51 +0200, Antonio Quartulli wrote:
> From: Antonio Quartulli <antonio@openvpn.net>
> 
> 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>
> ---
>  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. 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.

[*] 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.
Antonio Quartulli July 30, 2026, 7:24 p.m. UTC | #2 
Hi Sabrina!

On 30/07/2026 16:39, Sabrina Dubroca wrote:
> 2026-07-28, 13:48:51 +0200, Antonio Quartulli wrote:
>> From: Antonio Quartulli <antonio@openvpn.net>
>>
>> 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>
>> ---
>>   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,
diff mbox series

Patch

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,
+			};
 			salen = sizeof(*sa);
 			reset_cache = true;
 			break;
@@ -272,11 +279,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;