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

[Srish Srinivasan <ssrish@linux.ibm.com>]

Hi

On 11/5/26 8:26 PM, sashiko-bot@kernel.org wrote:
> 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()
> --


Yes, these are valid findings and I will fix these in my next version.

Thanks,
Srish.


>
> 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?
>