> -----Original Message----- > From: Alexandr Nedvedicky <alexandr.nedvedi...@oracle.com> > Sent: Donnerstag, 25. März 2021 17:36 > To: Schreilechner, Dominik (RC-AT DI FA DH-GRAZ ICO) > <dominik.schreilech...@siemens.com> > Cc: tech@openbsd.org > Subject: Re: [External] : [ICMP] IP options lead to malformed reply > > Hello, > > </snip> > > > 1) ip_insertoptions() does not update length of IP header (ip_hl) > > > > > > 2) ip_hl is being overridden anyway later in ip_output() called > > > by ip_send_dispatch() to send ICMP error packet out. Looks > > > like ip_send_dispatch() should use IP_RAWOUTPUT flag so > > > ip_hl won't get overridden. > > > > Maybe I have overlooked something, but I see two problems with this. > > When the IP_RAWOUTPUT flag is set, all packets send via ip_send() are > > no longer included in the ips_localout statistic. Also, all callers of > > ip_send() need to fill out the IP header themselves (as it would be > > done in the beginning of ip_output() without the IP_RAWOUTPUT flag). > As > > far as I can tell this is not done for packets in a gif tunnel (e.g. > > the ip_id is not calculated/added). > > > > no, you have not overlooked anything, you are absolutely right. > my patch is buggy. updated diff introduces ip_send_raw(), which > is an entry point for ipsendraw_task. > > whenever icmp_send() adds options, it completes IP header and > dispatches > packet to ipsendraw_task. > > OK?
You missed the initialization of ipsendraw_mq. Otherwise, ICMP with and without IP options work for me with the diff. I don't know how this is usually handled here and it is probably a bit nit-picky, but I introduced a new function to remove the duplicate code in ip_send_dispatch and ip_sendraw_dispatch. I am not a 100% happy about the function name, but it was the best I could come up with. Also, I added a comment in icmp_send(), because it is probably not immediately obvious why ip_send_raw() is used. New diff below. regards Dominik diff --git a/sys/netinet/ip_icmp.c b/sys/netinet/ip_icmp.c index a007aa6c2b3..a6a537da87c 100644 --- a/sys/netinet/ip_icmp.c +++ b/sys/netinet/ip_icmp.c @@ -846,10 +846,19 @@ icmp_send(struct mbuf *m, struct mbuf *opts) printf("icmp_send dst %s src %s\n", dst, src); } #endif - if (opts != NULL) + /* + * ip_send cannot handle IP options properly. So, in case we have + * options fill out the IP header here and use ip_send_raw instead. + */ + if (opts != NULL) { m = ip_insertoptions(m, opts, &hlen); - - ip_send(m); + ip->ip_v = IPVERSION; + ip->ip_off &= htons(IP_DF); + ip->ip_id = htons(ip_randomid()); + ipstat_inc(ips_localout); + ip_send_raw(m); + } else + ip_send(m); } u_int32_t diff --git a/sys/netinet/ip_input.c b/sys/netinet/ip_input.c index 0ec3f723be4..bfc700dee84 100644 --- a/sys/netinet/ip_input.c +++ b/sys/netinet/ip_input.c @@ -139,6 +139,7 @@ struct cpumem *ipcounters; int ip_sysctl_ipstat(void *, size_t *, void *); static struct mbuf_queue ipsend_mq; +static struct mbuf_queue ipsendraw_mq; extern struct niqueue arpinq; @@ -147,7 +148,12 @@ int ip_dooptions(struct mbuf *, struct ifnet *); int in_ouraddr(struct mbuf *, struct ifnet *, struct rtentry **); static void ip_send_dispatch(void *); +static void ip_sendraw_dispatch(void *); +static void ip_send_do_dispatch(void *, int); static struct task ipsend_task = TASK_INITIALIZER(ip_send_dispatch, &ipsend_mq); +static struct task ipsendraw_task = + TASK_INITIALIZER(ip_sendraw_dispatch, &ipsendraw_mq); + /* * Used to save the IP options in case a protocol wants to respond * to an incoming packet over the same route if the packet got here @@ -217,6 +223,7 @@ ip_init(void) DP_SET(rootonlyports.udp, defrootonlyports_udp[i]); mq_init(&ipsend_mq, 64, IPL_SOFTNET); + mq_init(&ipsendraw_mq, 64, IPL_SOFTNET); #ifdef IPSEC ipsec_init(); @@ -1777,7 +1784,7 @@ ip_savecontrol(struct inpcb *inp, struct mbuf **mp, struct ip *ip, } void -ip_send_dispatch(void *xmq) +ip_send_do_dispatch(void *xmq, int flags) { struct mbuf_queue *mq = xmq; struct mbuf *m; @@ -1789,14 +1796,33 @@ ip_send_dispatch(void *xmq) NET_LOCK(); while ((m = ml_dequeue(&ml)) != NULL) { - ip_output(m, NULL, NULL, 0, NULL, NULL, 0); + ip_output(m, NULL, NULL, flags, NULL, NULL, 0); } NET_UNLOCK(); } +void +ip_sendraw_dispatch(void *xmq) +{ + ip_send_do_dispatch(xmq, IP_RAWOUTPUT); +} + +void +ip_send_dispatch(void *xmq) +{ + ip_send_do_dispatch(xmq, 0); +} + void ip_send(struct mbuf *m) { mq_enqueue(&ipsend_mq, m); task_add(net_tq(0), &ipsend_task); } + +void +ip_send_raw(struct mbuf *m) +{ + mq_enqueue(&ipsendraw_mq, m); + task_add(net_tq(0), &ipsendraw_task); +} diff --git a/sys/netinet/ip_output.c b/sys/netinet/ip_output.c index c01a3e7803c..ea803077304 100644 --- a/sys/netinet/ip_output.c +++ b/sys/netinet/ip_output.c @@ -765,6 +765,11 @@ ip_insertoptions(struct mbuf *m, struct mbuf *opt, int *phlen) optlen = opt->m_len - sizeof(p->ipopt_dst); if (optlen + ntohs(ip->ip_len) > IP_MAXPACKET) return (m); /* XXX should fail */ + + /* check if options will fit to IP header */ + if ((optlen + (ip->ip_hl << 2)) > (0x0f << 2)) + return (m); + if (p->ipopt_dst.s_addr) ip->ip_dst = p->ipopt_dst; if (m->m_flags & M_EXT || m->m_data - optlen < m->m_pktdat) { @@ -790,6 +795,7 @@ ip_insertoptions(struct mbuf *m, struct mbuf *opt, int *phlen) memcpy(ip + 1, p->ipopt_list, optlen); *phlen = sizeof(struct ip) + optlen; ip->ip_len = htons(ntohs(ip->ip_len) + optlen); + ip->ip_hl += (optlen >> 2); return (m); } diff --git a/sys/netinet/ip_var.h b/sys/netinet/ip_var.h index 7ede24ce922..1a43675a7ac 100644 --- a/sys/netinet/ip_var.h +++ b/sys/netinet/ip_var.h @@ -240,6 +240,7 @@ struct mbuf * u_int16_t ip_randomid(void); void ip_send(struct mbuf *); +void ip_send_raw(struct mbuf *); void ip_slowtimo(void); struct mbuf * ip_srcroute(struct mbuf *);