Re: [Qemu-devel] [PATCH] Add ability to provide ifname when using netdev bridge or tap helper

[Daniel P. Berrange]

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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|