[Qemu-devel] [PATCH v3] slirp: fix ICMP handling on macOS hosts

[Qemu-devel] [PATCH v3] slirp: fix ICMP handling on macOS hosts

[Andrew Oates]

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) {
+        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

Re: [Qemu-devel] [PATCH v3] slirp: fix ICMP handling on macOS hosts

[Samuel Thibault]

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
> 

-- 
Samuel
As usual, this being a 1.3.x release, I haven't even compiled this
kernel yet.  So if it works, you should be doubly impressed.
(Linus Torvalds, announcing kernel 1.3.3 on the linux-kernel mailing list.)

Re: [Qemu-devel] [PATCH v3] slirp: fix ICMP handling on macOS hosts

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH v3] slirp: fix ICMP handling on macOS hosts

[Samuel Thibault]

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
> 

-- 
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 -+-

Re: [Qemu-devel] [PATCH v3] slirp: fix ICMP handling on macOS hosts

[Andrew Oates]

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 -+-
>