Re: [PATCH net-next 6/6] selftests/vsock: test the assign ioctl privilege checks

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 2 · Low: 1

- [Medium] test_ns_guest_assign_g2h_netns_unpriv_user_ns_fails()
  (tools/testing/selftests/vsock/vmtest.sh:1835-1863) runs the helper as…
- [Medium] Both new tests equate a remote command exit status of 1 with
  'the kernel returned -EPERM from IOCTL_VM_SOCKETS_ASSIGN_G2H_NETNS'.
- [Low] Both new shared-VM tests (vmtest.sh:1820-1833 and 1835-1863) are
  added to USE_SHARED_VM but omit the `vm_reset_g2h` teardown that every…

--

selftests/vsock: test the assign ioctl privilege checks

This adds two vmtest.sh cases around IOCTL_VM_SOCKETS_ASSIGN_G2H_NETNS on
/dev/vsock: one drops CAP_NET_ADMIN via setpriv, the other runs the helper
as an unprivileged uid inside a new user plus net namespace.  Both expect
the helper to exit with the ioctl's errno, and CONFIG_USER_NS is added to
the config.

No Link: tags are present in the commit header.

> diff --git a/tools/testing/selftests/vsock/vmtest.sh b/tools/testing/selftests/vsock/vmtest.sh
> index 53591fa07f1a..efb94d17d997 100755
> --- a/tools/testing/selftests/vsock/vmtest.sh
> +++ b/tools/testing/selftests/vsock/vmtest.sh
> @@ -28,6 +28,7 @@ readonly WAIT_PERIOD=3
>  readonly WAIT_PERIOD_MAX=60
>  readonly WAIT_QEMU=5
>  readonly PIDFILE_TEMPLATE=/tmp/vsock_vmtest_XXXX.pid
> +readonly EPERM=1
>  declare -A PIDFILES

[ ... ]

> @@ -333,7 +344,8 @@ check_args() {
>  }
>  
>  check_deps() {
> -	for dep in vng ${QEMU} busybox pkill ssh ss socat nsenter unshare; do
> +	for dep in vng ${QEMU} busybox pkill ssh ss socat nsenter unshare \
> +		setpriv; do
>  		if [[ ! -x $(command -v "${dep}") ]]; then
>  			echo -e "skip:    dependency ${dep} not found!\n"
>  			exit "${KSFT_SKIP}"

[ ... ]

> @@ -1805,6 +1817,51 @@ test_ns_guest_assign_g2h_netns_reset_on_ns_delete_ok() {
>  	return "${KSFT_PASS}"
>  }
>  
> +test_ns_guest_assign_g2h_netns_no_cap_net_admin_fails() {
> +	local cmd="unshare -n setpriv --bounding-set=-net_admin"
> +	local rc
> +
> +	vm_ssh "init_ns" -- "${cmd}" ./vsock_assign_g2h_netns &>/dev/null
> +	rc=$?
> +
> +	if [[ "${rc}" -ne "${EPERM}" ]]; then
> +		log_host "expected EPERM (${EPERM}) without CAP_NET_ADMIN, got ${rc}"
> +		return "${KSFT_FAIL}"
> +	fi
> +
> +	return "${KSFT_PASS}"
> +}

[Severity: Medium]
Can this test pass without ever issuing the ioctl?

The helper turns errno into its own exit status:

tools/testing/selftests/vsock/vsock_assign_g2h_netns.c:main() {
	ret = ioctl(fd, IOCTL_VM_SOCKETS_ASSIGN_G2H_NETNS);
	if (ret < 0) {
		ret = errno;
	...
	return ret;
}

so "the ioctl returned -EPERM" is encoded as exit status 1, which is also
the generic EXIT_FAILURE of the two wrappers that are prepended here.  If
util-linux unshare cannot create the network namespace, or setpriv was
built without libcap-ng and cannot handle --bounding-set=-net_admin, the
wrapper exits 1 before exec'ing the helper, rc is 1, and the test returns
KSFT_PASS with vsock_dev_do_ioctl() never reached.

The &>/dev/null redirection discards the wrapper's own error message, so
that outcome is silent.

check_deps() only verifies that the setpriv binary exists, not that
--bounding-set is supported, so an unsupported setpriv becomes a pass
rather than a skip.

The same conflation applies to the second test below: the pre-flight probe
runs only "unshare -U true", while the command actually executed is
"${unpriv} unshare -Urn ${helper}".  A failure of the -r or -n part, or of
setpriv --reuid/--regid/--clear-groups, also exits 1 and is read as the
ioctl's EPERM.

Would it be better for the helper to report a distinct sentinel exit code
(or print a machine-checkable marker) for the ioctl result, so that the
kernel's capable(CAP_NET_ADMIN) check in vsock_dev_do_ioctl() cannot be
weakened without these tests noticing?

> +
> +test_ns_guest_assign_g2h_netns_unpriv_user_ns_fails() {
> +	local helper=/tmp/vsock_assign_g2h_netns
> +	local unpriv_uid=65534
> +	local unpriv
> +	local rc
> +
> +	unpriv="setpriv --reuid=${unpriv_uid} --regid=${unpriv_uid}"
> +	unpriv="${unpriv} --clear-groups"
> +
> +	if ! vm_ssh "init_ns" -- "${unpriv} unshare -U true"; then
> +		log_host "unprivileged user namespaces unavailable, skipping"
> +		return "${KSFT_SKIP}"
> +	fi
> +
> +	# The home shared with the guest is root-only, so place the helper where
> +	# an unprivileged user can execute it.
> +	vm_ssh "init_ns" -- \
> +		"cp ./vsock_assign_g2h_netns ${helper} && chmod 755 ${helper}"
> +
> +	vm_ssh "init_ns" -- "${unpriv} unshare -Urn ${helper}" &>/dev/null
> +	rc=$?
> +
> +	if [[ "${rc}" -ne "${EPERM}" ]]; then
> +		log_host "expected EPERM (${EPERM}) for an unprivileged user, got ${rc}"
> +		return "${KSFT_FAIL}"
> +	fi

[Severity: Medium]
Can uid 65534 open /dev/vsock in this guest at all?

The node is created from a miscdevice with no .mode:

net/vmw_vsock/af_vsock.c
static struct miscdevice vsock_device = {
	.name		= "vsock",
	.fops		= &vsock_device_ops,
};

misc_devnode() only overrides the mode when c->mode is non-zero, and
devtmpfs_create_node() then applies mode 0600 with uid/gid
GLOBAL_ROOT_UID/GLOBAL_ROOT_GID.  In the bare virtme-ng guest this script
boots there is no udev rule loosening it, so /dev/vsock is 0600 root:root.

With unshare -Urn only kuid 65534 is mapped, so the inode's kuid 0 is
unmapped in the new user namespace and capable_wrt_inode_uidgid() cannot
grant CAP_DAC_OVERRIDE.  open("/dev/vsock", O_RDONLY) then fails with
EACCES and the helper does:

tools/testing/selftests/vsock/vsock_assign_g2h_netns.c:main() {
	fd = open("/dev/vsock", O_RDONLY);
	if (fd < 0) {
		fprintf(stderr, "open /dev/vsock: %s\n", strerror(errno));
		return -1;
	}

which is exit status 255, so this rc check reports "expected EPERM (1) for
an unprivileged user, got 255" and returns KSFT_FAIL even though the
kernel behaved as intended.  The stderr message explaining why is dropped
by &>/dev/null.

Would it make sense to test whether the unprivileged uid can open
/dev/vsock first and return KSFT_SKIP if it cannot, so that an
inaccessible device node is distinguished from a refusal by the
CAP_NET_ADMIN check?

> +
> +	return "${KSFT_PASS}"
> +}
> +

[Severity: Low]
Both new tests are listed in USE_SHARED_VM but neither calls vm_reset_g2h,
while every other g2h-assign test does so on both the success and failure
paths, for example test_ns_guest_assign_g2h_netns_connect_to_host_ok(),
..._init_ns_connect_fails(), ..._host_connect_ok() and
..._reset_on_ns_delete_ok().

If the capable(CAP_NET_ADMIN) gate in vsock_dev_do_ioctl() is ever
weakened, the assign succeeds inside the throwaway unshare -n or
unshare -Urn namespace and these tests return KSFT_FAIL with the guest's
vsock device still owned by a dying namespace.

The neighbouring test already documents that the hand-back is not
immediate:

	# The holder is gone, but the namespace itself is dismantled from a
	# workqueue, so the device does not come back the same instant. Retry
	# until it does, rather than expecting the first send to succeed.

Could the following shared-VM tests then run while the device is still
unavailable, turning one real failure into a series of spurious ones?
Should these two tests call vm_reset_g2h before returning, like the others?

>  shared_vm_test() {
>  	local tname
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902-vsock-guest-ns-v1-0-9995383e9a8b%40meta.com