Hide Forgot
Description of problem: On a system where I use libvirt's LXC and autofs, lxcContainerUnmountOldFS fails, possibly because it tries unmounting filesystems that were already unmounted by autofs while it was working. I believe that it is possible for a malevolent user to trigger this problem with careful accesses to /net or to removable devices, making this a local DoS attack. Factual information : - version number of the libvirt build: 0.8.8 - hardware architecture being used : amd64 - stracing the process gives the following output : [pid 9763] write(2, "16:08:45.287: 1: debug : lxcContainerUnmountOldFS:596 : Umount /.oldroot/mnt/gasiho/10.5.5.54/VFS/00/home-0\n", 108) = 108 [pid 9763] umount("/.oldroot/mnt/gasiho/10.5.5.54/VFS/00/home-0", 0) = 0 [pid 9763] write(2, "16:08:45.287: 1: debug : lxcContainerUnmountOldFS:596 : Umount /.oldroot/mnt/gasiho/10.5.5.54/VFS/00\n", 101) = 101 [pid 9763] umount("/.oldroot/mnt/gasiho/10.5.5.54/VFS/00", 0) = 0 [pid 9763] write(2, "16:08:45.287: 1: debug : lxcContainerUnmountOldFS:596 : Umount /.oldroot/mnt/gasiho/10.5.5.54/VFS/00\n", 101) = 101 [pid 9763] umount("/.oldroot/mnt/gasiho/10.5.5.54/VFS/00", 0) = -1 ENOENT (No such file or directory) [pid 9763] write(2, "16:08:45.287: 1: error : lxcContainerUnmountOldFS:600 : Failed to unmount '/.oldroot/mnt/gasiho/10.5.5.54/VFS/00': No such file or directory\n", 141) = 141 (that last error cancels the creation process) Suggested fix: lxcContainerUnmountOldFS should only fail if unmounting .oldroot fails, not if a single submount of .oldroot fails to unmount. The attached (untested) patch tries to implement that.
Created attachment 487385 [details] Suggested fix
I don't think ignoring all sub-mount umount failures is safe from a security POV, because we really need to guarantee that the container OS can never access any filesystem mount from the host OS. Thus we need to fail container startup if umount fails in a way that leaves the mount visible to the guest. I think we should thus just ignore any failure with errno == ENOENT.
If unmounting .oldroot succeeds, it means that the submounts are unmounted, and even if they weren't they would be inaccessible. Testing errno would also work, it's just slightly less nice IMHO : - we would need to think of all possible errors (eg. EINVAL should be accepted for the case where autofs has removed the mount but has not yet removed the directory) and maintain the list of those errors. - we would have problems if a signal handler were called, potentially changing errno - errno might not be threadsafe on non-Posix platforms Admittedly those potential issues are *very* unlikely, but the change I suggest (only testing the unmount of ".oldroot") avoids them while still allowing to be sure that the sub-mounts are unmounted. Therefore it is slightly better in my opinion.
Created attachment 488156 [details] Suggested fix v2 "Slightly better", except it shows the wrong error message xD. Sorry for that, please find a new version attached.
Sorry that second patch never received a response. I can't tell by inspecting the current code whether this was fixed or not, but things have changed quite a bit in the lxc driver in the past 5 years. Closing as DEFERRED, but if anyone can still reproduce, please reopen