Re: [External] : [ICMP] IP options lead to malformed reply
> > I think the changes in ip_insertoptions() can be dropped completely,
> > because the if-statement uses ip-ip_hl, which might not be initialized.
>
> yes, you are right, I think we should rather go for this tweak:
>
> 8<---8<---8<--8<
>
> diff --git a/sys/netinet/ip_output.c b/sys/netinet/ip_output.c
> index e19b744fdf3..5c1222c0c86 100644
> --- a/sys/netinet/ip_output.c
> +++ b/sys/netinet/ip_output.c
> @@ -767,7 +767,7 @@ ip_insertoptions(struct mbuf *m, struct mbuf *opt, int
> *phlen)
> return (m); /* XXX should fail */
>
> /* check if options will fit to IP header */
> - if ((optlen + (ip->ip_hl << 2)) > (0x0f << 2)) {
> + if ((optlen + (sizeof (struct ip))) > (0x0f << 2)) {
> *phlen = sizeof (struct ip);
> return (m);
> }
> 8<---8<---8<--8<
Looks good for me. Just one more remark. In all other failure-cases in
ip_insertoptions() the old mbuf is returned without setting phlen.
Maybe, for the sake of consistence, always or never set phlen in a
failure-case?
ip_output() and icmp_send() do initialize (p)hlen before calling
ip_insertoptions(). So it is not strictly necessary to set it within
ip_insertoptions().
Regards
Dominik
Re: [External] : [ICMP] IP options lead to malformed reply
Hi,
> yes, you are right. below is updated diff I would like to commit.
> >
> > Appart from that, adding a special task seems the way to go.
>
> I think so too. Alternative way would be to pass send flags via m_pkthdr
> in mbuf, however there is no space. We would have to add a new member
> to m_pkthdr. I see such change as too intrusive (given we address a true
> corner case).
I just wanted to reply, but you beat me to it :)
I think the changes in ip_insertoptions() can be dropped completely,
because the if-statement uses ip-ip_hl, which might not be initialized.
Also, in icmp_send(), ip_insertoptions() might return a different mbuf
pointer. Thus, the struct ip pointer must be updated as well, right?
block for refrence (Whole diff below):
if (opts != NULL) {
m = ip_insertoptions(m, opts, &hlen);
ip = mtod(m, struct ip *);
ip->ip_v = IPVERSION;
ip->ip_off &= htons(IP_DF);
ip->ip_id = htons(ip_randomid());
ip->ip_hl = hlen >> 2;
ipstat_inc(ips_localout);
ip_send_raw(m);
}
Regards
Dominik
diff --git a/sys/netinet/ip_icmp.c b/sys/netinet/ip_icmp.c
index a007aa6c2b3..8eb1c9b30e8 100644
--- a/sys/netinet/ip_icmp.c
+++ b/sys/netinet/ip_icmp.c
@@ -846,10 +846,22 @@ 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 = mtod(m, struct ip *);
+
+ ip->ip_v = IPVERSION;
+ ip->ip_off &= htons(IP_DF);
+ ip->ip_id = htons(ip_randomid());
+ ip->ip_hl = hlen >> 2;
+ 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 @@ intip_dooptions(struct mbuf *, struct ifnet *);
intin_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_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);
voidip_send(struct mbuf *);
+voidip_send_raw(struct mbuf *);
voidip_slowtimo(void);
struct mbuf *
ip_srcroute(struct mbuf *);
Re: [External] : [ICMP] IP options lead to malformed reply
> -Original Message-
> From: Alexandr Nedvedicky
> Sent: Donnerstag, 25. März 2021 17:36
> To: Schreilechner, Dominik (RC-AT DI FA DH-GRAZ ICO)
>
> Cc: tech@openbsd.org
> Subject: Re: [External] : [ICMP] IP options lead to malformed reply
>
> Hello,
>
>
> > > 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 @@ intip_dooptions(struct mbuf *, struct ifnet *);
intin_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
Re: [External] : [ICMP] IP options lead to malformed reply
> -Original Message-
> From: Alexandr Nedvedicky
> Sent: Mittwoch, 24. März 2021 23:09
> To: Schreilechner, Dominik (RC-AT DI FA DH-GRAZ ICO)
>
> Cc: tech@openbsd.org
> Subject: Re: [External] : [ICMP] IP options lead to malformed reply
>
> Hello,
>
>
>
> > We really need to fix ip_send() such the output task will handle IP
> options
> > properly.
>
> took a look at bug reported by Dominik earlier today. Looks like
> there are two issues:
>
> 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).
>
> Diff below fixes both issues. We still have no good story when
> ip_insertoptions() fails. I'll send another diff later this week.
>
>
> diff below makes 'nping --ip-options T ...' to work. OK?
>
> thanks and
> regards
> sashan
Regards
Dominik
>
> 8<---8<---8<--8<
> diff --git a/sys/netinet/ip_input.c b/sys/netinet/ip_input.c
> index 0ec3f723be4..234c798e7d4 100644
> --- a/sys/netinet/ip_input.c
> +++ b/sys/netinet/ip_input.c
> @@ -1789,7 +1789,7 @@ 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, IP_RAWOUTPUT, NULL, NULL,
> 0);
> }
> NET_UNLOCK();
> }
> 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);
> }
>
[ICMP] IP options lead to malformed reply
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"
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://github.com/openbsd/src/commit/528ff3946710c3940efc90589f62c714f31fb812
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
dhclient requested ip address in decline message
Hi,
If the dhclient receives an OFFER or ACK, that does not contain all required
parameters, a DECLINE is send. This DECLINE has the 'Requested IP Address'
(DHO_DHCP_REQUESTET_ADDRESS) set to 0 instead of using the client IP address
(yiaddr) from the packet. As far as I see it, the 'Requested IP Address' is the
address the dhclient is declining, so the yiaddr would make more sense than 0.
This happens in dhclient.c::packet_to_lease() and only if not all required
parameters are in the packet. For all other cases the 'Requested IP Address' of
the DECLINE is set to the yiaddr from the OFFER / ACK packet. My fix would be
to store (and check) the yiaddr from the packet before the first jump to the
decline label occurs. I have included a diff below.
Best Regards,
Dominik
diff --git a/sbin/dhclient/dhclient.c b/sbin/dhclient/dhclient.c
index 007358c5008..b287c19a3b2 100644
--- a/sbin/dhclient/dhclient.c
+++ b/sbin/dhclient/dhclient.c
@@ -1257,18 +1257,6 @@ packet_to_lease(struct interface_info *ifi, struct
option_data *options)
options[i].len = 0;
}
- /*
-* If this lease doesn't supply a required parameter, decline it.
-*/
- for (i = 0; i < config->required_option_count; i++) {
- if (lease->options[config->required_options[i]].len == 0) {
- name = code_to_name(config->required_options[i]);
- log_warnx("%s: %s required but missing", log_procname,
- name);
- goto decline;
- }
- }
-
/*
* If this lease is trying to sell us an address we are already
* using, decline it.
@@ -1282,6 +1270,18 @@ packet_to_lease(struct interface_info *ifi, struct
option_data *options)
goto decline;
}
+ /*
+* If this lease doesn't supply a required parameter, decline it.
+*/
+ for (i = 0; i < config->required_option_count; i++) {
+ if (lease->options[config->required_options[i]].len == 0) {
+ name = code_to_name(config->required_options[i]);
+ log_warnx("%s: %s required but missing", log_procname,
+ name);
+ goto decline;
+ }
+ }
+
/* Save the siaddr (a.k.a. next-server) info. */
lease->next_server.s_addr = packet->siaddr.s_addr;
Re: VFS vgone(l) manpage vs. code
> From: Jason McIntyre, Mittwoch, 29. Juli 2020 14:23
>
> On Wed, Jul 29, 2020 at 11:37:03AM +, Schreilechner, Dominik wrote:
> > Hi,
> >
> > I noticed, that the description of vgone/vgonel in the manpage does not
> > match the \
> > code. The manpage (https://man.openbsd.org/vgone) states:
> > The difference between vgone() and vgonel() is that vgone() locks the vnode
> > \
> > interlock and then calls vgonel() while vgonel() expects the interlock to
> > already \
> > be locked.
> > However, vgone simply calls vgonel with curproc (see vfs_subr.c).
> > void
> > vgone(struct vnode *vp)
> > {
> > struct proc *p = curproc;
> > vgonel(vp, p);
> > }
> >
> > Best regards,
> > Dominik
> >
>
> hi.
>
> it would be easier to get things improved if you attached a diff
> describing what you think is correct. even failing a diff, a piece of
> text saying what you think it should be would increase your chances.
>
> jmc
Sorry, I was not sure about that, but worst case I am wrong, I guess.
Below is a diff of the man page, that describes the current difference between
vgone and vgonel as it is in the code.
Best regards,
Dominik
diff --git a/share/man/man9/vgone.9 b/share/man/man9/vgone.9
index 3a663b582df..b3436fd9148 100644
--- a/share/man/man9/vgone.9
+++ b/share/man/man9/vgone.9
@@ -49,17 +49,13 @@ prepare a vnode for reuse by another file system.
The preparation includes the cleaning of all file system specific data and
the removal from its mount point vnode list.
.Pp
-The difference between
.Fn vgone
-and
-.Fn vgonel
-is that
-.Fn vgone
-locks the vnode interlock and then calls
-.Fn vgonel
-while
+is the same as
.Fn vgonel
-expects the interlock to already be locked.
+with
+.Fa p
+set to the current process. Historically, vgonel was called with a locked vnode
+interlock, but that lock was removed.
.Sh SEE ALSO
.Xr vclean 9 ,
.Xr vnode 9 ,
VFS vgone(l) manpage vs. code
Hi,
I noticed, that the description of vgone/vgonel in the manpage does not match
the code.
The manpage (https://man.openbsd.org/vgone) states:
The difference between vgone() and vgonel() is that vgone() locks the vnode
interlock and then calls vgonel() while vgonel() expects the interlock to
already be locked.
However, vgone simply calls vgonel with curproc (see vfs_subr.c).
void
vgone(struct vnode *vp)
{
struct proc *p = curproc;
vgonel(vp, p);
}
Best regards,
Dominik
