| Message ID | a0e7477985ef08c5f08f35b8c7336587c8adce12.1652368648.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | UDP/IPv6 refactoring | expand |
On Fri, 2022-05-13 at 16:26 +0100, Pavel Begunkov wrote: > Move up->pending section of udpv6_sendmsg() to the beginning of the > function. Even though it require some code duplication for sin6 parsing, > it clearly localises the pending handling in one place, removes an extra > if and more importantly will prepare the code for further patches. > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > --- > net/ipv6/udp.c | 70 ++++++++++++++++++++++++++++++-------------------- > 1 file changed, 42 insertions(+), 28 deletions(-) > > diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c > index 11d44ed46953..85bff1252f5c 100644 > --- a/net/ipv6/udp.c > +++ b/net/ipv6/udp.c > @@ -1318,6 +1318,46 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len) > ipc6.sockc.tsflags = sk->sk_tsflags; > ipc6.sockc.mark = sk->sk_mark; > > + /* Rough check on arithmetic overflow, > + better check is made in ip6_append_data(). > + */ > + if (unlikely(len > INT_MAX - sizeof(struct udphdr))) > + return -EMSGSIZE; > + > + getfrag = is_udplite ? udplite_getfrag : ip_generic_getfrag; > + > + /* There are pending frames. */ > + if (up->pending) { > + if (up->pending == AF_INET) > + return udp_sendmsg(sk, msg, len); > + > + /* Do a quick destination sanity check before corking. */ > + if (sin6) { > + if (msg->msg_namelen < offsetof(struct sockaddr, sa_data)) > + return -EINVAL; > + if (sin6->sin6_family == AF_INET6) { > + if (msg->msg_namelen < SIN6_LEN_RFC2133) > + return -EINVAL; > + if (ipv6_addr_any(&sin6->sin6_addr) && > + ipv6_addr_v4mapped(&np->saddr)) > + return -EINVAL; It looks like 'any' destination with ipv4 mapped source is now rejected, while the existing code accept it. /P
On 5/16/22 14:11, Paolo Abeni wrote: > On Fri, 2022-05-13 at 16:26 +0100, Pavel Begunkov wrote: >> Move up->pending section of udpv6_sendmsg() to the beginning of the >> function. Even though it require some code duplication for sin6 parsing, >> it clearly localises the pending handling in one place, removes an extra >> if and more importantly will prepare the code for further patches. >> >> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >> --- >> net/ipv6/udp.c | 70 ++++++++++++++++++++++++++++++-------------------- >> 1 file changed, 42 insertions(+), 28 deletions(-) >> >> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c >> index 11d44ed46953..85bff1252f5c 100644 >> --- a/net/ipv6/udp.c >> +++ b/net/ipv6/udp.c >> @@ -1318,6 +1318,46 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len) >> ipc6.sockc.tsflags = sk->sk_tsflags; >> ipc6.sockc.mark = sk->sk_mark; >> >> + /* Rough check on arithmetic overflow, >> + better check is made in ip6_append_data(). >> + */ >> + if (unlikely(len > INT_MAX - sizeof(struct udphdr))) >> + return -EMSGSIZE; >> + >> + getfrag = is_udplite ? udplite_getfrag : ip_generic_getfrag; >> + >> + /* There are pending frames. */ >> + if (up->pending) { >> + if (up->pending == AF_INET) >> + return udp_sendmsg(sk, msg, len); >> + >> + /* Do a quick destination sanity check before corking. */ >> + if (sin6) { >> + if (msg->msg_namelen < offsetof(struct sockaddr, sa_data)) >> + return -EINVAL; >> + if (sin6->sin6_family == AF_INET6) { >> + if (msg->msg_namelen < SIN6_LEN_RFC2133) >> + return -EINVAL; >> + if (ipv6_addr_any(&sin6->sin6_addr) && >> + ipv6_addr_v4mapped(&np->saddr)) >> + return -EINVAL; > > It looks like 'any' destination with ipv4 mapped source is now > rejected, while the existing code accept it. It should be up->pending == AF_INET6 to get there, and previously it'd fall into udp_sendmsg() and fail if (unlikely(up->pending != AF_INET)) return -EINVAL; I don't see it anyhow rejecting cases that were working before. Can you elaborate a bit?
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c index 11d44ed46953..85bff1252f5c 100644 --- a/net/ipv6/udp.c +++ b/net/ipv6/udp.c @@ -1318,6 +1318,46 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len) ipc6.sockc.tsflags = sk->sk_tsflags; ipc6.sockc.mark = sk->sk_mark; + /* Rough check on arithmetic overflow, + better check is made in ip6_append_data(). + */ + if (unlikely(len > INT_MAX - sizeof(struct udphdr))) + return -EMSGSIZE; + + getfrag = is_udplite ? udplite_getfrag : ip_generic_getfrag; + + /* There are pending frames. */ + if (up->pending) { + if (up->pending == AF_INET) + return udp_sendmsg(sk, msg, len); + + /* Do a quick destination sanity check before corking. */ + if (sin6) { + if (msg->msg_namelen < offsetof(struct sockaddr, sa_data)) + return -EINVAL; + if (sin6->sin6_family == AF_INET6) { + if (msg->msg_namelen < SIN6_LEN_RFC2133) + return -EINVAL; + if (ipv6_addr_any(&sin6->sin6_addr) && + ipv6_addr_v4mapped(&np->saddr)) + return -EINVAL; + } else if (sin6->sin6_family != AF_UNSPEC) { + return -EINVAL; + } + } + + /* The socket lock must be held while it's corked. */ + lock_sock(sk); + if (unlikely(up->pending != AF_INET6)) { + /* Just now it was seen corked, userspace is buggy */ + err = up->pending ? -EAFNOSUPPORT : -EINVAL; + release_sock(sk); + return err; + } + dst = NULL; + goto do_append_data; + } + /* destination address check */ if (sin6) { if (addr_len < offsetof(struct sockaddr, sa_data)) @@ -1343,12 +1383,11 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len) default: return -EINVAL; } - } else if (!up->pending) { + } else { if (sk->sk_state != TCP_ESTABLISHED) return -EDESTADDRREQ; daddr = &sk->sk_v6_daddr; - } else - daddr = NULL; + } if (daddr) { if (ipv6_addr_v4mapped(daddr)) { @@ -1365,31 +1404,6 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len) } } - /* Rough check on arithmetic overflow, - better check is made in ip6_append_data(). - */ - if (len > INT_MAX - sizeof(struct udphdr)) - return -EMSGSIZE; - - getfrag = is_udplite ? udplite_getfrag : ip_generic_getfrag; - if (up->pending) { - if (up->pending == AF_INET) - return udp_sendmsg(sk, msg, len); - /* - * There are pending frames. - * The socket lock must be held while it's corked. - */ - lock_sock(sk); - if (likely(up->pending)) { - if (unlikely(up->pending != AF_INET6)) { - release_sock(sk); - return -EAFNOSUPPORT; - } - dst = NULL; - goto do_append_data; - } - release_sock(sk); - } ulen += sizeof(struct udphdr); memset(fl6, 0, sizeof(*fl6));
Move up->pending section of udpv6_sendmsg() to the beginning of the function. Even though it require some code duplication for sin6 parsing, it clearly localises the pending handling in one place, removes an extra if and more importantly will prepare the code for further patches. Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- net/ipv6/udp.c | 70 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 42 insertions(+), 28 deletions(-)