On Wed, Jan 15, 2025 at 11:49:43AM -0300, Georgia Garcia wrote:
On Tue, 2025-01-14 at 12:13 -0600, Andrea Bolognani wrote:
Going by the example presented in [1], IIUC your change would make it so the lines needed for macvtap use, specifically
"/dev/net/tun" rwk, "/dev/tap82" rwk,
would be written to .runtime_files instead of .files. That's good enough to safeguard them from disappearing when disks are unplugged, but what if the macvtap interface itself is? Wouldn't those lines linger around despite being no longer needed?
Yes, they would, and that is the current behavior - if you remove only the macvtap, it will not be removed from .files
That's the current limitation because there are no security hooks called when macvtap devices are unplugged. I thought it would be better to be over-permissive (fd permissions linger throughout the runtime of the vm) than over-restrictive to fix the issue given what's available in the security side of libvirt.
From a functional standpoint sure, leaving rules behind is not a problem. But considering that the whole point of a security driver is to prevent the QEMU process from accessing resources that it shouldn't have access to, I would consider it a pretty serious flaw.
So I think we really need a --remove-file option that can be used to carefully undo the changes applied by an earlier use of --add-file.
Unfortunately this will likely involve a far more significant rework of the AppArmor driver, and we will certainly have to be careful about not introducing regressions in the process, but I'm really not a fan of half measures unless the trade-off is overwhelmingly stacked in their favor...
As I said earlier, it would also involve the addition of at least one security hook, impacting all security drivers. But yes, this change would basically involve rewriting the entire AppArmor driver and a part of virt-aa-helper. While I'm not against it, unfortunately I will not be able to dedicate the amount of time needed for such a significant change.
I haven't looked in detail at how much work adding the ability to remove rules on device unplug would require, but surely "basically rewrite the entire driver" is an overexaggeration? Look, I understand that you probably just want to fix the issue that's affecting your customers then move on with your life, and generally speaking I don't really have a problem with partial fixes that merely get us closer to the solution instead of all the way there. However, the changes you're proposing here alter how the driver operates in a pretty fundamental and, critically, user-visible way. I'm not keen on switching to a new approach while already being aware of the fact that a full fix with require yet another pivot... -- Andrea Bolognani / Red Hat / Virtualization