Re: [PATCH v4 06/17] KVM: arm64: Add pkvm_hyp_req infrastructure

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Phantom hypervisor requests leading to infinite loop and memory leak on hypercall errors.
--

--- Patch [6]: [PATCH v4 06/17] KVM: arm64: Add pkvm_hyp_req infrastructure ---
commit ec846697735578e1b5a4e25b88e07602c29ae6a2
Author: Vincent Donnefort <[email protected]>

KVM: arm64: Add pkvm_hyp_req infrastructure

Introduce a struct pkvm_hyp_req to enable the pKVM hypervisor to request
resources from the host. Provide serialisation helpers to transport these
requests via SMCCC registers and a macro to automate HVC retries.

> diff --git a/arch/arm64/include/asm/kvm_pkvm.h b/arch/arm64/include/asm/kvm_pkvm.h
> index 74fedd9c5ff02..6c2497047f8d8 100644
> --- a/arch/arm64/include/asm/kvm_pkvm.h
> +++ b/arch/arm64/include/asm/kvm_pkvm.h

[ ... ]

> +/* Return true if a pkvm_hyp_req has been decoded from the SMCCC args */
> +static inline bool smccc_to_pkvm_hyp_req(struct pkvm_hyp_req *req, struct arm_smccc_res *res)
> +{
> +	u8 *src = (u8 *)res + offsetof(struct arm_smccc_res, a2);
> +	u8 type = *src;
> +
> +	if (type == PKVM_HYP_NO_REQ || type >= __PKVM_HYP_REQ_TYPE_MAX)
> +		return false;
> +
> +	req->type = type;
> +	memcpy(&req->args, src + 1, pkvm_hyp_req_arg_size(type));
> +
> +	return true;
> +}
> +
> +int __pkvm_handle_smccc_req(struct arm_smccc_res *res);
> +
> +#define pkvm_call_hyp_req(f, ...)								\
> +({												\
> +	struct arm_smccc_res __res;								\
> +	int __ret;										\
> +	do {											\
> +		__ret = -1;									\
> +		arm_smccc_1_1_hvc(KVM_HOST_SMCCC_FUNC(f), ##__VA_ARGS__, &__res);		\
> +		if (WARN_ON(__res.a0 != SMCCC_RET_SUCCESS))					\
> +			break;									\
> +		__ret = __res.a1;								\
> +		if (!__ret)									\
> +			break;									\
> +		__ret = __pkvm_handle_smccc_req(&__res);					\
> +	} while (!__ret);									\
> +	__ret;											\
> +})

[Severity: High]
Can this sequence lead to an infinite loop and memory leak on hypercall errors?

When the hypercall fails and returns a non-zero error in __res.a1, the macro
passes the result to __pkvm_handle_smccc_req() to check for a piggybacked
request in __res.a2.

Standard EL2 handlers typically do not clear the x2 register (which maps to a2)
on error; they set x1 to the error code and return. This means a2 retains the
original second hypercall argument passed by the host.

If the least significant byte of that original host argument happens to match
a valid request type, smccc_to_pkvm_hyp_req() would mistakenly decode a phantom
request. The host would handle this phantom request, return success, and the
macro would then infinitely retry the failing hypercall.

Is there a way to explicitly distinguish a hypercall error from a legitimate
hypervisor request, perhaps using a specific return code in a1 to indicate when
a2 actually contains a request?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6