Re: [PATCH v4 04/11] drm/amdgpu: Add user save area params validation

["Lazar, Lijo" <lijo.lazar@amd.com>]

On 27-Jan-26 11:25 AM, Alex Deucher wrote:
> On Tue, Jan 27, 2026 at 12:35 AM Lazar, Lijo <lijo.lazar@amd.com> wrote:
>>
>>
>>
>> On 24-Jan-26 2:14 AM, Alex Deucher wrote:
>>> On Thu, Jan 22, 2026 at 5:52 AM Lijo Lazar <lijo.lazar@amd.com> wrote:
>>>>
>>>> Add an interface to validate user provided save area parameters. Address
>>>> validation is not done and expected to be done outside.
>>>>
>>>> Signed-off-by: Lijo Lazar <lijo.lazar@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_cwsr.c | 44 ++++++++++++++++++++++++
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_cwsr.h | 11 ++++++
>>>>    2 files changed, 55 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cwsr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cwsr.c
>>>> index 80020fd33ce6..32d9398cd1d1 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cwsr.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cwsr.c
>>>> @@ -64,6 +64,15 @@ static inline bool amdgpu_cwsr_is_supported(struct amdgpu_device *adev)
>>>>           return true;
>>>>    }
>>>>
>>>> +uint32_t amdgpu_cwsr_size_needed(struct amdgpu_device *adev, int num_xcc)
>>>> +{
>>>> +       if (!amdgpu_cwsr_is_enabled(adev))
>>>> +               return 0;
>>>> +
>>>> +       return num_xcc *
>>>> +              (adev->cwsr_info->xcc_cwsr_sz + adev->cwsr_info->xcc_dbg_mem_sz);
>>>
>>> These could overflow if userspace passes in especially large values.
>>>
>>
>> Sorry, I didn't get that. cwsr_info contains driver calculated values.
>> This function returns the size required.
> 
> Sorry, I mixed this up.  See below.
> 
>>
>> Thanks,
>> Lijo
>>
>>> Alex
>>>
>>>> +}
>>>> +
>>>>    static void amdgpu_cwsr_init_isa_details(struct amdgpu_device *adev,
>>>>                                            struct amdgpu_cwsr_info *cwsr_info)
>>>>    {
>>>> @@ -425,6 +434,41 @@ int amdgpu_cwsr_alloc(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>           return r;
>>>>    }
>>>>
>>>> +int amdgpu_cwsr_validate_params(struct amdgpu_device *adev,
>>>> +                               struct amdgpu_cwsr_params *cwsr_params,
>>>> +                               int num_xcc)
>>>> +{
>>>> +       struct amdgpu_cwsr_info *cwsr_info = adev->cwsr_info;
>>>> +
>>>> +       if (!amdgpu_cwsr_is_enabled(adev))
>>>> +               return -EOPNOTSUPP;
>>>> +
>>>> +       if (!cwsr_params)
>>>> +               return -EINVAL;
>>>> +
>>>> +       /*
>>>> +        * Only control stack and save area size details checked. Address validation needs to be
>>>> +        * carried out separately.
>>>> +        */
>>>> +       if (cwsr_params->ctl_stack_sz !=
>>>> +           (cwsr_info->xcc_ctl_stack_sz * num_xcc)) {
>>>> +               dev_dbg(adev->dev,
>>>> +                       "queue ctl stack size 0x%x not equal to node ctl stack size 0x%x\n",
>>>> +                       cwsr_params->ctl_stack_sz,
>>>> +                       num_xcc * cwsr_info->xcc_ctl_stack_sz);
>>>> +               return -EINVAL;
>>>> +       }
>>>> +
>>>> +       if (cwsr_params->cwsr_sz < (cwsr_info->xcc_cwsr_sz * num_xcc)) {
>>>> +               dev_dbg(adev->dev,
>>>> +                       "queue cwsr size 0x%x not equal to node cwsr size 0x%x\n",
>>>> +                       cwsr_params->cwsr_sz, num_xcc * cwsr_info->xcc_cwsr_sz);
>>>> +               return -EINVAL;
>>>> +       }
> 
> cwsr_params->cwsr_sz has no upper bound check.  Can this cause an
> overflow elsewhere?
> 

We could restrict to a max limit of 2 * cwsr size required. Adding 
David/Lancelot as well.

Thanks,
Lijo

> Alex
> 
> 
> Alex
> 
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>>    void amdgpu_cwsr_free(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>                         struct amdgpu_cwsr_trap_obj **trap_obj)
>>>>    {
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cwsr.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_cwsr.h
>>>> index 3c80d057bbed..96b03a8ed99b 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cwsr.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cwsr.h
>>>> @@ -56,6 +56,13 @@ struct amdgpu_cwsr_info {
>>>>           uint32_t xcc_cwsr_sz;
>>>>    };
>>>>
>>>> +struct amdgpu_cwsr_params {
>>>> +       uint64_t ctx_save_area_address;
>>>> +       /* cwsr size info */
>>>> +       uint32_t ctl_stack_sz;
>>>> +       uint32_t cwsr_sz;
>>>> +};
>>>> +
>>>>    int amdgpu_cwsr_init(struct amdgpu_device *adev);
>>>>    void amdgpu_cwsr_fini(struct amdgpu_device *adev);
>>>>
>>>> @@ -68,4 +75,8 @@ static inline bool amdgpu_cwsr_is_enabled(struct amdgpu_device *adev)
>>>>           return adev->cwsr_info != NULL;
>>>>    }
>>>>
>>>> +uint32_t amdgpu_cwsr_size_needed(struct amdgpu_device *adev, int num_xcc);
>>>> +int amdgpu_cwsr_validate_params(struct amdgpu_device *adev,
>>>> +                               struct amdgpu_cwsr_params *cwsr_params,
>>>> +                               int num_xcc);
>>>>    #endif
>>>> --
>>>> 2.49.0
>>>>
>>