Hello Dominik, thanks for reporting a bug. I'll take a look at it later today.
your proposed fix re-introduces a recursion to PF, which we want to avoid, hence we let icmp_send() to dispatch outbound ICMP response to task. We really need to fix ip_send() such the output task will handle IP options properly. thanks and regards sashan On Wed, Mar 24, 2021 at 01:36:20PM +0000, Schreilechner, Dominik wrote: > Hi, > > When sending an ICMP request with IP options to an OpenBSD Box, the > ICMP payload of the reply is malformed. In the reply the length field > of the IP header is 20, even though the reply contains the IP options. > Meaning, in this packet the IP options are part of ICMP and not IP. > This was tested with OpenBSD 6.8 and Nmap's Nping command: > nping --ip-options "T" <destination IP> > > The problem is not specific to this IP option, but it can be reproduced > with others as well. > > The root cause of the issue seems to be that the IP options of the ICMP > reply are added in icmp_send() and not in ip_output(). icmp_send() will > forward the packet to the softnet task via ip_send(). The softnet task > will then call ip_output(m, ...). Here all arguments to ip_output are > NULL or 0, except for the mbuf containing the IP packet. Thus, the opt > mbuf is NULL as well. > ip_output() cannot assume that the ip->ip_hl field is initialized by > its caller. Therefore, the header length is set to the default 20 bytes > and only if an opt mbuf is passed to ip_output(), the options are > accounted for in the header length. In other words, all packets passed > to ip_send() will be send with an IP header length of 20 bytes, > regardless if they contain IP options or not. > > I think that the problem exists since this commit: > https://urldefense.com/v3/__https://github.com/openbsd/src/commit/528ff3946710c3940efc90589f62c714f31fb812__;!!GqivPVa7Brio!JsIZviL72HJW1cGSiq3PyhzGSGLom2qAQI5JOPj8H_ZwR5yE-ksAGF92-lCfjJDvuoOsv4Gq$ > > > For ICMP a solution would be to replace ip_send() with ip_output(), as > it was before the commit above. A quick grep suggests that ICMP is the > only caller of ip_send(), that uses IP options. However, I am not sure > what other implications this change has. (Anyways, diff below) > > Another way would be to modify ip_send(), so that it additionally has > an option-mbuf as parameter. But as far as I can tell, this means the > mbuf_queue structure and its related functions cannot be used and a new > queue is needed, that holds two mbufs (the packet and the options) per > entry. Which means, even more changes... > > Regards > Dominik > > diff --git a/sys/netinet/ip_icmp.c b/sys/netinet/ip_icmp.c > index a007aa6c2b3..b4bb2bb7f6f 100644 > --- a/sys/netinet/ip_icmp.c > +++ b/sys/netinet/ip_icmp.c > @@ -846,10 +846,7 @@ icmp_send(struct mbuf *m, struct mbuf *opts) > printf("icmp_send dst %s src %s\n", dst, src); > } > #endif > - if (opts != NULL) > - m = ip_insertoptions(m, opts, &hlen); > - > - ip_send(m); > + ip_output(m, opts, NULL, 0, NULL, NULL, 0); > } > > u_int32_t >
