Re: [PATCH v6 7/9] pseries/plpks: add HCALLs for PKWM wrapping key life cycle management

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] pseries/plpks: output_buf leak and -EAGAIN return in plpks_get_object_labels()
--

commit 41ef08b282e00765eba284dad461f1985057d371
Author: Srish Srinivasan <ssrish@linux.ibm.com>

pseries/plpks: add HCALLs for PKWM wrapping key life cycle management

This commit adds support for the H_PKS_REVOKE_OBJECT, H_PKS_UNREVOKE_OBJECT,
and H_PKS_GET_OBJECTLABELS hypercalls to manage the life cycle of PKWM
wrapping keys and retrieve wrapping key labels in the Platform Keystore.

> diff --git a/arch/powerpc/platforms/pseries/plpks.c b/arch/powerpc/platforms/pseries/plpks.c
> index d5adc563f71f0..6e10bd6a9a481 100644
> --- a/arch/powerpc/platforms/pseries/plpks.c
> +++ b/arch/powerpc/platforms/pseries/plpks.c
[ ... ]
> @@ -1205,6 +1237,457 @@ int plpks_unwrap_object(u8 **input_buf, u64 input_len, u8 **output_buf,
[ ... ]
> +int plpks_get_object_labels(u8 **output_buf, u64 *output_len,
> +			    char *comp_prefix)
> +{
> +	unsigned long retbuf[PLPAR_HCALL_BUFSIZE] = { 0 };
> +	u8 *labels_buf = NULL;
> +	u8 *tmp_buf = NULL;
> +	struct plpks_auth *auth;
> +	struct label_attr *metadata = NULL;
> +	u8 prefix[PLPKS_OBJLABEL_PREFIX_LEN];
> +	u16 label_len;
> +	u64 labels_count;
> +	u64 continuetoken = 0, output_buf_len = 0;
> +	int rc, pseries_status;
> +	size_t labels_buf_offset = 0, output_buf_offset = 0;
> +	size_t obj_label_entry_size, i;
> +
> +	if (!output_buf || !output_len || !comp_prefix)
> +		return -EINVAL;
> +
> +	*output_buf = NULL;
> +	*output_len = 0;
> +
> +	memset(prefix, 0, sizeof(prefix));
> +	memcpy(prefix, comp_prefix, strnlen(comp_prefix, sizeof(prefix)));
> +
> +	auth = construct_auth(PLPKS_OS_OWNER);
> +	if (IS_ERR(auth))
> +		return PTR_ERR(auth);
> +
> +	do {
> +		labels_buf =
> +			kzalloc(PLPKS_OBJLABEL_BUF_SIZE, GFP_KERNEL);
> +
> +		if (!labels_buf) {
> +			rc = -ENOMEM;
> +			goto out_free_output_buf;
> +		}
> +
> +		rc = plpar_hcall(H_PKS_GET_OBJECTLABELS, retbuf,
> +				 virt_to_phys(auth), continuetoken,
> +				 virt_to_phys(labels_buf),
> +				 PLPKS_OBJLABEL_BUF_SIZE);
> +
> +		pseries_status = rc;
> +		rc = pseries_status_to_err(rc);
> +
> +		if (rc && rc != -EAGAIN) {
> +			pr_err("H_PKS_GET_OBJECTLABELS failed. pseries_status=%d rc=%d\n",
> +			       pseries_status, rc);
> +			goto out_free_labels_buf;
> +		} else {
> +			/*
> +			 * Setting an incorrect countinuetoken upon
> +			 * receiving H_CONTINUE would result in H_P2. Since
> +			 * the continuetoken is being set to the expected
> +			 * value from the previous call, H_P2 must not be
> +			 * returned.
> +			 */
> +			continuetoken = retbuf[1];
> +
> +			labels_count = retbuf[0];
> +			if (!labels_count) {
> +				kfree(labels_buf);
> +				labels_buf = NULL;
> +				goto out;
> +			}
> +
> +			/*
> +			 * Filter out object labels that don't have the provided
> +			 * component prefix.
> +			 */
> +
> +			output_buf_len += PLPKS_OBJLABEL_BUF_SIZE;
> +
> +			tmp_buf = krealloc(*output_buf, output_buf_len,
> +					   GFP_KERNEL);
> +
> +			if (!tmp_buf) {
> +				rc = -ENOMEM;
> +				goto out_free_labels_buf;
> +			}
> +
> +			*output_buf = tmp_buf;
> +
> +			for (i = 0; i < labels_count; ++i) {
> +				if (labels_buf_offset +
> +				    PLPKS_OBJLABEL_LEN_FIELD_SIZE >
> +				    PLPKS_OBJLABEL_BUF_SIZE) {
> +					rc = -EIO;
> +					goto out_free_labels_buf;
> +				}
> +
> +				label_len =
> +					get_unaligned_be16(labels_buf +
> +							   labels_buf_offset);
> +
> +				obj_label_entry_size =
> +					PLPKS_OBJLABEL_LEN_FIELD_SIZE +
> +					label_len;
> +
> +				if (label_len < sizeof(struct label_attr) ||
> +				    labels_buf_offset + obj_label_entry_size >
> +				    PLPKS_OBJLABEL_BUF_SIZE) {
> +					rc = -EIO;
> +					goto out_free_labels_buf;
> +				}
> +
> +				metadata =
> +					(struct label_attr *)(labels_buf +
> +					labels_buf_offset +
> +					PLPKS_OBJLABEL_LEN_FIELD_SIZE);
> +
> +				if (!memcmp(metadata->prefix, prefix,
> +					    PLPKS_OBJLABEL_PREFIX_LEN)) {
> +					memcpy(*output_buf + output_buf_offset,
> +					       labels_buf + labels_buf_offset,
> +					       obj_label_entry_size);
> +
> +					output_buf_offset +=
> +						obj_label_entry_size;
> +					(*output_len) += 1;
> +				}
> +				labels_buf_offset += obj_label_entry_size;
> +			}
> +			kfree(labels_buf);
> +			labels_buf = NULL;
> +			labels_buf_offset = 0;
> +		}
> +	} while (rc == -EAGAIN);
> +
> +	goto out;
> +
> +out_free_labels_buf:
> +	kfree(labels_buf);
> +	labels_buf = NULL;
> +out_free_output_buf:
> +	kfree(*output_buf);
> +	*output_buf = NULL;
> +	*output_len = 0;
> +out:
> +	kfree_sensitive(auth);
> +	return rc;
> +}

[Severity: Medium]
Can plpks_get_object_labels() leak *output_buf or return -EAGAIN to the
caller?

First, inside the do-while loop, when labels_count > 0, *output_buf is
allocated or grown via krealloc() before checking whether any of the returned
labels match comp_prefix. If PLPKS contains objects for other components so
labels_count > 0, but none of those labels match comp_prefix, the loop
completes with rc == 0 and *output_len == 0 and jumps to out without freeing
*output_buf. Unlike the case where PLPKS has 0 total objects on the first
call (which returns 0 with *output_len == 0 and *output_buf == NULL), this
returns 0 with *output_len == 0 while *output_buf points to an allocated
buffer of uninitialized heap memory. Callers checking *output_len == 0 will
leak *output_buf, while callers checking *output_buf != NULL will read
uninitialized memory.

Second, if !labels_count (retbuf[0] == 0) is encountered when
H_PKS_GET_OBJECTLABELS returns H_CONTINUE (converted by
pseries_status_to_err() to rc = -EAGAIN) or on a subsequent continuation
iteration after *output_buf was already allocated, jumping directly to out
bypasses the loop condition and out_free_output_buf. Could that return the
internal continuation code -EAGAIN to the caller and leak any *output_buf
allocated in prior iterations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105144320.357260-1-ssrish@linux.ibm.com?part=7