Re: [PATCH v5 03/10] crypto: pkcs7: add tests for pkcs7_get_authattr

[sashiko-bot@kernel.org]

Sashiko AI review found 1 potential issue(s):
- [Medium] Test failures are silently masked by unconditionally returning 0.
--

commit 11447b57d9861b91fcbcb5b89dcfd93489f5f252
Author: James Bottomley <James.Bottomley@HansenPartnership.com>

crypto: pkcs7: add tests for pkcs7_get_authattr

This commit adds example code to the pkcs7_key_type test module to verify
a message and extract a known authenticated attribute.

> diff --git a/crypto/asymmetric_keys/pkcs7_key_type.c b/crypto/asymmetric_keys/pkcs7_key_type.c
> index b930d3bbf1af5..e0b1ce0202f6d 100644
> --- a/crypto/asymmetric_keys/pkcs7_key_type.c
> +++ b/crypto/asymmetric_keys/pkcs7_key_type.c
[ ... ]
> @@ -51,16 +52,57 @@ static int pkcs7_view_content(void *ctx, const void *data, size_t len,
>  static int pkcs7_preparse(struct key_preparsed_payload *prep)
>  {
[ ... ]
> +	/*
> +	 * the parsed message has no trusted signer, so nothing should
> +	 * be returned here
> +	 */
> +	ret = pkcs7_get_authattr(pkcs7, OID_messageDigest, &data, &len);
> +	if (ret == 0) {
> +		pr_err("OID returned when no trust in signer\n");
> +		goto out;

Should a negative error code be assigned to ret here before jumping to out?

> +	}
> +	/* add trust and check again */
> +	ret = verify_pkcs7_message_sig(NULL, 0, pkcs7,
> +				       VERIFY_USE_SECONDARY_KEYRING, usage,
> +				       NULL, NULL);
> +	if (ret) {
> +		pr_err("verify_pkcs7_message_sig failed!!\n");
> +		goto out;
> +	}
> +	/* now we should find the OID */
> +	ret = pkcs7_get_authattr(pkcs7, OID_messageDigest, &data, &len);
> +	if (ret) {
> +		pr_err("Failed to get message digest\n");
> +		goto out;
> +	}
> +	pr_info("Correctly Got message hash, size=%zu\n", len);
> +
> + out:
> +	pkcs7_free_message(pkcs7);
> +	return 0;

Does this return 0 unconditionally mask the test failures?

If any of the checks above fail and jump to the out label, the function
returns 0, effectively discarding the error state. As a result, the key
instantiation will always succeed from the perspective of user space, and
test frameworks might incorrectly assume the test passed.

Should this return ret instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260420212653.438685-1-bboscaccy@linux.microsoft.com?part=3