Re: [PATCH mptcp-net] selftests: mptcp: fix an UAF in mptcp_connect.c
Matthieu Baerts <[email protected]>
| Newsgroups | dev.linux.lists.mptcp |
|---|---|
| Message-ID | <[email protected]> |
14 Aug 2026 11:20:47 [email protected]: > August 14, 2026 at 4:54 PM, "Matthieu Baerts" <[email protected] mailto:[email protected]?to=%22Matthieu%20Baerts%22%20%3Cmatttbe%40kernel.org%3E > wrote: > > > >>> >> This assumes peer == addr. It is certainly the case but it looks wrong, >> and I guess sashiko will complain like it did here. >> >> We could add something on the commit message to say that it is always an IP address that is given, but maybe we should also just >> handle that correctly: either peer here is not a pointer, and the content >> is copied, > > I prefer this, because it is smaller, like: > > ''' > static int sock_connect_mptcp(const char * const remoteaddr, > const char * const port, int proto, > - struct addrinfo **peer, > + struct sockaddr_storage *peer, socklen_t *peer_len, > int infd, struct wstate *winfo) > { > ... > - *peer = a; > break; > ... > - *peer = a; > break; > ... > + if (sock != -1) { > + memcpy(peer, a->ai_addr, a->ai_addrlen); > + *peer_len = a->ai_addrlen; > + } > freeaddrinfo(addr); > ... > > - struct addrinfo *peer = NULL; > + struct sockaddr_storage peer; > + socklen_t peer_len; > ... > - fd = sock_connect_mptcp(..., &peer, fd_in, &winfo); > + fd = sock_connect_mptcp(..., &peer, &peer_len, fd_in, &winfo); > ... > - if (connect(fd, peer->ai_addr, peer->ai_addrlen)) > + if (connect(fd, (struct sockaddr *)&peer, peer_len)) > ''' It looks OK but there are some modifications a bit everywhere. >> or you pass &addr to sock_connect_mptcp, and you do the free here. > > This needs a refactor of sock_connect_mptcp – I used to do it that way. But I > worry that backporting could be tricky. If you're fine with it, I'm okay with this plan > as well. I didn't check in details, but it seems like it would be shorter: just need to pass &addr to sock_connect_mptcp(... ••addr, ...) and use *addr instead of addr in two places + moving the declaration and free to the parent function. No? If I'm missing something, the copy is fine. Cheers, Matt