| Message ID | 20180815023521.185705-1-aoates@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] slirp: fix ICMP handling on macOS hosts | expand |
Andrew Oates, le mar. 14 août 2018 22:35:21 -0400, a ecrit: > On Linux, SOCK_DGRAM+IPPROTO_ICMP sockets give only the ICMP packet when > read from. On macOS, however, the socket acts like a SOCK_RAW socket > and includes the IP header as well. > > This change strips the extra IP header from the received packet on macOS > before sending it to the guest. SOCK_DGRAM ICMP sockets aren't > supported on other BSDs, but we enable this behavior for them as well to > treat the sockets the same as raw sockets. Applied to my tree, thanks! > Signed-off-by: Andrew Oates <aoates@google.com> > --- > v2: check validity of inner_hlen and update len appropriately > v3: CONFIG_DARWIN -> CONFIG_BSD; add comment explaining #ifdef > > slirp/ip_icmp.c | 24 +++++++++++++++++++++++- > 1 file changed, 23 insertions(+), 1 deletion(-) > > diff --git a/slirp/ip_icmp.c b/slirp/ip_icmp.c > index 0b667a429a..0e289fd9d9 100644 > --- a/slirp/ip_icmp.c > +++ b/slirp/ip_icmp.c > @@ -420,7 +420,29 @@ void icmp_receive(struct socket *so) > icp = mtod(m, struct icmp *); > > id = icp->icmp_id; > - len = qemu_recv(so->s, icp, m->m_len, 0); > + len = qemu_recv(so->s, icp, M_ROOM(m), 0); > + /* > + * The behavior of reading SOCK_DGRAM+IPPROTO_ICMP sockets is inconsistent > + * between host OSes. On Linux, only the ICMP header and payload is > + * included. On macOS/Darwin, the socket acts like a raw socket and > + * includes the IP header as well. On other BSDs, SOCK_DGRAM+IPPROTO_ICMP > + * sockets aren't supported at all, so we treat them like raw sockets. It > + * isn't possible to detect this difference at runtime, so we must use an > + * #ifdef to determine if we need to remove the IP header. > + */ > +#ifdef CONFIG_BSD > + if (len > 0) { > + struct ip *inner_ip = mtod(m, struct ip *); > + int inner_hlen = inner_ip->ip_hl << 2; > + if (inner_hlen > len) { > + len = -1; > + errno = -EINVAL; > + } else { > + len -= inner_hlen; > + memmove(icp, (unsigned char *)icp + inner_hlen, len); > + } > + } > +#endif > icp->icmp_id = id; > > m->m_data -= hlen; > -- > 2.18.0.865.gffc8e1a3cd6-goog >
On 15 August 2018 at 03:35, Andrew Oates <aoates@google.com> wrote: > On Linux, SOCK_DGRAM+IPPROTO_ICMP sockets give only the ICMP packet when > read from. On macOS, however, the socket acts like a SOCK_RAW socket > and includes the IP header as well. > > This change strips the extra IP header from the received packet on macOS > before sending it to the guest. SOCK_DGRAM ICMP sockets aren't > supported on other BSDs, but we enable this behavior for them as well to > treat the sockets the same as raw sockets. > > Signed-off-by: Andrew Oates <aoates@google.com> > --- > v2: check validity of inner_hlen and update len appropriately > v3: CONFIG_DARWIN -> CONFIG_BSD; add comment explaining #ifdef > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
Andrew Oates, le mar. 14 août 2018 22:35:21 -0400, a ecrit: > On Linux, SOCK_DGRAM+IPPROTO_ICMP sockets give only the ICMP packet when > read from. On macOS, however, the socket acts like a SOCK_RAW socket > and includes the IP header as well. > > This change strips the extra IP header from the received packet on macOS > before sending it to the guest. SOCK_DGRAM ICMP sockets aren't > supported on other BSDs, but we enable this behavior for them as well to > treat the sockets the same as raw sockets. > > Signed-off-by: Andrew Oates <aoates@google.com> > --- > v2: check validity of inner_hlen and update len appropriately > v3: CONFIG_DARWIN -> CONFIG_BSD; add comment explaining #ifdef > > slirp/ip_icmp.c | 24 +++++++++++++++++++++++- > 1 file changed, 23 insertions(+), 1 deletion(-) > > diff --git a/slirp/ip_icmp.c b/slirp/ip_icmp.c > index 0b667a429a..0e289fd9d9 100644 > --- a/slirp/ip_icmp.c > +++ b/slirp/ip_icmp.c > @@ -420,7 +420,29 @@ void icmp_receive(struct socket *so) > icp = mtod(m, struct icmp *); > > id = icp->icmp_id; > - len = qemu_recv(so->s, icp, m->m_len, 0); > + len = qemu_recv(so->s, icp, M_ROOM(m), 0); > + /* > + * The behavior of reading SOCK_DGRAM+IPPROTO_ICMP sockets is inconsistent > + * between host OSes. On Linux, only the ICMP header and payload is > + * included. On macOS/Darwin, the socket acts like a raw socket and > + * includes the IP header as well. On other BSDs, SOCK_DGRAM+IPPROTO_ICMP > + * sockets aren't supported at all, so we treat them like raw sockets. It > + * isn't possible to detect this difference at runtime, so we must use an > + * #ifdef to determine if we need to remove the IP header. > + */ > +#ifdef CONFIG_BSD > + if (len > 0) { Looking at it again, this used to be if (len >= sizeof(struct ip)) { shouldn't be it that way so that > + struct ip *inner_ip = mtod(m, struct ip *); > + int inner_hlen = inner_ip->ip_hl << 2; Reading in the header doesn't give uninitialized values? I guess that to be on the safe side and trigger explicit warnings, we should reject with EINVAL packets which are smaller than an IP header? > + if (inner_hlen > len) { > + len = -1; > + errno = -EINVAL; > + } else { > + len -= inner_hlen; > + memmove(icp, (unsigned char *)icp + inner_hlen, len); > + } > + } > +#endif > icp->icmp_id = id; > > m->m_data -= hlen; > -- > 2.18.0.865.gffc8e1a3cd6-goog >
On Wed, Aug 15, 2018 at 7:03 AM Samuel Thibault <samuel.thibault@gnu.org> wrote: > Andrew Oates, le mar. 14 août 2018 22:35:21 -0400, a ecrit: > > On Linux, SOCK_DGRAM+IPPROTO_ICMP sockets give only the ICMP packet when > > read from. On macOS, however, the socket acts like a SOCK_RAW socket > > and includes the IP header as well. > > > > This change strips the extra IP header from the received packet on macOS > > before sending it to the guest. SOCK_DGRAM ICMP sockets aren't > > supported on other BSDs, but we enable this behavior for them as well to > > treat the sockets the same as raw sockets. > > > > Signed-off-by: Andrew Oates <aoates@google.com> > > --- > > v2: check validity of inner_hlen and update len appropriately > > v3: CONFIG_DARWIN -> CONFIG_BSD; add comment explaining #ifdef > > > > slirp/ip_icmp.c | 24 +++++++++++++++++++++++- > > 1 file changed, 23 insertions(+), 1 deletion(-) > > > > diff --git a/slirp/ip_icmp.c b/slirp/ip_icmp.c > > index 0b667a429a..0e289fd9d9 100644 > > --- a/slirp/ip_icmp.c > > +++ b/slirp/ip_icmp.c > > @@ -420,7 +420,29 @@ void icmp_receive(struct socket *so) > > icp = mtod(m, struct icmp *); > > > > id = icp->icmp_id; > > - len = qemu_recv(so->s, icp, m->m_len, 0); > > + len = qemu_recv(so->s, icp, M_ROOM(m), 0); > > + /* > > + * The behavior of reading SOCK_DGRAM+IPPROTO_ICMP sockets is > inconsistent > > + * between host OSes. On Linux, only the ICMP header and payload is > > + * included. On macOS/Darwin, the socket acts like a raw socket and > > + * includes the IP header as well. On other BSDs, > SOCK_DGRAM+IPPROTO_ICMP > > + * sockets aren't supported at all, so we treat them like raw > sockets. It > > + * isn't possible to detect this difference at runtime, so we must > use an > > + * #ifdef to determine if we need to remove the IP header. > > + */ > > +#ifdef CONFIG_BSD > > + if (len > 0) { > > Looking at it again, this used to be > > if (len >= sizeof(struct ip)) { > > shouldn't be it that way so that > > > + struct ip *inner_ip = mtod(m, struct ip *); > > + int inner_hlen = inner_ip->ip_hl << 2; > > Reading in the header doesn't give uninitialized values? > > I guess that to be on the safe side and trigger explicit warnings, we > should reject with EINVAL packets which are smaller than an IP header? > Hmm, yeah, sorry. That got reverted in my patch and I didn't notice. Will re-send. > > > + if (inner_hlen > len) { > > + len = -1; > > + errno = -EINVAL; > > + } else { > > + len -= inner_hlen; > > + memmove(icp, (unsigned char *)icp + inner_hlen, len); > > + } > > + } > > +#endif > > icp->icmp_id = id; > > > > m->m_data -= hlen; > > -- > > 2.18.0.865.gffc8e1a3cd6-goog > > > > -- > Samuel > <N> (* If you have a precise idea of the intended use of the following > code, please > <N> write to Eduardo.Gimenez@inria.fr and ask for the prize :-) > <N> -- Eduardo (11/8/97) *) > -+- N sur #ens-mim - et c'était un des développeurs -+- >
diff --git a/slirp/ip_icmp.c b/slirp/ip_icmp.c index 0b667a429a..0e289fd9d9 100644 --- a/slirp/ip_icmp.c +++ b/slirp/ip_icmp.c @@ -420,7 +420,29 @@ void icmp_receive(struct socket *so) icp = mtod(m, struct icmp *); id = icp->icmp_id; - len = qemu_recv(so->s, icp, m->m_len, 0); + len = qemu_recv(so->s, icp, M_ROOM(m), 0); + /* + * The behavior of reading SOCK_DGRAM+IPPROTO_ICMP sockets is inconsistent + * between host OSes. On Linux, only the ICMP header and payload is + * included. On macOS/Darwin, the socket acts like a raw socket and + * includes the IP header as well. On other BSDs, SOCK_DGRAM+IPPROTO_ICMP + * sockets aren't supported at all, so we treat them like raw sockets. It + * isn't possible to detect this difference at runtime, so we must use an + * #ifdef to determine if we need to remove the IP header. + */ +#ifdef CONFIG_BSD + if (len > 0) { + struct ip *inner_ip = mtod(m, struct ip *); + int inner_hlen = inner_ip->ip_hl << 2; + if (inner_hlen > len) { + len = -1; + errno = -EINVAL; + } else { + len -= inner_hlen; + memmove(icp, (unsigned char *)icp + inner_hlen, len); + } + } +#endif icp->icmp_id = id; m->m_data -= hlen;
On Linux, SOCK_DGRAM+IPPROTO_ICMP sockets give only the ICMP packet when read from. On macOS, however, the socket acts like a SOCK_RAW socket and includes the IP header as well. This change strips the extra IP header from the received packet on macOS before sending it to the guest. SOCK_DGRAM ICMP sockets aren't supported on other BSDs, but we enable this behavior for them as well to treat the sockets the same as raw sockets. Signed-off-by: Andrew Oates <aoates@google.com> --- v2: check validity of inner_hlen and update len appropriately v3: CONFIG_DARWIN -> CONFIG_BSD; add comment explaining #ifdef slirp/ip_icmp.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-)