[PATCH] qemu_namespace: Deal with nested mounts when umount()-ing /dev

Michal Privoznik posted 1 patch 1 year, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/a5e07a37a4214ca5061e6c3fe86726a6d5a5182d.1675782135.git.mprivozn@redhat.com
src/qemu/qemu_namespace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] qemu_namespace: Deal with nested mounts when umount()-ing /dev
Posted by Michal Privoznik 1 year, 1 month ago
In one of recent commits (v9.0.0-rc1~106) I've made our QEMU
namespace code umount the original /dev. One of the reasons was
enhanced security, because previously we just mounted a tmpfs
over the original /dev. Thus a malicious QEMU could just
umount("/dev") and it would get to the original /dev with all
nodes.

Now, on some systems this introduced a regression:

   failed to umount devfs on /dev: Device or resource busy

But how this could be? We've moved all file systems mounted under
/dev to a temporary location. Or have we? As it turns out, not
quite. If there are two file systems mounted on the same target,
e.g. like this:

  mount -t tmpfs tmpfs /dev/shm/ && mount -t tmpfs tmpfs /dev/shm/

then only the top most (i.e. the last one) is moved. See
qemuDomainUnshareNamespace() for more info.

Now, we could enhance our code to deal with these "doubled" mount
points. Or, since it is the top most file system that is
accessible anyways (and this one is preserved), we can
umount("/dev") in a recursive fashion.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2167302
Fixes: 379c0ce4bfed8733dfbde557c359eecc5474ce38
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_namespace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
index 5769a4dfe0..5fc043bd62 100644
--- a/src/qemu/qemu_namespace.c
+++ b/src/qemu/qemu_namespace.c
@@ -777,7 +777,7 @@ qemuDomainUnshareNamespace(virQEMUDriverConfig *cfg,
     }
 
 #if defined(__linux__)
-    if (umount("/dev") < 0) {
+    if (umount2("/dev", MNT_DETACH) < 0) {
         virReportSystemError(errno, "%s", _("failed to umount devfs on /dev"));
         return -1;
     }
-- 
2.39.1
Re: [PATCH] qemu_namespace: Deal with nested mounts when umount()-ing /dev
Posted by Jim Fehlig 1 year, 1 month ago
On 2/7/23 08:02, Michal Privoznik wrote:
> In one of recent commits (v9.0.0-rc1~106) I've made our QEMU
> namespace code umount the original /dev. One of the reasons was
> enhanced security, because previously we just mounted a tmpfs
> over the original /dev. Thus a malicious QEMU could just
> umount("/dev") and it would get to the original /dev with all
> nodes.
> 
> Now, on some systems this introduced a regression:
> 
>     failed to umount devfs on /dev: Device or resource busy
> 
> But how this could be? We've moved all file systems mounted under
> /dev to a temporary location. Or have we? As it turns out, not
> quite. If there are two file systems mounted on the same target,
> e.g. like this:
> 
>    mount -t tmpfs tmpfs /dev/shm/ && mount -t tmpfs tmpfs /dev/shm/
> 
> then only the top most (i.e. the last one) is moved. See
> qemuDomainUnshareNamespace() for more info.
> 
> Now, we could enhance our code to deal with these "doubled" mount
> points. Or, since it is the top most file system that is
> accessible anyways (and this one is preserved), we can
> umount("/dev") in a recursive fashion.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2167302
> Fixes: 379c0ce4bfed8733dfbde557c359eecc5474ce38
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>   src/qemu/qemu_namespace.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
> index 5769a4dfe0..5fc043bd62 100644
> --- a/src/qemu/qemu_namespace.c
> +++ b/src/qemu/qemu_namespace.c
> @@ -777,7 +777,7 @@ qemuDomainUnshareNamespace(virQEMUDriverConfig *cfg,
>       }
>   
>   #if defined(__linux__)
> -    if (umount("/dev") < 0) {
> +    if (umount2("/dev", MNT_DETACH) < 0) {
>           virReportSystemError(errno, "%s", _("failed to umount devfs on /dev"));
>           return -1;
>       }

Reviewed-by: Jim Fehlig <jfehlig@suse.com>

Thanks for the investigating the problem and providing a reproducer and a fix!

Regards,
Jim