| Message ID | 1350024543-26211-1-git-send-email-mike@dev-zero.net |
|---|---|
| State | New |
| Headers | show |
On 12.10.2012 10:49, Mike Lovell wrote: > /* request a tap device, disable PI, and add vnet header support if > - * requested and it's available. */ > - prep_ifreq(&ifr, "tap%d"); > + * requested and it's available. use ifname if provided for tap name. */ > + prep_ifreq(&ifr, ifname != NULL ? ifname : "tap%d"); Should we check for special symbols here? prep_ifreq() does this: snprintf(ifr->ifr_name, IFNAMSIZ, "%s", ifname); so at least it ensures we have length constraint. Actually I'm not so sure anymore this is a good idea. For example, system may have firewall (iptables) rules in place for, say, future ppp interfaces for ppp clients, and this way we may request the interface to be named pppX and be allowed to send packets where we don't usually have access to. Maybe - at least - require some common prefix for the interfaces created this way, so we'll live in our own, easily distinguishable namespace -- like, qvif* (from Qemu Virtual InterFace)? This is not a simple question really. And the whole bridge helper is quite questionable too. Thanks, /mjt
On 10/12/2012 02:32 AM, Michael Tokarev wrote: > On 12.10.2012 10:49, Mike Lovell wrote: >> /* request a tap device, disable PI, and add vnet header support if >> - * requested and it's available. */ >> - prep_ifreq(&ifr, "tap%d"); >> + * requested and it's available. use ifname if provided for tap name. */ >> + prep_ifreq(&ifr, ifname != NULL ? ifname : "tap%d"); > Should we check for special symbols here? prep_ifreq() does this: > > snprintf(ifr->ifr_name, IFNAMSIZ, "%s", ifname); > > so at least it ensures we have length constraint. I tried the code as is with specifying ifnames with various random combinations of special characters. Some of them we just allowed through, some caused an error when initializing the tap device, and some cause problems in the shell invoking qemu. I think the linux kernel does the necessary checking during the TUNSETIFF ioctl and the qemu-bridge-helper exits with an error if there was a problem. > Actually I'm not so sure anymore this is a good idea. > For example, system may have firewall (iptables) rules > in place for, say, future ppp interfaces for ppp clients, > and this way we may request the interface to be named > pppX and be allowed to send packets where we don't usually > have access to. While I admit this does have that possibility, I'm not sure its a qemu problem. I don't know what the original motivation for the request was but I could see this being the reason for the request. Some administrator sets up firewall rules for a variety of guests ahead of actually running them and needs to specify the interface at runtime. Also, without using the helper programs, the qemu already allows specifying arbitrary names such as ppp0. > Maybe - at least - require some common prefix for the > interfaces created this way, so we'll live in our own, > easily distinguishable namespace -- like, qvif* (from > Qemu Virtual InterFace)? I do like the idea of using a common prefix for the default name of tap devices. Something like "qvif%d" instead of "tap%d" in tap initialization code. But something tells me this could break compatibility with external management software where something might be expecting the interface name to start with tap. mike
On 10/12/2012 12:49 AM, Mike Lovell wrote: > This makes a few changes to allow ifname to be specified when using > qemu-bridge-helper with both the bridge and tap network interfaces. It adds > the --ifname option to qemu-bridge-helper, removes the restriction that ifname > cannot be specified with helper for the tap interface, and adds logic to > specify the --ifname option when exec'ing the helper. > > Signed-off-by: Mike Lovell <mike@dev-zero.net> > --- > > This feature was originally requested by Mario De Chenno on the qemu-devel > mailing list. Seems pretty simple and figured it was something I could throw > together pretty quickly. I have tested the following combinations of invoking > qemu (where qbr is qemu-bridge-helper) > > qemu-system-x86_64 -net nic -net tap,helper="qbr --br=test1" > qemu-system-x86_64 -net nic -net tap,helper="qbr --br=test1",ifname=vm1 > qemu-system-x86_64 -net nic -net tap,helper=qbr > qemu-system-x86_64 -net nic -net tap,helper=qbr,ifname=vm1 > qemu-system-x86_64 -net nic -net bridge,helper=qbr > qemu-system-x86_64 -net nic -net bridge,helper=qbr,ifname=vm1 > qemu-system-x86_64 -net nic -net bridge,helper=qbr,ifname=vm1,br=test1 > qemu-system-x86_64 -net nic -net bridge,helper=qbr,br=test1 > > net/tap.c | 39 ++++++++++++++++++++++++++++----------- > qapi-schema.json | 3 ++- > qemu-bridge-helper.c | 10 +++++++--- > 3 files changed, 37 insertions(+), 15 deletions(-) > > diff --git a/net/tap.c b/net/tap.c > index a88ae8f..cfb5bff 100644 > --- a/net/tap.c > +++ b/net/tap.c > @@ -417,11 +417,13 @@ static int recv_fd(int c) > return len; > } > > -static int net_bridge_run_helper(const char *helper, const char *bridge) > +static int net_bridge_run_helper(const char *helper, > + const char *bridge, > + const char *ifname) > { > sigset_t oldmask, mask; > int pid, status; > - char *args[5]; > + char *args[6]; > char **parg; > int sv[2]; > > @@ -439,7 +441,9 @@ static int net_bridge_run_helper(const char *helper, const char *bridge) > int open_max = sysconf(_SC_OPEN_MAX), i; > char fd_buf[6+10]; > char br_buf[6+IFNAMSIZ] = {0}; > - char helper_cmd[PATH_MAX + sizeof(fd_buf) + sizeof(br_buf) + 15]; > + char ifname_buf[10+IFNAMSIZ] = {0}; > + char helper_cmd[PATH_MAX + sizeof(fd_buf) + sizeof(br_buf) + > + sizeof(ifname_buf) + 15]; > > for (i = 0; i < open_max; i++) { > if (i != STDIN_FILENO && > @@ -459,8 +463,13 @@ static int net_bridge_run_helper(const char *helper, const char *bridge) > snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge); > } > > - snprintf(helper_cmd, sizeof(helper_cmd), "%s %s %s %s", > - helper, "--use-vnet", fd_buf, br_buf); > + if ((strstr(helper, "--ifname=") == NULL) && (ifname != NULL)) { > + snprintf(ifname_buf, sizeof(ifname_buf), "%s%s" , > + "--ifname=", ifname); > + } > + > + snprintf(helper_cmd, sizeof(helper_cmd), "%s %s %s %s %s", > + helper, "--use-vnet", fd_buf, br_buf, ifname_buf); > > parg = args; > *parg++ = (char *)"sh"; > @@ -473,12 +482,17 @@ static int net_bridge_run_helper(const char *helper, const char *bridge) > /* assume helper is just the executable path name */ > > snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge); > + if (ifname != NULL) { > + snprintf(ifname_buf, sizeof(ifname_buf), "%s%s" , > + "--ifname=", ifname); > + } > > parg = args; > *parg++ = (char *)helper; > *parg++ = (char *)"--use-vnet"; > *parg++ = fd_buf; > *parg++ = br_buf; > + *parg++ = ifname_buf; > *parg++ = NULL; > > execv(helper, args); > @@ -517,7 +531,7 @@ int net_init_bridge(const NetClientOptions *opts, const char *name, > NetClientState *peer) > { > const NetdevBridgeOptions *bridge; > - const char *helper, *br; > + const char *helper, *br, *ifname; > > TAPState *s; > int fd, vnet_hdr; > @@ -527,8 +541,9 @@ int net_init_bridge(const NetClientOptions *opts, const char *name, > > helper = bridge->has_helper ? bridge->helper : DEFAULT_BRIDGE_HELPER; > br = bridge->has_br ? bridge->br : DEFAULT_BRIDGE_INTERFACE; > + ifname = bridge->has_ifname ? bridge->ifname : NULL; > > - fd = net_bridge_run_helper(helper, br); > + fd = net_bridge_run_helper(helper, br, ifname); > if (fd == -1) { > return -1; > } > @@ -622,14 +637,16 @@ int net_init_tap(const NetClientOptions *opts, const char *name, > model = "tap"; > > } else if (tap->has_helper) { > - if (tap->has_ifname || tap->has_script || tap->has_downscript || > - tap->has_vnet_hdr) { > - error_report("ifname=, script=, downscript=, and vnet_hdr= " > + if (tap->has_script || tap->has_downscript || tap->has_vnet_hdr) { > + error_report("script=, downscript=, and vnet_hdr= " > "are invalid with helper="); > return -1; > } > > - fd = net_bridge_run_helper(tap->helper, DEFAULT_BRIDGE_INTERFACE); > + const char *ifname; > + ifname = tap->has_ifname ? tap->ifname : NULL; > + fd = net_bridge_run_helper(tap->helper, DEFAULT_BRIDGE_INTERFACE, > + ifname); > if (fd == -1) { > return -1; > } > diff --git a/qapi-schema.json b/qapi-schema.json > index f9dbdae..feaac9e 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -2432,7 +2432,8 @@ > { 'type': 'NetdevBridgeOptions', > 'data': { > '*br': 'str', > - '*helper': 'str' } } > + '*helper': 'str', > + '*ifname': 'str' } } > > ## > # @NetdevHubPortOptions > diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c > index 652eec9..c1d1519 100644 > --- a/qemu-bridge-helper.c > +++ b/qemu-bridge-helper.c > @@ -67,7 +67,8 @@ typedef QSIMPLEQ_HEAD(ACLList, ACLRule) ACLList; > static void usage(void) > { > fprintf(stderr, > - "Usage: qemu-bridge-helper [--use-vnet] --br=bridge --fd=unixfd\n"); > + "Usage: qemu-bridge-helper [--use-vnet] [--ifname=name] " > + "--br=bridge --fd=unixfd\n"); > } > > static int parse_acl_file(const char *filename, ACLList *acl_list) > @@ -239,6 +240,7 @@ int main(int argc, char **argv) > ACLList acl_list; > int access_allowed, access_denied; > int ret = EXIT_SUCCESS; > + const char *ifname = NULL; > > #ifdef CONFIG_LIBCAP > /* if we're run from an suid binary, immediately drop privileges preserving > @@ -259,6 +261,8 @@ int main(int argc, char **argv) > bridge = &argv[index][5]; > } else if (strncmp(argv[index], "--fd=", 5) == 0) { > unixfd = atoi(&argv[index][5]); > + } else if (strncmp(argv[index], "--ifname=", 9) == 0) { > + ifname = &argv[index][9]; > } else { > usage(); > return EXIT_FAILURE; > @@ -329,8 +333,8 @@ int main(int argc, char **argv) > } > > /* request a tap device, disable PI, and add vnet header support if > - * requested and it's available. */ > - prep_ifreq(&ifr, "tap%d"); > + * requested and it's available. use ifname if provided for tap name. */ > + prep_ifreq(&ifr, ifname != NULL ? ifname : "tap%d"); > ifr.ifr_flags = IFF_TAP|IFF_NO_PI; > if (use_vnet && has_vnet_hdr(fd)) { > ifr.ifr_flags |= IFF_VNET_HDR; ping ... or syn. any other thoughts about this? mike
Somehow I missed this email initially.. replying now. On 12.10.2012 22:04, Mike Lovell wrote: > On 10/12/2012 02:32 AM, Michael Tokarev wrote: >> On 12.10.2012 10:49, Mike Lovell wrote: >>> /* request a tap device, disable PI, and add vnet header support if >>> - * requested and it's available. */ >>> - prep_ifreq(&ifr, "tap%d"); >>> + * requested and it's available. use ifname if provided for tap name. */ >>> + prep_ifreq(&ifr, ifname != NULL ? ifname : "tap%d"); >> Should we check for special symbols here? prep_ifreq() does this: >> >> snprintf(ifr->ifr_name, IFNAMSIZ, "%s", ifname); >> >> so at least it ensures we have length constraint. > > I tried the code as is with specifying ifnames with various random combinations of special characters. Some of them we just allowed through, some caused an error when initializing the tap device, and some cause problems in the shell invoking qemu. I think the linux kernel does the necessary checking during the TUNSETIFF ioctl and the qemu-bridge-helper exits with an error if there was a problem. > >> Actually I'm not so sure anymore this is a good idea. >> For example, system may have firewall (iptables) rules >> in place for, say, future ppp interfaces for ppp clients, >> and this way we may request the interface to be named >> pppX and be allowed to send packets where we don't usually >> have access to. > > While I admit this does have that possibility, I'm not sure its a qemu problem. I don't know what the original motivation for the request was but I could see this being the reason for the request. Some administrator sets up firewall rules for a variety of guests ahead of actually running them and needs to specify the interface at runtime. Also, without using the helper programs, the qemu already allows specifying arbitrary names such as ppp0. qemu allows arbitrary names, yes, but it does not have extra permissions to create them, -- only ones of the current user. The helper, on the other hand, does have extra privileges which a regular user does not. That's exactly what I was talking about. Maybe _always_ having a common prefix is a good idea after all, with --name=FOO appended to it, like qvifFOO. Or use --ifnumber=NNN instead of --name (which I dislike). >> Maybe - at least - require some common prefix for the >> interfaces created this way, so we'll live in our own, >> easily distinguishable namespace -- like, qvif* (from >> Qemu Virtual InterFace)? > > I do like the idea of using a common prefix for the default name of tap devices. Something like "qvif%d" instead of "tap%d" in tap initialization code. But something tells me this could break compatibility with external management software where something might be expecting the interface name to start with tap. Does any management interface use this bridge-helper functionality? If it were me, I'd always created the tap fd in the management layer and passed the tap fd# (or at least ifname= of an existing iface) to qemu. Bridge helper is useful for users calling qemu directly, not for management software. Sure, such users are also important - including compatibility. But I don't think current unpredictable tapNN names was a good idea to start with, and that it's good idea to rely on this prefix in firewall rules or whatnot. Thanks, /mjt
Il 30/11/2012 11:02, Michael Tokarev ha scritto: >> I do like the idea of using a common prefix for the default name >> of tap devices. Something like "qvif%d" instead of "tap%d" in tap >> initialization code. But something tells me this could break >> compatibility with external management software where something >> might be expecting the interface name to start with tap. > > Does any management interface use this bridge-helper functionality? Libvirt uses it if you're running libvirtd without privileges. GNOME Boxes is a 1-line patch away from using it, but it's Fedora-specific and not included in any distro. Paolo > If it were me, I'd always created the tap fd in the management layer > and passed the tap fd# (or at least ifname= of an existing iface) to > qemu. Bridge helper is useful for users calling qemu directly, not > for management software. Sure, such users are also important - > including compatibility. But I don't think current unpredictable > tapNN names was a good idea to start with, and that it's good idea to > rely on this prefix in firewall rules or whatnot.
Il 30/11/2012 08:10, Mike Lovell ha scritto: > On 10/12/2012 12:49 AM, Mike Lovell wrote: >> This makes a few changes to allow ifname to be specified when using >> qemu-bridge-helper with both the bridge and tap network interfaces. It >> adds >> the --ifname option to qemu-bridge-helper, removes the restriction >> that ifname >> cannot be specified with helper for the tap interface, and adds logic to >> specify the --ifname option when exec'ing the helper. > > ping ... or syn. any other thoughts about this? I share Michael's perplexity. This feature could be exploitable. If we want to add this, the ifname should be subject to ACL rules just like bridge names. For example you could have a special allow/deny directive "allow foo@" which allows ifnames starting with "foo". Paolo
On Fri, Nov 30, 2012 at 03:35:46PM +0100, Paolo Bonzini wrote: > Il 30/11/2012 08:10, Mike Lovell ha scritto: > > On 10/12/2012 12:49 AM, Mike Lovell wrote: > >> This makes a few changes to allow ifname to be specified when using > >> qemu-bridge-helper with both the bridge and tap network interfaces. It > >> adds > >> the --ifname option to qemu-bridge-helper, removes the restriction > >> that ifname > >> cannot be specified with helper for the tap interface, and adds logic to > >> specify the --ifname option when exec'ing the helper. > > > > ping ... or syn. any other thoughts about this? > > I share Michael's perplexity. This feature could be exploitable. > > If we want to add this, the ifname should be subject to ACL rules just > like bridge names. For example you could have a special allow/deny > directive "allow foo@" which allows ifnames starting with "foo". This is a good idea. The default should be that you are not allowed to choose arbitrary interface names. Stefan
diff --git a/net/tap.c b/net/tap.c index a88ae8f..cfb5bff 100644 --- a/net/tap.c +++ b/net/tap.c @@ -417,11 +417,13 @@ static int recv_fd(int c) return len; } -static int net_bridge_run_helper(const char *helper, const char *bridge) +static int net_bridge_run_helper(const char *helper, + const char *bridge, + const char *ifname) { sigset_t oldmask, mask; int pid, status; - char *args[5]; + char *args[6]; char **parg; int sv[2]; @@ -439,7 +441,9 @@ static int net_bridge_run_helper(const char *helper, const char *bridge) int open_max = sysconf(_SC_OPEN_MAX), i; char fd_buf[6+10]; char br_buf[6+IFNAMSIZ] = {0}; - char helper_cmd[PATH_MAX + sizeof(fd_buf) + sizeof(br_buf) + 15]; + char ifname_buf[10+IFNAMSIZ] = {0}; + char helper_cmd[PATH_MAX + sizeof(fd_buf) + sizeof(br_buf) + + sizeof(ifname_buf) + 15]; for (i = 0; i < open_max; i++) { if (i != STDIN_FILENO && @@ -459,8 +463,13 @@ static int net_bridge_run_helper(const char *helper, const char *bridge) snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge); } - snprintf(helper_cmd, sizeof(helper_cmd), "%s %s %s %s", - helper, "--use-vnet", fd_buf, br_buf); + if ((strstr(helper, "--ifname=") == NULL) && (ifname != NULL)) { + snprintf(ifname_buf, sizeof(ifname_buf), "%s%s" , + "--ifname=", ifname); + } + + snprintf(helper_cmd, sizeof(helper_cmd), "%s %s %s %s %s", + helper, "--use-vnet", fd_buf, br_buf, ifname_buf); parg = args; *parg++ = (char *)"sh"; @@ -473,12 +482,17 @@ static int net_bridge_run_helper(const char *helper, const char *bridge) /* assume helper is just the executable path name */ snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge); + if (ifname != NULL) { + snprintf(ifname_buf, sizeof(ifname_buf), "%s%s" , + "--ifname=", ifname); + } parg = args; *parg++ = (char *)helper; *parg++ = (char *)"--use-vnet"; *parg++ = fd_buf; *parg++ = br_buf; + *parg++ = ifname_buf; *parg++ = NULL; execv(helper, args); @@ -517,7 +531,7 @@ int net_init_bridge(const NetClientOptions *opts, const char *name, NetClientState *peer) { const NetdevBridgeOptions *bridge; - const char *helper, *br; + const char *helper, *br, *ifname; TAPState *s; int fd, vnet_hdr; @@ -527,8 +541,9 @@ int net_init_bridge(const NetClientOptions *opts, const char *name, helper = bridge->has_helper ? bridge->helper : DEFAULT_BRIDGE_HELPER; br = bridge->has_br ? bridge->br : DEFAULT_BRIDGE_INTERFACE; + ifname = bridge->has_ifname ? bridge->ifname : NULL; - fd = net_bridge_run_helper(helper, br); + fd = net_bridge_run_helper(helper, br, ifname); if (fd == -1) { return -1; } @@ -622,14 +637,16 @@ int net_init_tap(const NetClientOptions *opts, const char *name, model = "tap"; } else if (tap->has_helper) { - if (tap->has_ifname || tap->has_script || tap->has_downscript || - tap->has_vnet_hdr) { - error_report("ifname=, script=, downscript=, and vnet_hdr= " + if (tap->has_script || tap->has_downscript || tap->has_vnet_hdr) { + error_report("script=, downscript=, and vnet_hdr= " "are invalid with helper="); return -1; } - fd = net_bridge_run_helper(tap->helper, DEFAULT_BRIDGE_INTERFACE); + const char *ifname; + ifname = tap->has_ifname ? tap->ifname : NULL; + fd = net_bridge_run_helper(tap->helper, DEFAULT_BRIDGE_INTERFACE, + ifname); if (fd == -1) { return -1; } diff --git a/qapi-schema.json b/qapi-schema.json index f9dbdae..feaac9e 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -2432,7 +2432,8 @@ { 'type': 'NetdevBridgeOptions', 'data': { '*br': 'str', - '*helper': 'str' } } + '*helper': 'str', + '*ifname': 'str' } } ## # @NetdevHubPortOptions diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c index 652eec9..c1d1519 100644 --- a/qemu-bridge-helper.c +++ b/qemu-bridge-helper.c @@ -67,7 +67,8 @@ typedef QSIMPLEQ_HEAD(ACLList, ACLRule) ACLList; static void usage(void) { fprintf(stderr, - "Usage: qemu-bridge-helper [--use-vnet] --br=bridge --fd=unixfd\n"); + "Usage: qemu-bridge-helper [--use-vnet] [--ifname=name] " + "--br=bridge --fd=unixfd\n"); } static int parse_acl_file(const char *filename, ACLList *acl_list) @@ -239,6 +240,7 @@ int main(int argc, char **argv) ACLList acl_list; int access_allowed, access_denied; int ret = EXIT_SUCCESS; + const char *ifname = NULL; #ifdef CONFIG_LIBCAP /* if we're run from an suid binary, immediately drop privileges preserving @@ -259,6 +261,8 @@ int main(int argc, char **argv) bridge = &argv[index][5]; } else if (strncmp(argv[index], "--fd=", 5) == 0) { unixfd = atoi(&argv[index][5]); + } else if (strncmp(argv[index], "--ifname=", 9) == 0) { + ifname = &argv[index][9]; } else { usage(); return EXIT_FAILURE; @@ -329,8 +333,8 @@ int main(int argc, char **argv) } /* request a tap device, disable PI, and add vnet header support if - * requested and it's available. */ - prep_ifreq(&ifr, "tap%d"); + * requested and it's available. use ifname if provided for tap name. */ + prep_ifreq(&ifr, ifname != NULL ? ifname : "tap%d"); ifr.ifr_flags = IFF_TAP|IFF_NO_PI; if (use_vnet && has_vnet_hdr(fd)) { ifr.ifr_flags |= IFF_VNET_HDR;
This makes a few changes to allow ifname to be specified when using qemu-bridge-helper with both the bridge and tap network interfaces. It adds the --ifname option to qemu-bridge-helper, removes the restriction that ifname cannot be specified with helper for the tap interface, and adds logic to specify the --ifname option when exec'ing the helper. Signed-off-by: Mike Lovell <mike@dev-zero.net> --- This feature was originally requested by Mario De Chenno on the qemu-devel mailing list. Seems pretty simple and figured it was something I could throw together pretty quickly. I have tested the following combinations of invoking qemu (where qbr is qemu-bridge-helper) qemu-system-x86_64 -net nic -net tap,helper="qbr --br=test1" qemu-system-x86_64 -net nic -net tap,helper="qbr --br=test1",ifname=vm1 qemu-system-x86_64 -net nic -net tap,helper=qbr qemu-system-x86_64 -net nic -net tap,helper=qbr,ifname=vm1 qemu-system-x86_64 -net nic -net bridge,helper=qbr qemu-system-x86_64 -net nic -net bridge,helper=qbr,ifname=vm1 qemu-system-x86_64 -net nic -net bridge,helper=qbr,ifname=vm1,br=test1 qemu-system-x86_64 -net nic -net bridge,helper=qbr,br=test1 net/tap.c | 39 ++++++++++++++++++++++++++++----------- qapi-schema.json | 3 ++- qemu-bridge-helper.c | 10 +++++++--- 3 files changed, 37 insertions(+), 15 deletions(-)