[Qemu-devel] [PATCH 0/4] slirp: Fixes for restricted mode and more

[Qemu-devel] [PATCH 0/4] slirp: Fixes for restricted mode and more

[Jan Kiszka]

For isolating slirp-attached guests from the world, QEMU provides the
restricted mode. However, its implementation suffers from bugs that
makes it practically unusable. Most probablematic is broken DHCP.

This series fixes that and canonicalizes the corresponding command line
switch. It also cleans up the addressing of the built-in DHCP and TFTP
server and drops the pointless m_freem wrapper.

Please review/merge.

CC: Gleb Natapov <gleb@redhat.com>

Jan Kiszka (4):
  slirp: Fix restricted mode
  slirp: Canonicalize restrict syntax
  slirp: Strictly associate DHCP/BOOTP and TFTP with virtual host
  slirp: Replace m_freem with m_free

 net/slirp.c       |   21 +++++++++++++++------
 qemu-options.hx   |    4 ++--
 slirp/ip_icmp.c   |    8 +++++---
 slirp/ip_input.c  |   29 ++++-------------------------
 slirp/ip_output.c |    4 ++--
 slirp/mbuf.h      |    3 ---
 slirp/tcp_input.c |   10 +++++-----
 slirp/tcp_subr.c  |    2 +-
 slirp/udp.c       |   23 +++++++++++++----------
 9 files changed, 47 insertions(+), 57 deletions(-)

[Qemu-devel] [PATCH 1/4] slirp: Fix restricted mode

[Jan Kiszka]

This aligns the code to what the documentation claims: Allow everything
but requests that would have to be routed outside of the virtual LAN.

So we need to drop the unneeded IP-level filter, allow TFTP requests,
and add the missing protocol-level filter to ICMP.

CC: Gleb Natapov <gleb@redhat.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 slirp/ip_icmp.c  |    2 ++
 slirp/ip_input.c |   21 ---------------------
 slirp/udp.c      |    8 ++++----
 3 files changed, 6 insertions(+), 25 deletions(-)

diff --git a/slirp/ip_icmp.c b/slirp/ip_icmp.c
index 751a8e2..0cd129c 100644
--- a/slirp/ip_icmp.c
+++ b/slirp/ip_icmp.c
@@ -101,6 +101,8 @@ icmp_input(struct mbuf *m, int hlen)
     ip->ip_len += hlen;	             /* since ip_input subtracts this */
     if (ip->ip_dst.s_addr == slirp->vhost_addr.s_addr) {
       icmp_reflect(m);
+    } else if (slirp->restricted) {
+        goto freeit;
     } else {
       struct socket *so;
       struct sockaddr_in addr;
diff --git a/slirp/ip_input.c b/slirp/ip_input.c
index 768ab0c..2ff6adb 100644
--- a/slirp/ip_input.c
+++ b/slirp/ip_input.c
@@ -118,27 +118,6 @@ ip_input(struct mbuf *m)
 		goto bad;
 	}
 
-    if (slirp->restricted) {
-        if ((ip->ip_dst.s_addr & slirp->vnetwork_mask.s_addr) ==
-            slirp->vnetwork_addr.s_addr) {
-            if (ip->ip_dst.s_addr == 0xffffffff && ip->ip_p != IPPROTO_UDP)
-                goto bad;
-        } else {
-            uint32_t inv_mask = ~slirp->vnetwork_mask.s_addr;
-            struct ex_list *ex_ptr;
-
-            if ((ip->ip_dst.s_addr & inv_mask) == inv_mask) {
-                goto bad;
-            }
-            for (ex_ptr = slirp->exec_list; ex_ptr; ex_ptr = ex_ptr->ex_next)
-                if (ex_ptr->ex_addr.s_addr == ip->ip_dst.s_addr)
-                    break;
-
-            if (!ex_ptr)
-                goto bad;
-        }
-    }
-
 	/* Should drop packet if mbuf too long? hmmm... */
 	if (m->m_len > ip->ip_len)
 	   m_adj(m, ip->ip_len - m->m_len);
diff --git a/slirp/udp.c b/slirp/udp.c
index 02b3793..f1a9a10 100644
--- a/slirp/udp.c
+++ b/slirp/udp.c
@@ -125,10 +125,6 @@ udp_input(register struct mbuf *m, int iphlen)
             goto bad;
         }
 
-        if (slirp->restricted) {
-            goto bad;
-        }
-
         /*
          *  handle TFTP
          */
@@ -137,6 +133,10 @@ udp_input(register struct mbuf *m, int iphlen)
             goto bad;
         }
 
+        if (slirp->restricted) {
+            goto bad;
+        }
+
 	/*
 	 * Locate pcb for datagram.
 	 */
-- 
1.7.1

[Qemu-devel] [PATCH 2/4] slirp: Canonicalize restrict syntax

[Jan Kiszka]

All other boolean arguments accept on|off - except for slirp's restrict.
Fix that while still accepting the formerly allowed yes|y|no|n, but
reject everything else. This avoids accidentally allowing external
connections because syntax errors were so far interpreted as
'restrict=no'.

CC: Gleb Natapov <gleb@redhat.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 net/slirp.c     |   21 +++++++++++++++------
 qemu-options.hx |    4 ++--
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/net/slirp.c b/net/slirp.c
index e387a11..c66052a 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -240,7 +240,8 @@ static int net_slirp_init(VLANState *vlan, const char *model,
     nc = qemu_new_net_client(&net_slirp_info, vlan, NULL, model, name);
 
     snprintf(nc->info_str, sizeof(nc->info_str),
-             "net=%s, restricted=%c", inet_ntoa(net), restricted ? 'y' : 'n');
+             "net=%s,restrict=%s", inet_ntoa(net),
+             restricted ? "on" : "off");
 
     s = DO_UPCAST(SlirpState, nc, nc);
 
@@ -689,6 +690,7 @@ int net_init_slirp(QemuOpts *opts,
     const char *bootfile;
     const char *smb_export;
     const char *vsmbsrv;
+    const char *restrict_opt;
     char *vnet = NULL;
     int restricted = 0;
     int ret;
@@ -702,6 +704,18 @@ int net_init_slirp(QemuOpts *opts,
     smb_export  = qemu_opt_get(opts, "smb");
     vsmbsrv     = qemu_opt_get(opts, "smbserver");
 
+    restrict_opt = qemu_opt_get(opts, "restrict");
+    if (restrict_opt) {
+        if (!strcmp(restrict_opt, "on") ||
+            !strcmp(restrict_opt, "yes") || !strcmp(restrict_opt, "y")) {
+            restricted = 1;
+        } else if (strcmp(restrict_opt, "off") &&
+            strcmp(restrict_opt, "no") && strcmp(restrict_opt, "n")) {
+            error_report("invalid option: 'restrict=%s'", restrict_opt);
+            return -1;
+        }
+    }
+
     if (qemu_opt_get(opts, "ip")) {
         const char *ip = qemu_opt_get(opts, "ip");
         int l = strlen(ip) + strlen("/24") + 1;
@@ -720,11 +734,6 @@ int net_init_slirp(QemuOpts *opts,
         vnet = qemu_strdup(qemu_opt_get(opts, "net"));
     }
 
-    if (qemu_opt_get(opts, "restrict") &&
-        qemu_opt_get(opts, "restrict")[0] == 'y') {
-        restricted = 1;
-    }
-
     qemu_opt_foreach(opts, net_init_slirp_configs, NULL, 0);
 
     ret = net_slirp_init(vlan, "user", name, restricted, vnet, vhost,
diff --git a/qemu-options.hx b/qemu-options.hx
index 82e085a..9ccde3e 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1067,7 +1067,7 @@ DEF("net", HAS_ARG, QEMU_OPTION_net,
     "-net nic[,vlan=n][,macaddr=mac][,model=type][,name=str][,addr=str][,vectors=v]\n"
     "                create a new Network Interface Card and connect it to VLAN 'n'\n"
 #ifdef CONFIG_SLIRP
-    "-net user[,vlan=n][,name=str][,net=addr[/mask]][,host=addr][,restrict=y|n]\n"
+    "-net user[,vlan=n][,name=str][,net=addr[/mask]][,host=addr][,restrict=on|off]\n"
     "         [,hostname=host][,dhcpstart=addr][,dns=addr][,tftp=dir][,bootfile=f]\n"
     "         [,hostfwd=rule][,guestfwd=rule]"
 #ifndef _WIN32
@@ -1160,7 +1160,7 @@ either in the form a.b.c.d or as number of valid top-most bits. Default is
 Specify the guest-visible address of the host. Default is the 2nd IP in the
 guest network, i.e. x.x.x.2.
 
-@item restrict=y|yes|n|no
+@item restrict=on|off
 If this options is enabled, the guest will be isolated, i.e. it will not be
 able to contact the host and no guest IP packets will be routed over the host
 to the outside. This option does not affect explicitly set forwarding rule.
-- 
1.7.1

[Qemu-devel] [PATCH 3/4] slirp: Strictly associate DHCP/BOOTP and TFTP with virtual host

[Jan Kiszka]

Instead of accepting every DHCP/BOOTP and TFTP packet, only invoke the
built-in servers if the target is the virtual host.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 slirp/udp.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/slirp/udp.c b/slirp/udp.c
index f1a9a10..cefd50b 100644
--- a/slirp/udp.c
+++ b/slirp/udp.c
@@ -120,15 +120,18 @@ udp_input(register struct mbuf *m, int iphlen)
         /*
          *  handle DHCP/BOOTP
          */
-        if (ntohs(uh->uh_dport) == BOOTP_SERVER) {
-            bootp_input(m);
-            goto bad;
-        }
+        if (ntohs(uh->uh_dport) == BOOTP_SERVER &&
+            (ip->ip_dst.s_addr == slirp->vhost_addr.s_addr ||
+             ip->ip_dst.s_addr == 0xffffffff)) {
+                bootp_input(m);
+                goto bad;
+            }
 
         /*
          *  handle TFTP
          */
-        if (ntohs(uh->uh_dport) == TFTP_SERVER) {
+        if (ntohs(uh->uh_dport) == TFTP_SERVER &&
+            ip->ip_dst.s_addr == slirp->vhost_addr.s_addr) {
             tftp_input(m);
             goto bad;
         }
-- 
1.7.1

[Qemu-devel] [PATCH 4/4] slirp: Replace m_freem with m_free

[Jan Kiszka]

Remove this pointless wrapping.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 slirp/ip_icmp.c   |    6 +++---
 slirp/ip_input.c  |    8 ++++----
 slirp/ip_output.c |    4 ++--
 slirp/mbuf.h      |    3 ---
 slirp/tcp_input.c |   10 +++++-----
 slirp/tcp_subr.c  |    2 +-
 slirp/udp.c       |    2 +-
 7 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/slirp/ip_icmp.c b/slirp/ip_icmp.c
index 0cd129c..4f10826 100644
--- a/slirp/ip_icmp.c
+++ b/slirp/ip_icmp.c
@@ -81,7 +81,7 @@ icmp_input(struct mbuf *m, int hlen)
    */
   if (icmplen < ICMP_MINLEN) {          /* min 8 bytes payload */
   freeit:
-    m_freem(m);
+    m_free(m);
     goto end_error;
   }
 
@@ -155,11 +155,11 @@ icmp_input(struct mbuf *m, int hlen)
   case ICMP_TSTAMP:
   case ICMP_MASKREQ:
   case ICMP_REDIRECT:
-    m_freem(m);
+    m_free(m);
     break;
 
   default:
-    m_freem(m);
+    m_free(m);
   } /* swith */
 
 end_error:
diff --git a/slirp/ip_input.c b/slirp/ip_input.c
index 2ff6adb..46c60b0 100644
--- a/slirp/ip_input.c
+++ b/slirp/ip_input.c
@@ -204,7 +204,7 @@ ip_input(struct mbuf *m)
 	}
 	return;
 bad:
-	m_freem(m);
+	m_free(m);
 	return;
 }
 
@@ -297,7 +297,7 @@ ip_reass(Slirp *slirp, struct ip *ip, struct ipq *fp)
 			break;
 		}
 		q = q->ipf_next;
-		m_freem(dtom(slirp, q->ipf_prev));
+		m_free(dtom(slirp, q->ipf_prev));
 		ip_deq(q->ipf_prev);
 	}
 
@@ -363,7 +363,7 @@ insert:
 	return ip;
 
 dropfrag:
-	m_freem(m);
+	m_free(m);
         return NULL;
 }
 
@@ -379,7 +379,7 @@ ip_freef(Slirp *slirp, struct ipq *fp)
 	for (q = fp->frag_link.next; q != (struct ipasfrag*)&fp->frag_link; q = p) {
 		p = q->ipf_next;
 		ip_deq(q);
-		m_freem(dtom(slirp, q));
+		m_free(dtom(slirp, q));
 	}
 	remque(&fp->ip_link);
 	(void) m_free(dtom(slirp, fp));
diff --git a/slirp/ip_output.c b/slirp/ip_output.c
index 542f318..c82830f 100644
--- a/slirp/ip_output.c
+++ b/slirp/ip_output.c
@@ -159,7 +159,7 @@ sendorfree:
 		if (error == 0)
 			if_output(so, m);
 		else
-			m_freem(m);
+			m_free(m);
 	}
     }
 
@@ -167,6 +167,6 @@ done:
 	return (error);
 
 bad:
-	m_freem(m0);
+	m_free(m0);
 	goto done;
 }
diff --git a/slirp/mbuf.h b/slirp/mbuf.h
index 97729e2..b74544b 100644
--- a/slirp/mbuf.h
+++ b/slirp/mbuf.h
@@ -33,9 +33,6 @@
 #ifndef _MBUF_H_
 #define _MBUF_H_
 
-#define m_freem m_free
-
-
 #define MINCSIZE 4096	/* Amount to increase mbuf if too small */
 
 /*
diff --git a/slirp/tcp_input.c b/slirp/tcp_input.c
index e4a7731..c1214c0 100644
--- a/slirp/tcp_input.c
+++ b/slirp/tcp_input.c
@@ -136,7 +136,7 @@ tcp_reass(register struct tcpcb *tp, register struct tcpiphdr *ti,
 		i = q->ti_seq + q->ti_len - ti->ti_seq;
 		if (i > 0) {
 			if (i >= ti->ti_len) {
-				m_freem(m);
+				m_free(m);
 				/*
 				 * Try to present any queued data
 				 * at the left window edge to the user.
@@ -170,7 +170,7 @@ tcp_reass(register struct tcpcb *tp, register struct tcpiphdr *ti,
 		q = tcpiphdr_next(q);
 		m = tcpiphdr_prev(q)->ti_mbuf;
 		remque(tcpiphdr2qlink(tcpiphdr_prev(q)));
-		m_freem(m);
+		m_free(m);
 	}
 
 	/*
@@ -197,7 +197,7 @@ present:
 		m = ti->ti_mbuf;
 		ti = tcpiphdr_next(ti);
 		if (so->so_state & SS_FCANTSENDMORE)
-			m_freem(m);
+			m_free(m);
 		else {
 			if (so->so_emu) {
 				if (tcp_emu(so,m)) sbappend(so, m);
@@ -451,7 +451,7 @@ findso:
 				acked = ti->ti_ack - tp->snd_una;
 				sbdrop(&so->so_snd, acked);
 				tp->snd_una = ti->ti_ack;
-				m_freem(m);
+				m_free(m);
 
 				/*
 				 * If all outstanding data are acked, stop
@@ -1260,7 +1260,7 @@ dropafterack:
 	 */
 	if (tiflags & TH_RST)
 		goto drop;
-	m_freem(m);
+	m_free(m);
 	tp->t_flags |= TF_ACKNOW;
 	(void) tcp_output(tp);
 	return;
diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
index b661d26..61079b1 100644
--- a/slirp/tcp_subr.c
+++ b/slirp/tcp_subr.c
@@ -250,7 +250,7 @@ tcp_close(struct tcpcb *tp)
 		t = tcpiphdr_next(t);
 		m = tcpiphdr_prev(t)->ti_mbuf;
 		remque(tcpiphdr2qlink(tcpiphdr_prev(t)));
-		m_freem(m);
+		m_free(m);
 	}
 	free(tp);
         so->so_tcpcb = NULL;
diff --git a/slirp/udp.c b/slirp/udp.c
index cefd50b..5b060f3 100644
--- a/slirp/udp.c
+++ b/slirp/udp.c
@@ -222,7 +222,7 @@ udp_input(register struct mbuf *m, int iphlen)
 
 	return;
 bad:
-	m_freem(m);
+	m_free(m);
 	return;
 }
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH 1/4] slirp: Fix restricted mode

[Gleb Natapov]

On Mon, May 23, 2011 at 04:48:16PM +0200, Jan Kiszka wrote:
> This aligns the code to what the documentation claims: Allow everything
> but requests that would have to be routed outside of the virtual LAN.
> 
> So we need to drop the unneeded IP-level filter, allow TFTP requests,
> and add the missing protocol-level filter to ICMP.
> 
May be I am missing something, but how do you disallow requests by
removing code that actually does filtering.

> CC: Gleb Natapov <gleb@redhat.com>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  slirp/ip_icmp.c  |    2 ++
>  slirp/ip_input.c |   21 ---------------------
>  slirp/udp.c      |    8 ++++----
>  3 files changed, 6 insertions(+), 25 deletions(-)
> 
> diff --git a/slirp/ip_icmp.c b/slirp/ip_icmp.c
> index 751a8e2..0cd129c 100644
> --- a/slirp/ip_icmp.c
> +++ b/slirp/ip_icmp.c
> @@ -101,6 +101,8 @@ icmp_input(struct mbuf *m, int hlen)
>      ip->ip_len += hlen;	             /* since ip_input subtracts this */
>      if (ip->ip_dst.s_addr == slirp->vhost_addr.s_addr) {
>        icmp_reflect(m);
> +    } else if (slirp->restricted) {
> +        goto freeit;
>      } else {
>        struct socket *so;
>        struct sockaddr_in addr;
> diff --git a/slirp/ip_input.c b/slirp/ip_input.c
> index 768ab0c..2ff6adb 100644
> --- a/slirp/ip_input.c
> +++ b/slirp/ip_input.c
> @@ -118,27 +118,6 @@ ip_input(struct mbuf *m)
>  		goto bad;
>  	}
>  
> -    if (slirp->restricted) {
> -        if ((ip->ip_dst.s_addr & slirp->vnetwork_mask.s_addr) ==
> -            slirp->vnetwork_addr.s_addr) {
> -            if (ip->ip_dst.s_addr == 0xffffffff && ip->ip_p != IPPROTO_UDP)
> -                goto bad;
> -        } else {
> -            uint32_t inv_mask = ~slirp->vnetwork_mask.s_addr;
> -            struct ex_list *ex_ptr;
> -
> -            if ((ip->ip_dst.s_addr & inv_mask) == inv_mask) {
> -                goto bad;
> -            }
> -            for (ex_ptr = slirp->exec_list; ex_ptr; ex_ptr = ex_ptr->ex_next)
> -                if (ex_ptr->ex_addr.s_addr == ip->ip_dst.s_addr)
> -                    break;
> -
> -            if (!ex_ptr)
> -                goto bad;
> -        }
> -    }
> -
>  	/* Should drop packet if mbuf too long? hmmm... */
>  	if (m->m_len > ip->ip_len)
>  	   m_adj(m, ip->ip_len - m->m_len);
> diff --git a/slirp/udp.c b/slirp/udp.c
> index 02b3793..f1a9a10 100644
> --- a/slirp/udp.c
> +++ b/slirp/udp.c
> @@ -125,10 +125,6 @@ udp_input(register struct mbuf *m, int iphlen)
>              goto bad;
>          }
>  
> -        if (slirp->restricted) {
> -            goto bad;
> -        }
> -
>          /*
>           *  handle TFTP
>           */
> @@ -137,6 +133,10 @@ udp_input(register struct mbuf *m, int iphlen)
>              goto bad;
>          }
>  
> +        if (slirp->restricted) {
> +            goto bad;
> +        }
> +
>  	/*
>  	 * Locate pcb for datagram.
>  	 */
> -- 
> 1.7.1

--
			Gleb.

Re: [Qemu-devel] [PATCH 1/4] slirp: Fix restricted mode

[Jan Kiszka]

On 2011-05-24 14:37, Gleb Natapov wrote:
> On Mon, May 23, 2011 at 04:48:16PM +0200, Jan Kiszka wrote:
>> This aligns the code to what the documentation claims: Allow everything
>> but requests that would have to be routed outside of the virtual LAN.
>>
>> So we need to drop the unneeded IP-level filter, allow TFTP requests,
>> and add the missing protocol-level filter to ICMP.
>>
> May be I am missing something, but how do you disallow requests by
> removing code that actually does filtering.

All we need to filter are the per-IP-protocol parts that do the
forwarding via the host IP stack. That does not need to happen at IP level.

Moreover, the existing code contained some practically dead bits anyway:

        if ((ip->ip_dst.s_addr & slirp->vnetwork_mask.s_addr) ==
            slirp->vnetwork_addr.s_addr) {
            if (ip->ip_dst.s_addr == 0xffffffff && ip->ip_p !=
                IPPROTO_UDP)
                goto bad;

This could only trigger if vnetwork_mask.s_addr was 0 (the same applied
to the original code before my refactoring in 2009).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 1/4] slirp: Fix restricted mode

[Gleb Natapov]

On Tue, May 24, 2011 at 02:42:55PM +0200, Jan Kiszka wrote:
> On 2011-05-24 14:37, Gleb Natapov wrote:
> > On Mon, May 23, 2011 at 04:48:16PM +0200, Jan Kiszka wrote:
> >> This aligns the code to what the documentation claims: Allow everything
> >> but requests that would have to be routed outside of the virtual LAN.
> >>
> >> So we need to drop the unneeded IP-level filter, allow TFTP requests,
> >> and add the missing protocol-level filter to ICMP.
> >>
> > May be I am missing something, but how do you disallow requests by
> > removing code that actually does filtering.
> 
> All we need to filter are the per-IP-protocol parts that do the
> forwarding via the host IP stack. That does not need to happen at IP level.
> 
> Moreover, the existing code contained some practically dead bits anyway:
> 
>         if ((ip->ip_dst.s_addr & slirp->vnetwork_mask.s_addr) ==
>             slirp->vnetwork_addr.s_addr) {
>             if (ip->ip_dst.s_addr == 0xffffffff && ip->ip_p !=
>                 IPPROTO_UDP)
>                 goto bad;
> 
> This could only trigger if vnetwork_mask.s_addr was 0 (the same applied
> to the original code before my refactoring in 2009).
> 
Not sure what do you mean by that. This checks that the ip_dst.s_addr is in
the vnetwork range. It does this by comparing net mask bits of ip_dst.s_addr with
vnetwork_addr.s_addr. Grep for vnetwork_mask.s_addr. This idiom is used
many times throughout the code.

--
			Gleb.

Re: [Qemu-devel] [PATCH 1/4] slirp: Fix restricted mode

[Jan Kiszka]

On 2011-05-24 15:01, Gleb Natapov wrote:
> On Tue, May 24, 2011 at 02:42:55PM +0200, Jan Kiszka wrote:
>> On 2011-05-24 14:37, Gleb Natapov wrote:
>>> On Mon, May 23, 2011 at 04:48:16PM +0200, Jan Kiszka wrote:
>>>> This aligns the code to what the documentation claims: Allow everything
>>>> but requests that would have to be routed outside of the virtual LAN.
>>>>
>>>> So we need to drop the unneeded IP-level filter, allow TFTP requests,
>>>> and add the missing protocol-level filter to ICMP.
>>>>
>>> May be I am missing something, but how do you disallow requests by
>>> removing code that actually does filtering.
>>
>> All we need to filter are the per-IP-protocol parts that do the
>> forwarding via the host IP stack. That does not need to happen at IP level.
>>
>> Moreover, the existing code contained some practically dead bits anyway:
>>
>>         if ((ip->ip_dst.s_addr & slirp->vnetwork_mask.s_addr) ==
>>             slirp->vnetwork_addr.s_addr) {
>>             if (ip->ip_dst.s_addr == 0xffffffff && ip->ip_p !=
>>                 IPPROTO_UDP)
>>                 goto bad;
>>
>> This could only trigger if vnetwork_mask.s_addr was 0 (the same applied
>> to the original code before my refactoring in 2009).
>>
> Not sure what do you mean by that. This checks that the ip_dst.s_addr is in
> the vnetwork range. It does this by comparing net mask bits of ip_dst.s_addr with
> vnetwork_addr.s_addr. Grep for vnetwork_mask.s_addr. This idiom is used
> many times throughout the code.

Ok, it's a bit more tricky, and I contributed some buglet. Let

ip_dst.s_addr		= 255.255.255.255
vnetwork_mask.s_addr	= 0.255.255.255
vnetwork_addr.s_addr	= 10.0.2.0
(QEMU's strange defaults)

then dst & vnetwork_mask != vnetwork_addr, so the second condition to
exclude network broadcasts can't trigger.

Your original code matched the first three bytes of dst against the
first three of vnetwork_addr, mine inverted the mask. However, both
variants fail to let DHCP broadcasts pass.

In short, this was always wrong and unneeded as we can (and partly
already did) check for restricted mode in the various IP protocols.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 1/4] slirp: Fix restricted mode

[Gleb Natapov]

On Tue, May 24, 2011 at 03:34:33PM +0200, Jan Kiszka wrote:
> On 2011-05-24 15:01, Gleb Natapov wrote:
> > On Tue, May 24, 2011 at 02:42:55PM +0200, Jan Kiszka wrote:
> >> On 2011-05-24 14:37, Gleb Natapov wrote:
> >>> On Mon, May 23, 2011 at 04:48:16PM +0200, Jan Kiszka wrote:
> >>>> This aligns the code to what the documentation claims: Allow everything
> >>>> but requests that would have to be routed outside of the virtual LAN.
> >>>>
> >>>> So we need to drop the unneeded IP-level filter, allow TFTP requests,
> >>>> and add the missing protocol-level filter to ICMP.
> >>>>
> >>> May be I am missing something, but how do you disallow requests by
> >>> removing code that actually does filtering.
> >>
> >> All we need to filter are the per-IP-protocol parts that do the
> >> forwarding via the host IP stack. That does not need to happen at IP level.
> >>
> >> Moreover, the existing code contained some practically dead bits anyway:
> >>
> >>         if ((ip->ip_dst.s_addr & slirp->vnetwork_mask.s_addr) ==
> >>             slirp->vnetwork_addr.s_addr) {
> >>             if (ip->ip_dst.s_addr == 0xffffffff && ip->ip_p !=
> >>                 IPPROTO_UDP)
> >>                 goto bad;
> >>
> >> This could only trigger if vnetwork_mask.s_addr was 0 (the same applied
> >> to the original code before my refactoring in 2009).
> >>
> > Not sure what do you mean by that. This checks that the ip_dst.s_addr is in
> > the vnetwork range. It does this by comparing net mask bits of ip_dst.s_addr with
> > vnetwork_addr.s_addr. Grep for vnetwork_mask.s_addr. This idiom is used
> > many times throughout the code.
> 
> Ok, it's a bit more tricky, and I contributed some buglet. Let
> 
> ip_dst.s_addr		= 255.255.255.255
> vnetwork_mask.s_addr	= 0.255.255.255
Isn't it 255.0.0.0?

> vnetwork_addr.s_addr	= 10.0.2.0
> (QEMU's strange defaults)
> 
> then dst & vnetwork_mask != vnetwork_addr, so the second condition to
> exclude network broadcasts can't trigger.
> 
> Your original code matched the first three bytes of dst against the
> first three of vnetwork_addr, mine inverted the mask. However, both
> variants fail to let DHCP broadcasts pass.
The original code used memcmp which return 0 when equal. When you
changed it to use variable length mask you also inverted if() condition.
It should be != not == !

The code worked (for some value of 'worked') back then :)

> 
> In short, this was always wrong and unneeded as we can (and partly
> already did) check for restricted mode in the various IP protocols.
> 
Checking of exec_list here and again in tcp_input looks suspiciously
similar. I have to admit I do not remember much about slirp code though.

--
			Gleb.

Re: [Qemu-devel] [PATCH 1/4] slirp: Fix restricted mode

[Jan Kiszka]

On 2011-05-24 16:37, Gleb Natapov wrote:
> On Tue, May 24, 2011 at 03:34:33PM +0200, Jan Kiszka wrote:
>> On 2011-05-24 15:01, Gleb Natapov wrote:
>>> On Tue, May 24, 2011 at 02:42:55PM +0200, Jan Kiszka wrote:
>>>> On 2011-05-24 14:37, Gleb Natapov wrote:
>>>>> On Mon, May 23, 2011 at 04:48:16PM +0200, Jan Kiszka wrote:
>>>>>> This aligns the code to what the documentation claims: Allow everything
>>>>>> but requests that would have to be routed outside of the virtual LAN.
>>>>>>
>>>>>> So we need to drop the unneeded IP-level filter, allow TFTP requests,
>>>>>> and add the missing protocol-level filter to ICMP.
>>>>>>
>>>>> May be I am missing something, but how do you disallow requests by
>>>>> removing code that actually does filtering.
>>>>
>>>> All we need to filter are the per-IP-protocol parts that do the
>>>> forwarding via the host IP stack. That does not need to happen at IP level.
>>>>
>>>> Moreover, the existing code contained some practically dead bits anyway:
>>>>
>>>>         if ((ip->ip_dst.s_addr & slirp->vnetwork_mask.s_addr) ==
>>>>             slirp->vnetwork_addr.s_addr) {
>>>>             if (ip->ip_dst.s_addr == 0xffffffff && ip->ip_p !=
>>>>                 IPPROTO_UDP)
>>>>                 goto bad;
>>>>
>>>> This could only trigger if vnetwork_mask.s_addr was 0 (the same applied
>>>> to the original code before my refactoring in 2009).
>>>>
>>> Not sure what do you mean by that. This checks that the ip_dst.s_addr is in
>>> the vnetwork range. It does this by comparing net mask bits of ip_dst.s_addr with
>>> vnetwork_addr.s_addr. Grep for vnetwork_mask.s_addr. This idiom is used
>>> many times throughout the code.
>>
>> Ok, it's a bit more tricky, and I contributed some buglet. Let
>>
>> ip_dst.s_addr		= 255.255.255.255
>> vnetwork_mask.s_addr	= 0.255.255.255
> Isn't it 255.0.0.0?
> 
>> vnetwork_addr.s_addr	= 10.0.2.0
>> (QEMU's strange defaults)
>>
>> then dst & vnetwork_mask != vnetwork_addr, so the second condition to
>> exclude network broadcasts can't trigger.
>>
>> Your original code matched the first three bytes of dst against the
>> first three of vnetwork_addr, mine inverted the mask. However, both
>> variants fail to let DHCP broadcasts pass.
> The original code used memcmp which return 0 when equal. When you
> changed it to use variable length mask you also inverted if() condition.
> It should be != not == !
> 
> The code worked (for some value of 'worked') back then :)

Mmh, so I really broke - then it's only fair that I'm now fixing it
again. :)

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux