> -----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 *);
