| Message ID | 20180116231824.27114-1-shaun.reitan@ndchost.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add ability to provide ifname when using netdev bridge or tap helper | expand |
On 2018年01月17日 07:18, Shaun Reitan wrote: > This patch replaces the patch I sent yesturday. This one fixes > a bug in my original code as well as corrects a few styling > issues. Hopfully this one comes out correct! Sorry for the > inconvienece. > > When currently using -netdev bridge or -netdev tap with a helper > you are unable to set an ifname. This patch adds that ability so > that you can now specify an ifname. > > Signed-off-by: Shaun Reitan <shaun.reitan@ndchost.com> > --- > net/tap.c | 33 ++++++++++++++++++++++++--------- > qapi/net.json | 1 + > qemu-bridge-helper.c | 11 +++++++++-- > qemu-options.hx | 2 +- > 4 files changed, 35 insertions(+), 12 deletions(-) > Applied. Thanks
On Tue, Jan 16, 2018 at 03:18:24PM -0800, Shaun Reitan wrote: > This patch replaces the patch I sent yesturday. This one fixes > a bug in my original code as well as corrects a few styling > issues. Hopfully this one comes out correct! Sorry for the > inconvienece. > > When currently using -netdev bridge or -netdev tap with a helper > you are unable to set an ifname. This patch adds that ability so > that you can now specify an ifname. I really don't think users should be allowed to override the ifname when using the setuid bridge helper. This allows an unprivileged users to create tap devices that may confuse the sysadmin, and/or network mgmt scripts. eg consider the user asks for a tap device called eth1. To the sysadmin the user's tap device now looks like a physical NIC. This can be even worse if the host does physical NIC hotplug, or uses SRIOV. eg consider the host as eth0 -> eth7 for SRIOV NICs, and eth3 is given to a guest. Now a user uses the setuid helper to ask for a TAP called eth3. When the SRIOV device is later released by the guest it will end up called eth8, as the TAP device occupies eth3. In bad cases this could even cause the host mgmt layer to configure bogus addresses on the eth3 TAP device instead of the SRIOV device. If we want to allow ifname to be set via the setuid helper, then IMHO, the config file for the helper *must* whitelist the various permitted naming patterns. Regards, Daniel
On 17/01/2018 11:31, Daniel P. Berrange wrote: > > eg consider the user asks for a tap device called eth1. To the > sysadmin the user's tap device now looks like a physical NIC. > This can be even worse if the host does physical NIC hotplug, > or uses SRIOV. eg consider the host as eth0 -> eth7 for SRIOV > NICs, and eth3 is given to a guest. Now a user uses the setuid > helper to ask for a TAP called eth3. When the SRIOV device is > later released by the guest it will end up called eth8, as the > TAP device occupies eth3. In bad cases this could even cause > the host mgmt layer to configure bogus addresses on the eth3 > TAP device instead of the SRIOV device. > > If we want to allow ifname to be set via the setuid helper, then IMHO, > the config file for the helper *must* whitelist the various permitted > naming patterns. Indeed, a similar patch has been proposed several times, and always the response was the same as Daniel's. :) Paolo
On 2018年01月17日 18:31, Daniel P. Berrange wrote: > On Tue, Jan 16, 2018 at 03:18:24PM -0800, Shaun Reitan wrote: >> This patch replaces the patch I sent yesturday. This one fixes >> a bug in my original code as well as corrects a few styling >> issues. Hopfully this one comes out correct! Sorry for the >> inconvienece. >> >> When currently using -netdev bridge or -netdev tap with a helper >> you are unable to set an ifname. This patch adds that ability so >> that you can now specify an ifname. > I really don't think users should be allowed to override the > ifname when using the setuid bridge helper. This allows an > unprivileged users to create tap devices that may confuse the > sysadmin, and/or network mgmt scripts. Ok, I drop it from my queue. > > eg consider the user asks for a tap device called eth1. To the > sysadmin the user's tap device now looks like a physical NIC. > This can be even worse if the host does physical NIC hotplug, > or uses SRIOV. eg consider the host as eth0 -> eth7 for SRIOV > NICs, and eth3 is given to a guest. Now a user uses the setuid > helper to ask for a TAP called eth3. When the SRIOV device is > later released by the guest it will end up called eth8, as the > TAP device occupies eth3. In bad cases this could even cause > the host mgmt layer to configure bogus addresses on the eth3 > TAP device instead of the SRIOV device. It looks to me that mgmt should not assume the type or location of device just from its name. Ethtool should be used to do this. > > If we want to allow ifname to be set via the setuid helper, then IMHO, > the config file for the helper *must* whitelist the various permitted > naming patterns. Good point but this only work for e.g default helper. Qemu allows 3rd helper which can do anything they want. Thanks > > > Regards, > Daniel
On Wed, Jan 17, 2018 at 06:53:30PM +0800, Jason Wang wrote: > > > On 2018年01月17日 18:31, Daniel P. Berrange wrote: > > On Tue, Jan 16, 2018 at 03:18:24PM -0800, Shaun Reitan wrote: > > > This patch replaces the patch I sent yesturday. This one fixes > > > a bug in my original code as well as corrects a few styling > > > issues. Hopfully this one comes out correct! Sorry for the > > > inconvienece. > > > When currently using -netdev bridge or -netdev tap with a helper > > > you are unable to set an ifname. This patch adds that ability so > > > that you can now specify an ifname. > > I really don't think users should be allowed to override the > > ifname when using the setuid bridge helper. This allows an > > unprivileged users to create tap devices that may confuse the > > sysadmin, and/or network mgmt scripts. > > Ok, I drop it from my queue. > > > > > eg consider the user asks for a tap device called eth1. To the > > sysadmin the user's tap device now looks like a physical NIC. > > This can be even worse if the host does physical NIC hotplug, > > or uses SRIOV. eg consider the host as eth0 -> eth7 for SRIOV > > NICs, and eth3 is given to a guest. Now a user uses the setuid > > helper to ask for a TAP called eth3. When the SRIOV device is > > later released by the guest it will end up called eth8, as the > > TAP device occupies eth3. In bad cases this could even cause > > the host mgmt layer to configure bogus addresses on the eth3 > > TAP device instead of the SRIOV device. > > It looks to me that mgmt should not assume the type or location of device > just from its name. Ethtool should be used to do this. Yes, a well written tool, or a prudent administrator should be careful and validate the devices they use, but most have assumptions they make which could easily be violated here. > > If we want to allow ifname to be set via the setuid helper, then IMHO, > > the config file for the helper *must* whitelist the various permitted > > naming patterns. > > Good point but this only work for e.g default helper. Qemu allows 3rd helper > which can do anything they want. If anyone writes a custom setuid helper, they are responsible for writing to in a way that doesn't open security holes, or expose the admin to intentionally misleading setup. IOW, that is somebody else's problem. Regards, Daniel
On 01/17/2018 07:39 AM, Paolo Bonzini wrote: > On 17/01/2018 11:31, Daniel P. Berrange wrote: >> >> eg consider the user asks for a tap device called eth1. To the >> sysadmin the user's tap device now looks like a physical NIC. >> This can be even worse if the host does physical NIC hotplug, >> or uses SRIOV. eg consider the host as eth0 -> eth7 for SRIOV >> NICs, and eth3 is given to a guest. Now a user uses the setuid >> helper to ask for a TAP called eth3. When the SRIOV device is >> later released by the guest it will end up called eth8, as the >> TAP device occupies eth3. In bad cases this could even cause >> the host mgmt layer to configure bogus addresses on the eth3 >> TAP device instead of the SRIOV device. >> >> If we want to allow ifname to be set via the setuid helper, then IMHO, >> the config file for the helper *must* whitelist the various permitted >> naming patterns. > > Indeed, a similar patch has been proposed several times, and always the > response was the same as Daniel's. :) Can this be added to docs/netdev-helpers.rst and with a comment referencing this file in qemu-bridge-helper.c ?
On 01/16/2018 05:18 PM, Shaun Reitan wrote: > This patch replaces the patch I sent yesturday. This one fixes > a bug in my original code as well as corrects a few styling > issues. Hopfully this one comes out correct! Sorry for the > inconvienece. This paragraph belongs... > > When currently using -netdev bridge or -netdev tap with a helper > you are unable to set an ifname. This patch adds that ability so > that you can now specify an ifname. > > Signed-off-by: Shaun Reitan <shaun.reitan@ndchost.com> > --- ...here, after the --- separator. It is useful for reviewers reading the list, but pointless in the git log a year from now. > net/tap.c | 33 ++++++++++++++++++++++++--------- > qapi/net.json | 1 + > qemu-bridge-helper.c | 11 +++++++++-- > qemu-options.hx | 2 +- > 4 files changed, 35 insertions(+), 12 deletions(-) > +++ b/qapi/net.json > @@ -402,6 +402,7 @@ > { 'struct': 'NetdevBridgeOptions', > 'data': { > '*br': 'str', > + '*ifname': 'str', Missing documentation (including a '(since 2.12)' tag) of the new field.
Ok, how should i proceed with this? I can attempt to write a whitelist/blacklist check if that is what's needed. -- Shaun Reitan NDCHost.com ------ Original Message ------ From: "Eric Blake" <eblake@redhat.com> To: "Shaun Reitan" <shaun.reitan@ndchost.com>; qemu-devel@nongnu.org Cc: "Jason Wang" <jasowang@redhat.com>; "Markus Armbruster" <armbru@redhat.com> Sent: 1/17/2018 7:26:55 AM Subject: Re: [PATCH] Add ability to provide ifname when using netdev bridge or tap helper >On 01/16/2018 05:18 PM, Shaun Reitan wrote: >>This patch replaces the patch I sent yesturday. This one fixes >>a bug in my original code as well as corrects a few styling >>issues. Hopfully this one comes out correct! Sorry for the >>inconvienece. > >This paragraph belongs... > >> >>When currently using -netdev bridge or -netdev tap with a helper >>you are unable to set an ifname. This patch adds that ability so >>that you can now specify an ifname. >> >>Signed-off-by: Shaun Reitan <shaun.reitan@ndchost.com> >>--- > >...here, after the --- separator. It is useful for reviewers reading >the list, but pointless in the git log a year from now. > >> net/tap.c | 33 ++++++++++++++++++++++++--------- >> qapi/net.json | 1 + >> qemu-bridge-helper.c | 11 +++++++++-- >> qemu-options.hx | 2 +- >> 4 files changed, 35 insertions(+), 12 deletions(-) > >>+++ b/qapi/net.json >>@@ -402,6 +402,7 @@ >> { 'struct': 'NetdevBridgeOptions', >> 'data': { >> '*br': 'str', >>+ '*ifname': 'str', > >Missing documentation (including a '(since 2.12)' tag) of the new >field. > >-- >Eric Blake, Principal Software Engineer >Red Hat, Inc. +1-919-301-3266 >Virtualization: qemu.org | libvirt.org >
On 2018年01月18日 02:11, Shaun Reitan wrote: > Ok, how should i proceed with this? I can attempt to write a > whitelist/blacklist check if that is what's needed. > > -- > Shaun Reitan > NDCHost.com I think you can try. Thanks
Would that make everyone happy? I dont want to work on this feature if it's going to be denied still :) -- Shaun Reitan NDCHost.com ------ Original Message ------ From: "Jason Wang" <jasowang@redhat.com> To: "Shaun Reitan" <shaun.reitan@ndchost.com>; "Eric Blake" <eblake@redhat.com>; qemu-devel@nongnu.org Cc: "Markus Armbruster" <armbru@redhat.com> Sent: 2018-01-18 03:53:29 AM Subject: Re: [Qemu-devel] [PATCH] Add ability to provide ifname when using netdev bridge or tap helper > > >On 2018年01月18日 02:11, Shaun Reitan wrote: >>Ok, how should i proceed with this? I can attempt to write a >>whitelist/blacklist check if that is what's needed. >> >>-- Shaun Reitan >>NDCHost.com > >I think you can try. > >Thanks
diff --git a/net/tap.c b/net/tap.c index 979e622..c4f3bb0 100644 --- a/net/tap.c +++ b/net/tap.c @@ -472,7 +472,7 @@ static int recv_fd(int c) } static int net_bridge_run_helper(const char *helper, const char *bridge, - Error **errp) + const char *ifname, Error **errp) { sigset_t oldmask, mask; int pid, status; @@ -499,7 +499,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]; + char helper_cmd[PATH_MAX + sizeof(fd_buf) + sizeof(br_buf) + + sizeof(ifname_buf) + 16]; for (i = 3; i < open_max; i++) { if (i != sv[1]) { @@ -516,8 +518,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"; @@ -536,6 +543,13 @@ static int net_bridge_run_helper(const char *helper, const char *bridge, *parg++ = (char *)"--use-vnet"; *parg++ = fd_buf; *parg++ = br_buf; + + if (ifname != NULL) { + snprintf(ifname_buf, sizeof(ifname_buf), "%s%s", "--ifname=", + ifname); + *parg++ = ifname_buf; + } + *parg++ = NULL; execv(helper, args); @@ -586,7 +600,7 @@ int net_init_bridge(const Netdev *netdev, const char *name, helper = bridge->has_helper ? bridge->helper : DEFAULT_BRIDGE_HELPER; br = bridge->has_br ? bridge->br : DEFAULT_BRIDGE_INTERFACE; - fd = net_bridge_run_helper(helper, br, errp); + fd = net_bridge_run_helper(helper, br, bridge->ifname, errp); if (fd == -1) { return -1; } @@ -854,16 +868,17 @@ free_fail: g_free(vhost_fds); return -1; } else if (tap->has_helper) { - if (tap->has_ifname || tap->has_script || tap->has_downscript || - tap->has_vnet_hdr || tap->has_queues || tap->has_vhostfds) { - error_setg(errp, "ifname=, script=, downscript=, vnet_hdr=, " - "queues=, and vhostfds= are invalid with helper="); + if (tap->has_script || tap->has_downscript || tap->has_vnet_hdr || + tap->has_queues || tap->has_vhostfds) { + error_setg(errp, "script=, downscript=, vnet_hdr=, queues=, and " + "vhostfds= are invalid with helper="); return -1; } fd = net_bridge_run_helper(tap->helper, tap->has_br ? tap->br : DEFAULT_BRIDGE_INTERFACE, + tap->ifname, errp); if (fd == -1) { return -1; diff --git a/qapi/net.json b/qapi/net.json index 4beff5d..5b5b281 100644 --- a/qapi/net.json +++ b/qapi/net.json @@ -402,6 +402,7 @@ { 'struct': 'NetdevBridgeOptions', 'data': { '*br': 'str', + '*ifname': 'str', '*helper': 'str' } } ## diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c index 5396fbf..7b01cd5 100644 --- a/qemu-bridge-helper.c +++ b/qemu-bridge-helper.c @@ -57,7 +57,7 @@ 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] --br=bridge --fd=unixfd [--ifname=name]\n"); } static int parse_acl_file(const char *filename, ACLList *acl_list) @@ -223,6 +223,7 @@ int main(int argc, char **argv) int use_vnet = 0; int mtu; const char *bridge = NULL; + const char *ifname = NULL; char iface[IFNAMSIZ]; int index; ACLRule *acl_rule; @@ -249,6 +250,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; @@ -320,7 +323,11 @@ 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"); + if (ifname == NULL) { + prep_ifreq(&ifr, "tap%d"); + } else { + prep_ifreq(&ifr, ifname); + } ifr.ifr_flags = IFF_TAP|IFF_NO_PI; if (use_vnet && has_vnet_hdr(fd)) { ifr.ifr_flags |= IFF_VNET_HDR; diff --git a/qemu-options.hx b/qemu-options.hx index 678181c..81cbc3c 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -1961,7 +1961,7 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev, " use 'queues=n' to specify the number of queues to be created for multiqueue TAP\n" " use 'poll-us=n' to speciy the maximum number of microseconds that could be\n" " spent on busy polling for vhost net\n" - "-netdev bridge,id=str[,br=bridge][,helper=helper]\n" + "-netdev bridge,id=str[,br=bridge][,helper=helper][,ifname=name]\n" " configure a host TAP network backend with ID 'str' that is\n" " connected to a bridge (default=" DEFAULT_BRIDGE_INTERFACE ")\n" " using the program 'helper (default=" DEFAULT_BRIDGE_HELPER ")\n"
This patch replaces the patch I sent yesturday. This one fixes a bug in my original code as well as corrects a few styling issues. Hopfully this one comes out correct! Sorry for the inconvienece. When currently using -netdev bridge or -netdev tap with a helper you are unable to set an ifname. This patch adds that ability so that you can now specify an ifname. Signed-off-by: Shaun Reitan <shaun.reitan@ndchost.com> --- net/tap.c | 33 ++++++++++++++++++++++++--------- qapi/net.json | 1 + qemu-bridge-helper.c | 11 +++++++++-- qemu-options.hx | 2 +- 4 files changed, 35 insertions(+), 12 deletions(-)