Message ID | 1555066599-9698-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp |
---|---|
State | Accepted |
Delegated to: | David Miller |
Headers | show |
Series | [1/9] net/rds: Check address length before reading address family | expand |
> On Apr 12, 2019, at 3:56 AM, Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote: > > KMSAN will complain if valid address length passed to udpv6_pre_connect() > is shorter than sizeof("struct sockaddr"->sa_family) bytes. > > (This patch is bogus if it is guaranteed that udpv6_pre_connect() is > always called after checking "struct sockaddr"->sa_family. In that case, > we want a comment why we don't need to check valid address length here.) > > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Looks good. Acked-by: Song Liu <songliubraving@fb.com> > --- > net/ipv6/udp.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c > index d538fafaf4a9..2464fba569b4 100644 > --- a/net/ipv6/udp.c > +++ b/net/ipv6/udp.c > @@ -1045,6 +1045,8 @@ static void udp_v6_flush_pending_frames(struct sock *sk) > static int udpv6_pre_connect(struct sock *sk, struct sockaddr *uaddr, > int addr_len) > { > + if (addr_len < offsetofend(struct sockaddr, sa_family)) > + return -EINVAL; > /* The following checks are replicated from __ip6_datagram_connect() > * and intended to prevent BPF program called below from accessing > * bytes that are out of the bound specified by user in addr_len. > -- > 2.16.5 >
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> [Fri, 2019-04-12 03:57 -0700]: > KMSAN will complain if valid address length passed to udpv6_pre_connect() > is shorter than sizeof("struct sockaddr"->sa_family) bytes. > > (This patch is bogus if it is guaranteed that udpv6_pre_connect() is > always called after checking "struct sockaddr"->sa_family. In that case, > we want a comment why we don't need to check valid address length here.) > > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > --- > net/ipv6/udp.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c > index d538fafaf4a9..2464fba569b4 100644 > --- a/net/ipv6/udp.c > +++ b/net/ipv6/udp.c > @@ -1045,6 +1045,8 @@ static void udp_v6_flush_pending_frames(struct sock *sk) > static int udpv6_pre_connect(struct sock *sk, struct sockaddr *uaddr, > int addr_len) > { > + if (addr_len < offsetofend(struct sockaddr, sa_family)) > + return -EINVAL; Such a check wasn't added since it's already checked in inet_dgram_connect, the only place where udpv6_pre_connect is called: int inet_dgram_connect(struct socket *sock, struct sockaddr *uaddr, int addr_len, int flags) { struct sock *sk = sock->sk; int err; if (addr_len < sizeof(uaddr->sa_family)) return -EINVAL; if (uaddr->sa_family == AF_UNSPEC) return sk->sk_prot->disconnect(sk, flags); if (BPF_CGROUP_PRE_CONNECT_ENABLED(sk)) { err = sk->sk_prot->pre_connect(sk, uaddr, addr_len); if (err) return err; } So it's already handled. But if it helps KMSAN, that's probably fine to double-check it here. Or it's considered false positive? > /* The following checks are replicated from __ip6_datagram_connect() > * and intended to prevent BPF program called below from accessing > * bytes that are out of the bound specified by user in addr_len.
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Date: Fri, 12 Apr 2019 19:56:39 +0900 > KMSAN will complain if valid address length passed to udpv6_pre_connect() > is shorter than sizeof("struct sockaddr"->sa_family) bytes. > > (This patch is bogus if it is guaranteed that udpv6_pre_connect() is > always called after checking "struct sockaddr"->sa_family. In that case, > we want a comment why we don't need to check valid address length here.) > > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Applied.
On 2019/04/13 1:49, Andrey Ignatov wrote: > Such a check wasn't added since it's already checked in > inet_dgram_connect, the only place where udpv6_pre_connect is called: > > int inet_dgram_connect(struct socket *sock, struct sockaddr *uaddr, > int addr_len, int flags) > { > struct sock *sk = sock->sk; > int err; > > if (addr_len < sizeof(uaddr->sa_family)) > return -EINVAL; > if (uaddr->sa_family == AF_UNSPEC) > return sk->sk_prot->disconnect(sk, flags); > > if (BPF_CGROUP_PRE_CONNECT_ENABLED(sk)) { > err = sk->sk_prot->pre_connect(sk, uaddr, addr_len); > if (err) > return err; > } > > So it's already handled. But if it helps KMSAN, that's probably fine to > double-check it here. Or it's considered false positive? OK, then KMSAN will not complain and this patch can be dropped.
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c index d538fafaf4a9..2464fba569b4 100644 --- a/net/ipv6/udp.c +++ b/net/ipv6/udp.c @@ -1045,6 +1045,8 @@ static void udp_v6_flush_pending_frames(struct sock *sk) static int udpv6_pre_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len) { + if (addr_len < offsetofend(struct sockaddr, sa_family)) + return -EINVAL; /* The following checks are replicated from __ip6_datagram_connect() * and intended to prevent BPF program called below from accessing * bytes that are out of the bound specified by user in addr_len.
KMSAN will complain if valid address length passed to udpv6_pre_connect() is shorter than sizeof("struct sockaddr"->sa_family) bytes. (This patch is bogus if it is guaranteed that udpv6_pre_connect() is always called after checking "struct sockaddr"->sa_family. In that case, we want a comment why we don't need to check valid address length here.) Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- net/ipv6/udp.c | 2 ++ 1 file changed, 2 insertions(+)