Re: [PATCH net 3/3] wireguard: netlink: avoid variable-sized memcpy on sockaddr

[Kees Cook <keescook@chromium.org>]

On Fri, Sep 16, 2022 at 03:37:40PM +0100, Jason A. Donenfeld wrote:
> Doing a variable-sized memcpy is slower, and the compiler isn't smart
> enough to turn this into a constant-size assignment.
> 
> Further, Kees' latest fortified memcpy will actually bark, because the
> destination pointer is type sockaddr, not explicitly sockaddr_in or
> sockaddr_in6, so it thinks there's an overflow:
> 
>     memcpy: detected field-spanning write (size 28) of single field
>     "&endpoint.addr" at drivers/net/wireguard/netlink.c:446 (size 16)
> 
> Fix this by just assigning by using explicit casts for each checked
> case.
> 
> Cc: Kees Cook <keescook@chromium.org>
> Fixes: e7096c131e51 ("net: WireGuard secure network tunnel")
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  drivers/net/wireguard/netlink.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/wireguard/netlink.c b/drivers/net/wireguard/netlink.c
> index d0f3b6d7f408..5c804bcabfe6 100644
> --- a/drivers/net/wireguard/netlink.c
> +++ b/drivers/net/wireguard/netlink.c
> @@ -436,14 +436,13 @@ static int set_peer(struct wg_device *wg, struct nlattr **attrs)
>  	if (attrs[WGPEER_A_ENDPOINT]) {
>  		struct sockaddr *addr = nla_data(attrs[WGPEER_A_ENDPOINT]);
>  		size_t len = nla_len(attrs[WGPEER_A_ENDPOINT]);
> +		struct endpoint endpoint = { { { 0 } } };

FWIW, this is equivalent[1] on all our compiler versions now:

+		struct endpoint endpoint = { };

>  
> -		if ((len == sizeof(struct sockaddr_in) &&
> -		     addr->sa_family == AF_INET) ||
> -		    (len == sizeof(struct sockaddr_in6) &&
> -		     addr->sa_family == AF_INET6)) {
> -			struct endpoint endpoint = { { { 0 } } };
> -
> -			memcpy(&endpoint.addr, addr, len);
> +		if (len == sizeof(struct sockaddr_in) && addr->sa_family == AF_INET) {
> +			endpoint.addr4 = *(struct sockaddr_in *)addr;
> +			wg_socket_set_peer_endpoint(peer, &endpoint);
> +		} else if (len == sizeof(struct sockaddr_in6) && addr->sa_family == AF_INET6) {
> +			endpoint.addr6 = *(struct sockaddr_in6 *)addr;
>  			wg_socket_set_peer_endpoint(peer, &endpoint);
>  		}
>  	}

Ah, sneaky! I like it. :)

Reviewed-by: Kees Cook <keescook@chromium.org>


I wonder if we need a "converter" struct to help with this -- this isn't
the only place this code pattern exists.

struct sockaddr_decode {
	union {
		struct sockaddr		addr;
		struct sockaddr_in	addr4;
		struct sockaddr_in6	addr6;
		DECLARE_FLEX_ARRAY(u8,	content);
	};
};

	struct sockaddr_decode *addr = nla_data(attrs[WGPEER_A_ENDPOINT]);
	...
	if (len == sizeof(addr->addr4) && addr->addr.sa_family == AF_INET) {
		endpoint.addr4 = addr->addr4;
	...


This looks a lot like these open issues we've had for a while:
https://github.com/KSPP/linux/issues/169
https://github.com/KSPP/linux/issues/140

-Kees

[1] https://lore.kernel.org/lkml/20210910225207.3272766-1-keescook@chromium.org/

-- 
Kees Cook