Re: netbsd-8: panic: sockaddr_copy: source too long, 28 < 128 bytes

[Ryota Ozaki <ozaki-r%netbsd.org@localhost>]

On Mon, Nov 5, 2018 at 10:38 AM Paul Ripke <stix%stix.id.au@localhost> wrote:
>
> I'm running netbsd-8 synced as of ~2018-10-30, and am continuing to
> get occasional panics, about once a week on the prior kernel, and
> another just now. Is this familiar to anybody? Core on hand, pointers
> on what to look at appreciated.
>
> NetBSD slave 8.0_STABLE NetBSD 8.0_STABLE (SLAVE) #1: Tue Oct 30 07:40:01 AEDT 2018  stix@slave:/home/netbsd/netbsd-8/obj.amd64/home/netbsd/netbsd-8/src/sys/arch/amd64/compile/SLAVE amd64
>
> Nov  5 12:09:59 slave /netbsd: panic: sockaddr_copy: source too long, 28 < 128 bytes
> Nov  5 12:09:59 slave /netbsd: cpu0: Begin traceback...
> Nov  5 12:09:59 slave /netbsd: vpanic() at netbsd:vpanic+0x15d
> Nov  5 12:09:59 slave /netbsd: snprintf() at netbsd:snprintf
> Nov  5 12:09:59 slave /netbsd: sockaddr_copy() at netbsd:sockaddr_copy+0x7f
> Nov  5 12:09:59 slave /netbsd: rtcache_setdst() at netbsd:rtcache_setdst+0x5d
> Nov  5 12:09:59 slave /netbsd: rtcache_lookup2() at netbsd:rtcache_lookup2+0x5e
> Nov  5 12:09:59 slave /netbsd: in6_selectroute() at netbsd:in6_selectroute+0x15a
> Nov  5 12:09:59 slave /netbsd: in6_selectsrc() at netbsd:in6_selectsrc+0x100
> Nov  5 12:09:59 slave /netbsd: udp6_output() at netbsd:udp6_output+0x246
> Nov  5 12:09:59 slave /netbsd: udp6_send_wrapper() at netbsd:udp6_send_wrapper+0x51
> Nov  5 12:09:59 slave /netbsd: sosend() at netbsd:sosend+0x77f
> Nov  5 12:09:59 slave /netbsd: do_sys_sendmsg_so() at netbsd:do_sys_sendmsg_so+0x26d
> Nov  5 12:09:59 slave /netbsd: do_sys_sendmsg() at netbsd:do_sys_sendmsg+0x85
> Nov  5 12:09:59 slave /netbsd: sys_sendto() at netbsd:sys_sendto+0x5c
> Nov  5 12:09:59 slave /netbsd: syscall() at netbsd:syscall+0x1ec
> Nov  5 12:09:59 slave /netbsd: --- syscall (number 133) ---
> Nov  5 12:09:59 slave /netbsd: 7db02a8eea4a:
> Nov  5 12:09:59 slave /netbsd: cpu0: End traceback...
>
> (gdb) p *src
> $3 = {
>   sa_len = 128 '\200',
>   sa_family = 24 '\030',
>   sa_data = "\254\005\000\000\000\000 \001A\320\000\n^\207"
> }
>
> --
> Paul Ripke
> "Great minds discuss ideas, average minds discuss events, small minds
>  discuss people."
> -- Disputed: Often attributed to Eleanor Roosevelt. 1948.

I can reproduce the panic easily by the small program:

// start--
#include <sys/socket.h>
#include <netinet/in.h>
#include <err.h>

int
main(void)
{
        char buf[64];
        struct sockaddr_storage ss = {0};
        int s, e;

        ss.ss_family = AF_INET6;
        ss.ss_len = sizeof(struct sockaddr_in6);
        s = socket(AF_INET6, SOCK_DGRAM, 0);
        e = sendto(s, buf, sizeof(buf), 0, (struct sockaddr *)&ss, ss.ss_len);
        if (e == -1)
                warn("sendto");
        ss.ss_len = sizeof(ss);
        e = sendto(s, buf, sizeof(buf), 0, (struct sockaddr *)&ss, ss.ss_len);
        if (e == -1)
                warn("sendto");
}
// --end

It seems that, on UDP/IPv6, a passed sockaddr to sendto isn't validated
well and passed to the lower layers as is, then it triggers the panic.

The length check of a sockaddr was performed implicitly in udp6_output but
the check was removed accidentally between NetBSD 7 and 8 (*).

(*) http://cvsweb.netbsd.org/cgi-bin/cvsweb.cgi/src/sys/netinet6/Attic/udp6_output.c.diff?r1=1.48&r2=1.49&f=h

So the follow patch fixes the issue. (There can be better fixes.)

Thanks,
  ozaki-r

diff --git a/sys/netinet6/udp6_usrreq.c b/sys/netinet6/udp6_usrreq.c
index ee4fc6fdfb3..a4a74c8009e 100644
--- a/sys/netinet6/udp6_usrreq.c
+++ b/sys/netinet6/udp6_usrreq.c
@@ -668,10 +668,18 @@ udp6_output(struct in6pcb * const in6p, struct mbuf *m,

        if (addr6) {
                sin6 = addr6;
+               if (sin6->sin6_len != sizeof(*sin6)) {
+                       error = EINVAL;
+                       goto release;
+               }
                if (sin6->sin6_family != AF_INET6) {
                        error = EAFNOSUPPORT;
                        goto release;
                }
+               if (sin6->sin6_port == 0) {
+                       error = EADDRNOTAVAIL;
+                       goto release;
+               }

                /* protect *sin6 from overwrites */
                tmp = *sin6;