Re: [PATCH v3] platform/x86/amd/pmf: Fix ACPI buffer validation in APTS path

[Mario Limonciello <superm1@kernel.org>]

On 10/5/26 02:28, Shyam Sundar S K wrote:
> apts_if_call_store_buffer() reads a u16 size from info->buffer.pointer
> without checking it is non-NULL and at least 2 bytes long, so a short or
> NULL firmware buffer can cause an out-of-bounds read.
> 
> Add a common helper, amd_pmf_if_verify_buffer(), that validates the
> buffer (type, NULL or short pointer, header and output size) and returns
> the size on success. Call it from both apts_if_call_store_buffer() and
> apmf_if_call_store_buffer().
> 
> Fixes: 3eecb434d7f2 ("platform/x86/amd/pmf: Add support to get sps default APTS index values")
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>

Thanks for following my suggestions.

Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>

> ---
> v3:
>   - Move full buffer validation (type, NULL or short pointer, header and
>     output size checks) into verify buffer helper.
> 
> v2:
>   - Add a helper function for NULL pointer and buffer validation. Reuse it
>     in apmf_if_call_store_buffer() and apts_if_call_store_buffer().
> 
>   drivers/platform/x86/amd/pmf/acpi.c | 70 +++++++++++++----------------
>   1 file changed, 30 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
> index 3d94b03cf794..59173bcc3750 100644
> --- a/drivers/platform/x86/amd/pmf/acpi.c
> +++ b/drivers/platform/x86/amd/pmf/acpi.c
> @@ -50,47 +50,54 @@ static union acpi_object *apmf_if_call(struct amd_pmf_dev *pdev, int fn, struct
>   	return buffer.pointer;
>   }
>   
> -static int apmf_if_call_store_buffer(struct amd_pmf_dev *pdev, int fn, void *dest, size_t out_sz)
> +static int amd_pmf_if_verify_buffer(struct amd_pmf_dev *pdev,
> +				    union acpi_object *info, size_t out_sz)
>   {
> -	union acpi_object *info;
>   	size_t size;
> -	int err = 0;
> -
> -	info = apmf_if_call(pdev, fn, NULL);
> -	if (!info)
> -		return -EIO;
>   
>   	if (info->type != ACPI_TYPE_BUFFER) {
>   		dev_err(pdev->dev, "object is not a buffer\n");
> -		err = -EINVAL;
> -		goto out;
> +		return -EINVAL;
>   	}
>   
> -	if (info->buffer.length < 2) {
> -		dev_err(pdev->dev, "buffer too small\n");
> -		err = -EINVAL;
> -		goto out;
> +	if (!info->buffer.pointer || info->buffer.length < 2) {
> +		dev_err(pdev->dev, "buffer pointer is NULL or too small\n");
> +		return -EINVAL;
>   	}
>   
>   	size = *(u16 *)info->buffer.pointer;
>   	if (info->buffer.length < size) {
> -		dev_err(pdev->dev, "buffer smaller then headersize %u < %zu\n",
> +		dev_err(pdev->dev, "buffer smaller than headersize %u < %zu\n",
>   			info->buffer.length, size);
> -		err = -EINVAL;
> -		goto out;
> +		return -EINVAL;
>   	}
>   
>   	if (size < out_sz) {
>   		dev_err(pdev->dev, "buffer too small %zu\n", size);
> -		err = -EINVAL;
> -		goto out;
> +		return -EINVAL;
>   	}
>   
> +	return (int)size;
> +}
> +
> +static int apmf_if_call_store_buffer(struct amd_pmf_dev *pdev, int fn, void *dest, size_t out_sz)
> +{
> +	union acpi_object *info;
> +	int size;
> +
> +	info = apmf_if_call(pdev, fn, NULL);
> +	if (!info)
> +		return -EIO;
> +
> +	size = amd_pmf_if_verify_buffer(pdev, info, out_sz);
> +	if (size < 0)
> +		goto out;
> +
>   	memcpy(dest, info->buffer.pointer, out_sz);
>   
>   out:
>   	kfree(info);
> -	return err;
> +	return size < 0 ? size : 0;
>   }
>   
>   static union acpi_object *apts_if_call(struct amd_pmf_dev *pdev, u32 state_index)
> @@ -125,37 +132,20 @@ static int apts_if_call_store_buffer(struct amd_pmf_dev *pdev,
>   				     u32 index, void *data, size_t out_sz)
>   {
>   	union acpi_object *info;
> -	size_t size;
> -	int err = 0;
> +	int size;
>   
>   	info = apts_if_call(pdev, index);
>   	if (!info)
>   		return -EIO;
>   
> -	if (info->type != ACPI_TYPE_BUFFER) {
> -		dev_err(pdev->dev, "object is not a buffer\n");
> -		err = -EINVAL;
> -		goto out;
> -	}
> -
> -	size = *(u16 *)info->buffer.pointer;
> -	if (info->buffer.length < size) {
> -		dev_err(pdev->dev, "buffer smaller than header size %u < %zu\n",
> -			info->buffer.length, size);
> -		err = -EINVAL;
> -		goto out;
> -	}
> -
> -	if (size < out_sz) {
> -		dev_err(pdev->dev, "buffer too small %zu\n", size);
> -		err = -EINVAL;
> +	size = amd_pmf_if_verify_buffer(pdev, info, out_sz);
> +	if (size < 0)
>   		goto out;
> -	}
>   
>   	memcpy(data, info->buffer.pointer, out_sz);
>   out:
>   	kfree(info);
> -	return err;
> +	return size < 0 ? size : 0;
>   }
>   
>   int is_apmf_func_supported(struct amd_pmf_dev *pdev, unsigned long index)