Re: [PATCH v4 07/10] hornet: Introduce gen_sig

[sashiko-bot@kernel.org]

Sashiko AI review found 5 potential issue(s):
- [High] Infinite loop during argument parsing on architectures with unsigned `char`.
- [High] Unchecked `sha256()` return value leads to usage of uninitialized stack memory.
- [Medium] Stack buffer overflow when parsing `--add` arguments.
- [Medium] Unchecked return values for OpenSSL serialization and file output lead to silent failures.
- [Medium] `write-sig.sh` outputs to stdout instead of modifying the target file in-place.
--

commit 5e34aa795f3e17cbd08a08fff14bbdb70263937f
Author: Blaise Boscaccy <bboscaccy@linux.microsoft.com>

hornet: Introduce gen_sig

This introduces the gen_sig tool to create a pkcs#7 signature of a
data payload. It additionally appends a signed attribute containing a
set of hashes.

> diff --git a/scripts/hornet/gen_sig.c b/scripts/hornet/gen_sig.c
> new file mode 100644
> index 0000000000000..f966516ebc99b
> --- /dev/null
> +++ b/scripts/hornet/gen_sig.c

[ ... ]

> +int main(int argc, char **argv)
> +{
> +	const char *cert_path = NULL;
> +	const char *key_path = NULL;
> +	const char *data_path = NULL;
> +	const char *out_path = NULL;
> +
> +	X509 *signer;
> +	EVP_PKEY *pkey;
> +	BIO *data_in;
> +	CMS_ContentInfo *cms_out;
> +	struct hash_spec hashes[MAX_HASHES];
> +	int hash_count = 0;
> +	int flags;
> +	CMS_SignerInfo *si;
> +	MAP_SET *set;
> +	unsigned char hash_buffer[SHA256_LEN];
> +	unsigned int hash_len;
> +	ASN1_OBJECT *oid;
> +	unsigned char *der = NULL;
> +	int der_len;
> +	int err;
> +	BIO *b_out;
> +	int i;
> +	char opt;

Could this cause an infinite loop on certain architectures?

Because opt is a char, on platforms where char is unsigned by default (such as
ARM and PowerPC), it cannot hold the value -1. 

> +
> +	const char *short_opts = "C:K:P:O:A:Sh";
> +
> +	static const struct option long_opts[] = {
> +		{"cert", required_argument, 0, 'C'},
> +		{"key",  required_argument, 0, 'K'},
> +		{"pass",  required_argument, 0, 'P'},
> +		{"out",  required_argument, 0, 'O'},
> +		{"data",  required_argument, 0, 'D'},
> +		{"add",  required_argument, 0, 'A'},
> +		{"help",    no_argument,       0, 'h'},
> +		{0, 0, 0, 0}
> +	};
> +
> +	while ((opt = getopt_long_only(argc, argv, short_opts, long_opts, NULL)) != -1) {

When getopt_long_only() returns -1, it would be converted to 255. The loop
condition 255 != -1 would evaluate to true, resulting in an infinite loop that
could hang the build process.

> +		switch (opt) {

[ ... ]

> +		case 'A':
> +			if (strchr(optarg, ':')) {
> +				hashes[hash_count].file = strsep(&optarg, ":");
> +				hashes[hash_count].index = atoi(optarg);
> +				hash_count++;

Is it possible to overflow the hashes array here?

The hashes array is allocated with a fixed capacity of MAX_HASHES (64). Since
hash_count is incremented without bounds checking, providing more than 64 --add
flags could write out of bounds on the stack.

> +			} else {
> +				usage(argv[0]);
> +				return EXIT_FAILURE;
> +			}
> +		}
> +	}

[ ... ]

> +	set = MAP_SET_new();
> +	set->maps = sk_HORNET_MAP_new_null();
> +
> +	for (i = 0; i < hash_count; i++) {
> +		sha256(hashes[i].file, hash_buffer, &hash_len);

What happens if sha256() fails here?

If sha256() returns a negative error code (for instance, if the file cannot be
opened), the local variables hash_buffer and hash_len remain uninitialized.

> +		add_hash(set, hash_buffer, hash_len, hashes[i].index);

Since the return value of sha256() is unchecked, this could blindly pass
uninitialized stack memory to add_hash(). The garbage value in hash_len might
be very large, causing out of bounds reads from the stack and potentially
embedding uninitialized memory into the generated signature.

> +	}
> +
> +	oid = OBJ_txt2obj("2.25.316487325684022475439036912669789383960", 1);
> +	if (!oid) {
> +		ERR_print_errors_fp(stderr);
> +		DIE("create oid failed");
> +	}
> +
> +	der_len = ASN1_item_i2d((ASN1_VALUE *)set, &der, ASN1_ITEM_rptr(MAP_SET));
> +	CMS_signed_add1_attr_by_OBJ(si, oid, V_ASN1_SEQUENCE, der, der_len);

Can ASN1_item_i2d() fail and return a negative length?

If der_len is negative upon failure, passing it to CMS_signed_add1_attr_by_OBJ()
might cause memory issues inside OpenSSL.

> +
> +	err = CMS_final(cms_out, data_in, NULL, CMS_NOCERTS | CMS_BINARY);
> +	ERR(!err, "cms final failed");
> +
> +	OPENSSL_free(der);
> +	MAP_SET_free(set);
> +
> +	b_out = bio_open_wr(out_path);
> +	ERR(!b_out, "opening output path failed");
> +
> +	i2d_CMS_bio_stream(b_out, cms_out, NULL, 0);

Does this silently ignore write failures?

If writing the final signature to the output file fails (e.g., due to a full
disk or an I/O error), ignoring the return value of i2d_CMS_bio_stream() means
the tool will still exit with a 0 status code. This could silently fail to
produce the necessary signature artifacts without breaking the build.

> +
> +	BIO_free(data_in);
> +	BIO_free(b_out);
> +	EVP_cleanup();
> +	ERR_free_strings();
> +	return 0;
> +}

> diff --git a/scripts/hornet/write-sig.sh b/scripts/hornet/write-sig.sh
> new file mode 100755
> index 0000000000000..7eaabe3bab9aa
> --- /dev/null
> +++ b/scripts/hornet/write-sig.sh

[ ... ]

> +if [ $ARGC -ne $EXPECTED_ARGS ] ; then
> +    usage
> +else
> +    SIG=$(xxd -p $2 | tr -d '\n' | sed 's/\(..\)/\\\\x\1/g')
> +    sed '/const char opts_sig/,/;/c\\tstatic const char opts_sig[] __attribute__((__aligned__(8))) = "\\\n'"$(printf '%s\n' "$SIG")"'\";' $1

Should this use the -i flag for in-place modification?

Currently, sed prints the modified content to standard output instead of
modifying the target header file, which appears to leave the original file
unchanged.

> +fi

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