Re: [PATCH net-next v6 8/8] selftests/vsock: test the netns assign privilege checks

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In tools/testing/selftests/vsock/vmtest.sh,
  check_assign_g2h_eperm() returns KSFT_FAIL from the "assign…
- [Low] In tools/testing/selftests/vsock/vmtest.sh,
  check_assign_g2h_eperm() passes whenever the ssh command fails and the…

--

selftests/vsock: test the netns assign privilege checks

This adds two vsock selftests that check VSOCK_CMD_DEV_NETNS_SET is
refused without CAP_NET_ADMIN in the init user namespace. One test drops
net_admin from the bounding set, and the other runs as an unprivileged
user inside a new user ns. The patch also adds CONFIG_USER_NS to the
config and makes setpriv a dependency for these two tests.

> diff --git a/tools/testing/selftests/vsock/vmtest.sh b/tools/testing/selftests/vsock/vmtest.sh
> index a800b63dfbb11..9614532a56285 100755
> --- a/tools/testing/selftests/vsock/vmtest.sh
> +++ b/tools/testing/selftests/vsock/vmtest.sh

[ ... ]

> @@ -1951,6 +1970,44 @@ test_ns_guest_assign_g2h_netns_pins_ns_ok() {
>  	return "${KSFT_PASS}"
>  }
>  
> +check_assign_g2h_eperm() {
> +	local wrapper=$1
> +	local out
> +
> +	if out=$(vm_ssh "init_ns" -- "${wrapper}" python3 /root/ynl/cli.py \
> +		--no-schema --spec /root/ynl/vsock.yaml \
> +		--do dev-netns-set 2>&1); then
> +		log_host "assign unexpectedly succeeded under '${wrapper}'"
> +		return "${KSFT_FAIL}"
> +	fi

[Severity: Low]
If the GENL_ADMIN_PERM check ever regresses and dev-netns-set succeeds
here, where does the guest's G2H device end up?

The request carries no netns fd or pid attribute. In that case
vsock_nl_dev_netns_set_doit() assigns the device to the netns the
request came from, which here is the throwaway netns created by
unshare -n or unshare -Urn.

test_ns_guest_assign_g2h_netns_pins_ns_ok() notes that this kind of
assignment keeps the namespace pinned after its process exits:

	# Assigning it back is what releases the deleted namespace.
	vm_reset_g2h

Both new tests are in USE_SHARED_VM. That means run_shared_vm_tests()
keeps running tests on the same VM while the device is still stuck in
a dead netns. With the default order only the second new test is hit.
With a user-chosen order, more of the later shared-VM tests could fail
for unrelated reasons.

The other assign tests in this file call vm_reset_g2h on their failure
paths. Should this branch also call it before returning KSFT_FAIL?

> +
> +	if [[ "${out}" != *"Operation not permitted"* ]]; then
> +		log_host "no EPERM under '${wrapper}': [${out}]"
> +		return "${KSFT_FAIL}"
> +	fi
> +
> +	return "${KSFT_PASS}"
> +}

[Severity: Low]
Could this match an EPERM that never came from the netlink request?

The substring check runs against the combined stdout/stderr of the
whole wrapper chain. Suppose unshare -n or setpriv
--bounding-set=-net_admin fails with EPERM in the guest. The output
would then contain something like:

    unshare: unshare failed: Operation not permitted

and check_assign_g2h_eperm() would return KSFT_PASS even though cli.py
never sent VSOCK_CMD_DEV_NETNS_SET.

test_ns_guest_assign_g2h_netns_unpriv_user_ns_fails() probes its
wrapper with "unshare -Urn true" first and skips if that fails.
test_ns_guest_assign_g2h_netns_no_cap_net_admin_fails() below has no
such probe.

cli.py prints kernel errors through NlError.__str__() in
tools/net/ynl/pyynl/lib/ynl.py, which starts with the "Netlink error: "
prefix followed by the strerror. Would it be tighter to match on that
prefix, or to probe the wrapper in the no_cap test as well?

> +
> +test_ns_guest_assign_g2h_netns_no_cap_net_admin_fails() {
> +	check_assign_g2h_eperm "unshare -n setpriv --bounding-set=-net_admin"
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261113-vsock-guest-ns-v6-0-3737ce4673c0%40meta.com