On Fri, Sep 06, 2019 at 11:37:12AM -0400, Laine Stump wrote:
On 9/6/19 5:16 AM, Daniel P. Berrangé wrote:
On Tue, Aug 27, 2019 at 09:46:38PM -0400, Laine Stump wrote:
libvirt creates its tap devices without the IFF_PERSIST flag, so they will be automatically deleted when qemu is finished with them. In the case of tap devices created outside of libvirt, if the creating entity wants the devices to be deleted, it will also omit IFF_PERSIST, but if it wants them to remain (e.g. for re-use), then it will use IFF_PERSIST when creating the device.
Back when support was added for autocreation by libvirt of tap devices for <interface type='ethernet'> (commit 9c17d665), code was mistakenly put in qemuProcessStop to always delete tap devices for type='ethernet'. This should only be done on platforms that have VIR_NETDEV_TAP_REQUIRE_MANUAL_CLEANUP #defined (which is only FreeBSD). This isn't right. The tap devices should *always* be deleted as we don't trust that QEMU hasn't (possibly maliciously) set IFF_PERSIST itself.
(So you're saying this is a security issue that was coincidentally fixed by commit 9c17d665?)
I'm not sure I'd call it a security issue - more of a reliability issue - since its really just a denial of service at most. We just want to be sure we're not leaving anything behind when QEMU quits.
Interesting point. But I wonder if it's also problematic that (in the case of a tap device created by someone else (not libvirt) who purposefully set IFF_PERSIST) QEMU could mistakenly/maliciously *clear* IFF_PERSIST. I guess there's really nothing we could do about that though, since the device would already be deleted by the time we found out about it...
I found this bit of code specifically because I was creating tap devices with IFF_PERSIST set (that's just what "ip tuntap add" does), and they were disappearing after each use, which may or may not be what the user wants - another case of "someone" clearing IFF_PERSIST, but in this case it is *us*.
If they don't want the device deleted, then they can set managed=no now with your patch series.
And as a matter of fact I can't see a way to even force macvtap devices to be deleted by an unprivileged process - when I had libvirt try to do the standard delete, it would fail. So having this unconditional forced delete of all standard tap devices both causes an unexpected behavior for some users, as well as creating an inconsistency between tap and macvtap behavior (standard taps are always deleted, macvtaps are never deleted).
We don't currently support pre-createds macvtap, so we can fix this inconsistency so that it works the way tap devs do.
(This reminds me of another inconsistency I saw while looking at this, but then later forgot - virNetDevTapDelete() is *never * called in the case of hot-unplug. So if you think that we should be unconditionally deleting all taps after use regardless of the previous state of IFF_PERSIST of pre-created taps, then we should also be doing it for hot-unplug.)
So how about if we remember the setting of IFF_PERSIST prior to starting QEMU, and restore it to its previous state afterwards? That would make behavior more what was expected / consistent with macvtap.
I don't think we need rmemeber IFF_PERSIST when we have the "managed" flag now. 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 :|