| Message ID | 20220916143740.831881-4-Jason@zx2c4.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 26c013108c12b94bc023bf19198a4300596c98b1 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | wireguard patches for 6.0-rc6 | expand |
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/
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> Oh, also, please include reporter details: Reported-by: syzbot+a448cda4dba2dac50de5@syzkaller.appspotmail.com
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 } } }; - 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); } }
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(-)