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

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Neither new privilege test checks why the command failed.
- [Low] Both new tests are in USE_SHARED_VM, but they skip the shared-VM
  cleanup that the other ns_guest_assign_g2h_netns_* tests do.

--

selftests/vsock: test the netns assign privilege checks

This patch adds two shared-VM tests to vmtest.sh. One checks that
VSOCK_CMD_DEV_NETNS_SET is refused without CAP_NET_ADMIN. The other checks
that it is refused when CAP_NET_ADMIN is held only in an unprivileged user
namespace. The patch also adds setpriv to the ns_guest test dependencies
and CONFIG_USER_NS to the selftest config.

> diff --git a/tools/testing/selftests/vsock/vmtest.sh b/tools/testing/selftests/vsock/vmtest.sh
> index 7693d5e0a1d07..d5d7b0660c5da 100755
> --- a/tools/testing/selftests/vsock/vmtest.sh
> +++ b/tools/testing/selftests/vsock/vmtest.sh

[ ... ]

> @@ -1909,6 +1919,48 @@ test_ns_guest_assign_g2h_netns_pins_ns_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}" python3 /root/ynl/cli.py --no-schema \
> +		--spec /root/ynl/vsock.yaml --do dev-netns-set &>/dev/null
> +	rc=$?
> +
> +	if [[ "${rc}" -eq 0 ]]; then

[Severity: Medium]
Does this check show that the refusal came from the CAP_NET_ADMIN check?

vm_ssh returns the exit status of the whole remote command chain, so any
non-zero status counts as a pass here. That includes:

  - ssh transport failure (255)
  - unshare -n failing
  - setpriv rejecting --bounding-set=-net_admin
  - python or ynl import errors, or a genl family lookup failure
  - non-permission errors from vsock_g2h_net_assign(), such as -ENODEV,
    -EOPNOTSUPP, -EADDRINUSE or -EMSGSIZE

test_ns_guest_assign_g2h_netns_unpriv_user_ns_fails() below uses the same
pattern. Its skip probe also only runs:

    vm_ssh "init_ns" -- "${unpriv} unshare -U true"

If the -r uid/gid mapping or the -n netns creation fails inside
"unshare -Urn", that test reports PASS instead of SKIP.

The changelog says:

  "Assert only that the command fails, not EPERM: ynl's cli.py does not
  report the errno"

Is that still accurate? In tools/net/ynl/pyynl/cli.py, main() does:

    except NlError as e:
        print(e)
        sys.exit(1)

NlError.__str__() in tools/net/ynl/pyynl/lib/ynl.py appends
os.strerror(self.error), and NlError.error holds the errno itself.

So when the GENL_ADMIN_PERM check on VSOCK_CMD_DEV_NETNS_SET returns EPERM,
cli.py prints "Netlink error: Operation not permitted" to stdout. Here,
&>/dev/null throws that output away.

The commit message says:

  "one confirms that CAP_NET_ADMIN is required even by a privileged user
  and the other confirms that CAP_NET_ADMIN in an unprivileged user ns
  alone is insufficient."

As written, both tests only confirm that the command did not succeed. If
the permission check were removed or relaxed while some unrelated failure
also happened, both tests would still pass.

Could the tests capture the cli.py output and check for "Operation not
permitted"?

> +		log_host "assign unexpectedly succeeded without CAP_NET_ADMIN"
> +		return "${KSFT_FAIL}"
> +	fi
> +
> +	return "${KSFT_PASS}"
> +}
> +
> +test_ns_guest_assign_g2h_netns_unpriv_user_ns_fails() {

[ ... ]

> +	vm_ssh "init_ns" -- "${unpriv} unshare -Urn" \
> +		python3 /root/ynl/cli.py --no-schema \
> +		--spec /root/ynl/vsock.yaml --do dev-netns-set &>/dev/null
> +	rc=$?
> +
> +	if [[ "${rc}" -eq 0 ]]; then
> +		log_host "assign unexpectedly succeeded for unprivileged user"
> +		return "${KSFT_FAIL}"
> +	fi

[Severity: Low]
Should this path, and the matching one in
test_ns_guest_assign_g2h_netns_no_cap_net_admin_fails(), call
vm_reset_g2h() before returning?

Both new tests are in USE_SHARED_VM. The other ns_guest_assign_g2h_netns_*
tests, such as test_ns_guest_assign_g2h_netns_pins_ns_ok(), call
vm_reset_g2h() on their failure paths once the device may have moved.

If the assign unexpectedly succeeds, dev-netns-set moves the guest's g2h
vsock device into the throwaway netns created by unshare -n or
unshare -Urn.

The kernel-doc for vsock_g2h_net_assign() says the move "keeps @net alive
until the device leaves again". So after the command exits, the device
stays in that orphaned netns.

run_shared_vm_tests() runs tests in the order given on the command line.
For example:

    vmtest.sh ns_guest_assign_g2h_netns_no_cap_net_admin_fails \
        vm_server_host_client

Here the later shared-VM test would run with the guest's init netns cut
off from vsock, and its failure would be reported against the wrong test.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012-vsock-guest-ns-v3-0-b32447c66b23%40meta.com