Skip to content

Disk: Allow volume security.shifted and directory shift to be enabled when raw.idmap is set on a VM - #18918

Merged
tomponline merged 11 commits into
canonical:mainfrom
tomponline:tp-virtiofsd-userns
Sep 8, 2026
Merged

Disk: Allow volume security.shifted and directory shift to be enabled when raw.idmap is set on a VM#18918
tomponline merged 11 commits into
canonical:mainfrom
tomponline:tp-virtiofsd-userns

Conversation

@tomponline

@tomponline tomponline commented Aug 25, 2026

Copy link
Copy Markdown
Member

This is achieved by always running virtiofsd in a userns, and just adjusting the uid/gid mappings based on the settings:

If security.shifted or shift are enabled, then raw.idmap is ignored (for that disk device) and the host's UID/GID userns range is passed through.

Previously, when raw.idmap was empty and no restricted path was set, virtiofsd ran without a user namespace - effectively as real root in the host namespace.

Now it always runs inside a user namespace. This means the process handling untrusted guest-driven filesystem operations no longer holds genuine host root.

If virtiofsd is compromised (e.g. via a parsing or protocol bug driven by the guest), the blast radius is reduced to the mapped, unprivileged ID range instead of host UID 0.

Partially fixes #18686

https://warthogs.atlassian.net/browse/LXD-4937

Comment thread lxd/subprocess/proc_linux.go Outdated
Comment thread lxd/device/disk.go

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR changes VM disk sharing so virtiofsd always runs in a user namespace while supporting shifted volumes and directories alongside raw.idmap.

Changes:

  • Selects virtiofsd ID mappings based on disk-shifting settings.
  • Enables supplementary groups in subprocess user namespaces.
  • Adds unit and VM integration coverage.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
lxd/device/disk.go Selects mappings for shifted VM shares.
lxd/device/device_utils_disk.go Resolves mappings and always enables a user namespace.
lxd/device/device_utils_disk_test.go Tests mapping resolution.
lxd/subprocess/proc_linux.go Enables setgroups in user namespaces.
test/suites/vm.sh Tests shifted-volume ownership with raw.idmap.
Suppressed comments (1)

lxd/device/device_utils_disk_test.go:47

  • The GID expectation has the same nesting error as the UID expectation above: this child map must target current-namespace GID 0, not the current namespace's parent-side GID 100000. Expect Hostid: 0 after fixing the helper.
				Hostid:   100000,

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread lxd/device/device_utils_disk.go Outdated
Comment thread lxd/device/device_utils_disk.go
Comment thread lxd/device/disk.go
Comment thread lxd/device/disk.go Outdated
Comment thread lxd/device/device_utils_disk_test.go
@tomponline
tomponline force-pushed the tp-virtiofsd-userns branch 2 times, most recently from 4af9cc8 to 25b4b1e Compare August 25, 2026 11:43
@tomponline
tomponline requested a balanced review from Copilot August 25, 2026 11:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

lxd/device/device_utils_disk.go:275

  • On a non-nested host, /proc/self/uid_map and gid_map normally contain an identity mapping starting at 0. Converting those entries to Hostid = Nsid therefore maps namespace UID/GID 0 to host UID/GID 0 (and, with setgroups enabled, permits assuming any mapped host group). Since SetUserns starts virtiofsd as namespace UID 0, this does not put it in the unprivileged host ID range claimed by the PR and still grants owner access to host-root-owned files visible to the process. The fallback mapping needs to exclude privileged host identities or the isolation design must otherwise ensure virtiofsd's credentials cannot resolve to them.
		effectiveIDMaps = append(effectiveIDMaps, idmap.IdmapEntry{
			Hostid:   idmapEntry.Nsid,
			Isuid:    idmapEntry.Isuid,
			Isgid:    idmapEntry.Isgid,
			Nsid:     idmapEntry.Nsid,

Comment thread lxd/device/disk.go
Comment thread lxd/device/disk.go
@tomponline
tomponline force-pushed the tp-virtiofsd-userns branch 5 times, most recently from d95cf4d to dd4dc55 Compare August 27, 2026 10:59
Comment thread lxd/device/disk.go
@tomponline
tomponline force-pushed the tp-virtiofsd-userns branch 4 times, most recently from d3d6217 to 80e39fd Compare August 28, 2026 14:58
@jonathan-conder

jonathan-conder commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

@jonathan-conder that worked nicely thanks! Although I'm still a bit concerned now we know about the leaking MNT_LOCKED problem that this might bite us elsewhere. I'm am still exploring 2 other avenues: using idmapped mounts as you suggested previously, or using virtiofsd own userns idmapping functionality.

Yeah there seem to be at least 2 limitations, even with working directory /:

  • there's still a small window where container mounts become busy (from unshare(CLONE_NEWNS) until close(oldroot_fd) in virtiofsd)
  • anything mounted inside the --shared-dir remains locked

I think the builtin idmapping is gated behind uid!=0, maybe it's usable if combined with ID-mapped mounts though.

Another option is for LXD to set up the pivot_root sandbox itself and run virtiofsd with --sandbox=none

@tomponline

Copy link
Copy Markdown
Member Author

@jonathan-conder indeed, this is exactly my concern, that your current working dir fix solves the problem for the other instance config drive mounts, but the same issue might occur elsewhere.

anything mounted inside the --shared-dir remains locked

I think we can expect that in any case, because if we use a idmapped mount it will similarly hold the shared dir open.

I'm tempted to get this merged now so you are unblocked, and we can then address your other issue #18875

And then once we have a thorough test suite for this behaviour, we can then pivot to idmapped mounts as an (hopefully) no-op change?

What do you think?

@jonathan-conder

Copy link
Copy Markdown
Contributor

I think we can expect that in any case, because if we use a idmapped mount it will similarly hold the shared dir open.

yes this is reasonable, I think. the unreasonable part is you can mount something inside the shared dir, before attaching the shared dir to a VM, and then the kernel prevents you from unmounting the nested dir as well!

I'm tempted to get this merged now

Let me build and test this with Workshop tomorrow, just to check. I think it's meaningful progress, at least. Maybe we can just retry a few times if we see the EBUSY error, for now.

@tomponline

Copy link
Copy Markdown
Member Author

I have #18965 as another WIP that I think we can try once we've got a working solution with tests.

@jonathan-conder jonathan-conder left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it today and was able to mount a volume in an idmapped VM. Was also above to remove a VM after mounting a directory in a second VM. Thanks!

@jonathan-conder

jonathan-conder commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

couple of other things: I think the proc.Dir = "/" could use a comment (my fault). e.g.:

// This is required because virtiofsd is split into two long-running processes.
// The child calls `pivot_root(2)`, which sandboxes both processes inside `sharePath`.
// However, it only pivots the working directory of the parent process when it starts as `/`.
// Normally this would only prevent unmounting LXD's working directory, which is OK.
// But when we run virtiofsd from a non-initial user namespace, all existing mounts are
// brought into the sandbox as a single unit (see `mount_namespaces(7)`). These remain
// alive even after unmounting them on the host, which can prevent LXD from deleting
// container rootfs volumes.
proc.Dir = "/"

or maybe it's temporary enough not to worry.

I was also wondering if you saw my comment over here: canonical/workshop#911 (comment)

I assume this won't be backported to 6.9, but it would be nice to have a snap branch, e.g. 6.9/edge/workshop-vms with this PR cherry-picked

@tomponline

Copy link
Copy Markdown
Member Author

couple of other things: I think the proc.Dir = "/" could use a comment (my fault). e.g.:

// This is required because virtiofsd is split into two long-running processes.
// The child calls `pivot_root(2)`, which sandboxes both processes inside `sharePath`.
// However, it only pivots the working directory of the parent process when it starts as `/`.
// Normally this would only prevent unmounting LXD's working directory, which is OK.
// But when we run virtiofsd from a non-initial user namespace, all existing mounts are
// brought into the sandbox as a single unit (see `mount_namespaces(7)`). These remain
// alive even after unmounting them on the host, which can prevent LXD from deleting
// container rootfs volumes.
proc.Dir = "/"

or maybe it's temporary enough not to worry.

I was also wondering if you saw my comment over here: canonical/workshop#911 (comment)

I assume this won't be backported to 6.9, but it would be nice to have a snap branch, e.g. 6.9/edge/workshop-vms with this PR cherry-picked

Done!

@tomponline
tomponline requested review from jonathan-conder and roosterfish and removed request for jonathan-conder September 4, 2026 08:07
tomponline and others added 11 commits September 7, 2026 12:12
Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
…ion and tests

Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
But use diskVMVirtiofsdResolveIDMaps to decide whether its a broad userns (allowing allow host IDs)
or whether it is a restricted range based on `raw.idmap`.

Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
…r of /

Avoids keeping a reference to other instance's mounted volumes which can prevent deactivation of the volume later.

Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
…roups when run inside a userns from DiskVMVirtiofsdStart

Related to https://gitlab.com/qemu-project/qemu/-/commit/449e8171f96a6a944d1f3b7d3627ae059eae21ca and CVE-2022-0358

Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
Without this, file ownership in VMs doesn't match containers. This is
technically a breaking change, but I suspect not many people are using
raw.idmap together with shifted volumes, otherwise canonical#18561 probably would
have been discovered sooner. If it's an issue I'm happy to gate this
behind a new config option though.

Signed-off-by: Jonathan Conder <jonathan.conder@canonical.com>
…deviceConfig.MountOwnerShiftDynamic mode

This ensures that the host's ID ranges are used, even if `raw.idmap` is set on the instance.

Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>

@tugbataluy tugbataluy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @tomponline ❇️ !

@tomponline
tomponline merged commit 6e5b734 into canonical:main Sep 8, 2026
72 checks passed
@tomponline
tomponline deleted the tp-virtiofsd-userns branch September 8, 2026 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

File ownership in storage volumes differs between containers and VMs

6 participants