Re: [LTP] lapi/keyctl.h: Add fallback definitions for extended ops

[linuxtestproject.agent@gmail.com]

Hi Andrea,

On Wed, 02 Sep 2026 13:04:16 +0200, Andrea Cervesato wrote:
> lapi/keyctl.h: Add fallback definitions for extended ops

--- [PATCH 1/33] ---

> +#if !defined(HAVE_KEYUTILS_H) && !defined(HAVE_LINUX_KEYCTL_H)
> +struct keyctl_dh_params {

This block sits in the #else branch of
"defined(HAVE_KEYUTILS_H) && defined(HAVE_LIBKEYUTILS)", so <keyutils.h>
is never included on this path and only <linux/keyctl.h> can declare the
structs. With keyutils.h present (HAVE_KEYUTILS_H, configure.ac:59) but
libkeyutils not linkable (m4/ltp-keyutils.m4) and linux/keyctl.h absent,
the guard is false and nothing declares keyctl_dh_params /
keyctl_kdf_params, breaking keyctl25..keyctl28. Use
"#ifndef HAVE_LINUX_KEYCTL_H". The same guard is reused for
keyctl_pkey_query / keyctl_pkey_params in patch 21/33.

> +#ifndef KEYCTL_RESTRICT_KEYRING
> +# define KEYCTL_RESTRICT_KEYRING 29
> +#endif

Commands 29 and 30 are appended after KEYCTL_WATCH_KEY (32), and patches
21/33 and 27/33 then insert 24..28 and 31 after those. The rest of the
list is in ascending command order.

--- [PATCH 2/33] ---

> +	TEST(keyctl(KEYCTL_DESCRIBE, key, (unsigned long)NULL, 0, 0));
> +	if (TST_RET < 0)
> +		tst_brk(TBROK | TTERRNO, "KEYCTL_DESCRIBE failed");
> +
> +	desc_len = TST_RET;

safe_keyctl() already treats a negative KEYCTL_DESCRIBE return as a
failure, and the previous line in the same function uses SAFE_KEYCTL().
This is a setup precondition, not the tested call, so the "subject
syscall" exception does not apply:

	desc_len = SAFE_KEYCTL(KEYCTL_DESCRIBE, key, 0, 0, 0);

Same in keyctl11, keyctl12, keyctl16, keyctl34, keyctl35 and keyctl36.

> +	if (strcmp(type, "user")) {
> +		tst_res(TFAIL, "wrong key type '%s', expected 'user'", type);
> +		return;
> +	}

Use TST_EXP_EQ_STR(type, "user") instead of strcmp() + tst_res(). Same
for the strcmp() against KEY_DESC below.

--- [PATCH 3/33] ---

> +	memset(buf, 0, desc_len);
> +	TST_EXP_EQ_LI_SILENT(keyctl(KEYCTL_DESCRIBE, key, (unsigned long)buf,
> +				    desc_len, 0), desc_len);
> +	if (!TST_PASS)
> +		return;
> +
> +	if (buf[desc_len - 1] != '\0') {
> +		tst_res(TFAIL, "description is not NUL terminated");

buf is zeroed immediately before the call, so buf[desc_len - 1] is
already '\0' and this check cannot fail even if the kernel copied
nothing. Nothing verifies the content either. Poison the buffer
(memset(buf, 'x', desc_len)) and assert
strlen(buf) + 1 == (size_t)desc_len, as keyctl12 already does with 0xAA.

> +	TST_EXP_EQ_LI_SILENT(keyctl(KEYCTL_DESCRIBE, key, (unsigned long)buf,
> +				    desc_len, 0), desc_len);

TST_EXP_EQ_*() compare two plain values and do not go through TEST(), so
errno is lost on failure. For syscall return values use
TST_EXP_VAL()/TST_EXP_VAL_SILENT(), or TST_EXP_PASS()/
TST_EXP_PASS_SILENT() when 0 is expected. Recurs in keyctl12, keyctl16,
keyctl18, keyctl19, keyctl20, keyctl25, keyctl27 and keyctl29..keyctl36.

--- [PATCH 7/33] ---

> +	memset(buf, 0, sizeof(buf));
> +
> +	TEST(keyctl(KEYCTL_GET_SECURITY, key, (unsigned long)buf, sizeof(buf), 0));
...
> +	if (TST_RET == 1) {
> +		if (buf[0] != '\0') {
> +			tst_res(TFAIL, "empty label is not NUL terminated");
...
> +	tst_res(TPASS, "security label returned, full length %ld", TST_RET);

Same tautology as keyctl11: buf is zeroed first, so buf[0] == '\0'
always holds. The TST_RET > 1 branch reports TPASS without inspecting
buf at all. Poison buf and assert strlen(buf) + 1 == (size_t)TST_RET
whenever TST_RET <= sizeof(buf); that covers both branches.

--- [PATCH 14/33] ---

> +	TEST(keyctl(KEYCTL_RESTRICT_KEYRING, ring_reject, 0, 0, 0));
> +	if (TST_RET == 0)
> +		tst_res(TPASS, "KEYCTL_RESTRICT_KEYRING with NULL type and restriction passed");
> +	else if (TST_RET == -1 && TST_ERR == EEXIST)
> +		tst_res(TPASS, "KEYCTL_RESTRICT_KEYRING reject-all already active");

Restricting a keyring is one-shot (keyring_restrict() returns -EEXIST
once restrict_link is set), so this branch only exists to survive -i N,
but it also accepts EEXIST on the first iteration where the keyring has
never been restricted. Move the restriction into setup(), which the
framework calls once, and keep run() to the EPERM assertions. Same
pattern in keyctl23.

--- [PATCH 15/33] ---

> +	TST_EXP_FAIL(keyctl(KEYCTL_LINK, user_key, ring_builtin, 0, 0),
> +		     EOPNOTSUPP,
> +		     "KEYCTL_LINK of non-asymmetric key on builtin_trusted restricted keyring");
> +
> +	TST_EXP_FAIL2(add_key("asymmetric", "cert", untrusted_cert,
> +			      sizeof(untrusted_cert), ring_builtin), ENOKEY,

EOPNOTSUPP and ENOKEY come from restrict_link_by_signature(), which is
only reachable with CONFIG_SYSTEM_TRUSTED_KEYRING=y. Without it,
include/keys/system_keyring.h does
"#define restrict_link_by_builtin_trusted restrict_link_reject", both
calls return EPERM and the test reports two TFAILs instead of skipping.
The restriction still installs in that configuration, so the existing
EOPNOTSUPP TCONF branch never fires. Add:

	.needs_kconfigs = (const char *[]) {
		"CONFIG_SYSTEM_TRUSTED_KEYRING=y",
		NULL
	},

Note certs/Kconfig makes SYSTEM_TRUSTED_KEYRING depend on
X509_CERTIFICATE_PARSER=y, so whenever modprobe actually loads
x509_key_parser as a module the option is off and the test always fails.

> +	TEST(keyctl(KEYCTL_RESTRICT_KEYRING, ring_builtin,
> +		    (unsigned long)"asymmetric", (unsigned long)"bogus", 0));
> +	asym_supported = (TST_RET != -1 || TST_ERR != ENODEV);

KEYCTL_RESTRICT_KEYRING resolves the type through keyring_restrict() ->
key_type_lookup(), which returns ENOKEY for an unregistered type; ENODEV
is what add_key() returns (security/keys/key.c:830), as
add_asymmetric_key_or_tconf() correctly assumes in patch 22/33. With a
registered type the probe gets EINVAL from "bogus", so asym_supported is
always 1 and the TCONF branch is dead code. Compare against ENOKEY.

--- [PATCH 16/33] ---

> +	asym_supported = (TST_RET != -1 || TST_ERR != ENODEV);

Same wrong errno as keyctl23. Here the dead TCONF branch also turns two
tcases into spurious TFAILs on a kernel without CONFIG_ASYMMETRIC_KEY_TYPE:
"asymmetric with invalid restriction string" expects EINVAL and
"self-referencing key_or_keyring chain" expects EDEADLK, but both receive
ENOKEY.

> +	keyctl(KEYCTL_UNLINK, probe_ring, KEY_SPEC_PROCESS_KEYRING, 0, 0);

These housekeeping unlinks are not the tested call and must succeed. Use
SAFE_KEYCTL(); as written a failure is silently discarded. Same for the
fresh_ring unlink in verify_negative().

--- [PATCH 17/33] ---

> +/* RFC 7919 2048-bit FFDHE Group parameters (ffdhe2048) */
> +static const unsigned char dh_prime[] = {
> +	0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xc9, 0x0f, 0xda, 0xa2, 0x21, 0x68, 0xc2, 0x34,

This prime is the RFC 3526 2048-bit MODP group (Group 14 / modp2048),
not RFC 7919 ffdhe2048 - ffdhe2048 continues 0xad 0xf8 0x54 0x58 after
the leading 0xff bytes. The commit message repeats the claim. The vector
itself is correct: pow(dh_base, dh_priv, dh_prime) reproduces
dh_expected_secret byte for byte. The header is reused by
keyctl26..keyctl28, so fix the comment and the commit message.

--- [PATCH 28/33] ---

> +	TEST(keyctl(KEYCTL_CAPABILITIES, (unsigned long)caps, sizeof(caps), 0, 0));
> +	if (TST_RET < 0)
> +		tst_brk(TBROK | TTERRNO, "KEYCTL_CAPABILITIES failed");

Patch 27/33 adds "case KEYCTL_CAPABILITIES" to safe_keyctl(), and patch
21/33 adds the five KEYCTL_PKEY_* cases, but no test in the series ever
calls SAFE_KEYCTL() with any of them. Either use SAFE_KEYCTL() here and
in keyctl35/keyctl36, or drop the unused cases.

--- [PATCH 32/33] ---

> +	.ulimit = (const struct tst_ulimit_val []) {
> +		{RLIMIT_NOFILE, 524288},
> +		{}
> +	},

set_ulimit_() (lib/tst_test.c:1330) raises rlim_max when rlim_cur exceeds
it, which needs CAP_SYS_RESOURCE, and safe_setrlimit() aborts with
TBROK | TERRNO on failure. The test does not set .needs_root, so on any
host whose hard RLIMIT_NOFILE is below 524288 an unprivileged run ends in
TBROK before a single assertion runs - reproduced here with a hard limit
of 65536, where the same getrlimit/setrlimit sequence returns EPERM. The
test holds one watch at a time, so the default soft limit already
suffices; keyctl39 exercises the same code with no .ulimit at all.

> +	TST_EXP_PASS(keyctl(KEYCTL_WATCH_KEY, key, pipefd[0], 1),
> +		     "KEYCTL_WATCH_KEY add watch on key");

The keyctl() helper in lapi/keyctl.h unconditionally reads four variadic
arguments (arg2..arg5); only three are passed here, which is undefined
behaviour. Append an explicit 0, as every other call site in this series
does. Same in keyctl39.

> +	TEST(pipe2(pipefd, O_NOTIFICATION_PIPE));
> +	if (TST_RET < 0) {
> +		if (TST_ERR == ENOPKG)
> +			tst_brk(TCONF | TTERRNO, "CONFIG_WATCH_QUEUE is not set");

This block plus the IOC_WATCH_QUEUE_SET_SIZE call duplicates
wqueue_watch() in testcases/kernel/watchqueue/common.h:100-116.

Verdict - Needs revision

Pre-existing issues:

include/lapi/keyctl.h already fails checkpatch on master with one ERROR
("open brace '{' following function definitions go on the next line",
keyctl_join_session_keyring()) and two CHECKs (multiple blank lines,
missing blank line after a declaration).

---
Note:

The agent can sometimes produce false positives although often its
findings are genuine. If you find issues with the review, please
comment this email or ignore the suggestions.

Regards,
LTP AI Reviewer

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp