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

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

[aoates]

From: Andrew Oates <aoates@google.com>

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.

Signed-off-by: Andrew Oates <aoates@google.com>
---
 slirp/ip_icmp.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/slirp/ip_icmp.c b/slirp/ip_icmp.c
index 0b667a429a..5fa67814f4 100644
--- a/slirp/ip_icmp.c
+++ b/slirp/ip_icmp.c
@@ -420,7 +420,15 @@ 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);
+#ifdef CONFIG_DARWIN
+    if (len > 0) {
+        /* Skip the IP header that OS X (unlike Linux) includes. */
+        struct ip *inner_ip = mtod(m, struct ip *);
+        int inner_hlen = inner_ip->ip_hl << 2;
+        memmove(icp, (unsigned char *)icp + inner_hlen, len - inner_hlen);
+    }
+#endif
     icp->icmp_id = id;
 
     m->m_data -= hlen;
-- 
2.17.0

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

[Samuel Thibault]

Hello,

aoates@google.com, le mer. 25 juil. 2018 21:08:12 -0400, a ecrit:
> From: Andrew Oates <aoates@google.com>
> 
> 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.
> 
> Signed-off-by: Andrew Oates <aoates@google.com>
> ---
>  slirp/ip_icmp.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/slirp/ip_icmp.c b/slirp/ip_icmp.c
> index 0b667a429a..5fa67814f4 100644
> --- a/slirp/ip_icmp.c
> +++ b/slirp/ip_icmp.c
> @@ -420,7 +420,15 @@ 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);
> +#ifdef CONFIG_DARWIN
> +    if (len > 0) {
> +        /* Skip the IP header that OS X (unlike Linux) includes. */
> +        struct ip *inner_ip = mtod(m, struct ip *);
> +        int inner_hlen = inner_ip->ip_hl << 2;
> +        memmove(icp, (unsigned char *)icp + inner_hlen, len - inner_hlen);

Please also check that the provided inner_hlen is not bigger than len.
(otherwise it'd be a security issue). Also, substract inner_len from len
for coherency. In case inner_len is bigger than len, you'd set len to -1
and set errno to EINVAL, handled below.

Thanks,
Samuel

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

[Andrew Oates]

On Sat, Jul 28, 2018, 13:53 Samuel Thibault <samuel.thibault@gnu.org> wrote:

> Hello,
>
> aoates@google.com, le mer. 25 juil. 2018 21:08:12 -0400, a ecrit:
> > From: Andrew Oates <aoates@google.com>
> >
> > 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.
> >
> > Signed-off-by: Andrew Oates <aoates@google.com>
> > ---
> >  slirp/ip_icmp.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/slirp/ip_icmp.c b/slirp/ip_icmp.c
> > index 0b667a429a..5fa67814f4 100644
> > --- a/slirp/ip_icmp.c
> > +++ b/slirp/ip_icmp.c
> > @@ -420,7 +420,15 @@ 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);
> > +#ifdef CONFIG_DARWIN
> > +    if (len > 0) {
> > +        /* Skip the IP header that OS X (unlike Linux) includes. */
> > +        struct ip *inner_ip = mtod(m, struct ip *);
> > +        int inner_hlen = inner_ip->ip_hl << 2;
> > +        memmove(icp, (unsigned char *)icp + inner_hlen, len -
> inner_hlen);
>
> Please also check that the provided inner_hlen is not bigger than len.
> (otherwise it'd be a security issue). Also, substract inner_len from len
> for coherency. In case inner_len is bigger than len, you'd set len to -1
> and set errno to EINVAL, handled below.
>

Good catch, thanks! I think that could only happen if there was a bug in
the host kernel, since it generates the inner IP header. I'll send a
revised version.


> Thanks,
> Samuel
>