| Message ID | 20180726010812.5909-1-aoates@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | slirp: fix ICMP handling on macOS hosts | expand |
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
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 >
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;