Re: [PATCH] drm/amdgpu: zero reg_state buffer before filling it

["Christian König" <christian.koenig@amd.com>]

On 9/14/26 17:25, Lazar, Lijo wrote:
> On 14-Sep-26 8:24 PM, Christian König wrote:
>> On 9/14/26 16:45, Lazar, Lijo wrote:
>>>
>>>
>>> On 14-Sep-26 7:30 PM, Christian König wrote:
>>>> On 9/14/26 15:56, Dmitriy Chumachenko wrote:
>>>>> The reg_state readouts return szbuf bytes but never write the pad members
>>>>> of the headers and of amdgpu_smn_reg_data (and, for pcie, the fields
>>>>> skipped when the upstream port or AER capability is absent). The sysfs
>>>>> buffer is a plain kmalloc(), so those bytes leak stale slab data to any
>>>>> local user through the 0444 reg_state attribute.
>>>>>
>>>>> Zero the buffer once the size is known.
>>>>>
>>>>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>>>>
>>>>> Fixes: 081a6eda2b25 ("drm/amdgpu: Read aquavanjaram PCIE register state")
>>>>> Signed-off-by: Dmitriy Chumachenko <Dmitry.Chumachenko@cyberprotect.ru>
>>>>
>>>> Good catch, but stuff like that should be handled by using kzalloc() instead of calling memset it manually.
>>>>
>>>
>>> It's the default sysfs buffer allocation which may not be used entirely by all drivers.
>>>
>>> The concern in this case is that driver is telling it has written x bytes whereas it could have left some fields unfilled in the buffer. I think the fix is fine.
>>
>> Wait a second, is that because of messed up padding in sysfs structures? That is usually a really big NO-GO for upstreaming.
>>
>> Structures returned to userspace must be padding free, see the tool pahole for how to double check that on different architectures.
>>
> 
> It's an explicit padding like below for alignment. Those fields also could go unfilled.
> 
> struct amdgpu_reg_state_header {
>         uint16_t                structure_size;
>         uint8_t                 format_revision;
>         uint8_t                 content_revision;
>         uint8_t                 state_type;
>         uint8_t                 num_instances;
>         uint16_t                pad;
> };

Ok, got it. Yeah that approach is perfectly fine.

But we should still us kzalloc() or at least do the memset at the IOCTL level and not in the backend.

Regards,
Christian.

> 
> Thanks,
> Lijo
> 
>> Regards,
>> Christian.
>>
>>>
>>> Thanks,
>>> Lijo
>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> ---
>>>>>    drivers/gpu/drm/amd/amdgpu/aqua_vanjaram.c | 8 ++++++++
>>>>>    1 file changed, 8 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/aqua_vanjaram.c b/drivers/gpu/drm/amd/amdgpu/aqua_vanjaram.c
>>>>> index 72ea37dbfea8..0086de0f685d 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/aqua_vanjaram.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/aqua_vanjaram.c
>>>>> @@ -610,6 +610,8 @@ static ssize_t aqua_vanjaram_read_pcie_state(struct amdgpu_device *adev,
>>>>>           if (max_size < szbuf)
>>>>>                   return -EOVERFLOW;
>>>>>
>>>>> +       memset(buf, 0, szbuf);
>>>>> +
>>>>>           pcie_regs = (struct amdgpu_regs_pcie_v1_0 *)((uint8_t *)buf +
>>>>>                                                        sizeof(*pcie_reg_state));
>>>>>           pcie_regs->inst_header.instance = 0;
>>>>> @@ -702,6 +704,8 @@ static ssize_t aqua_vanjaram_read_xgmi_state(struct amdgpu_device *adev,
>>>>>           if (max_size < szbuf)
>>>>>                   return -EOVERFLOW;
>>>>>
>>>>> +       memset(buf, 0, szbuf);
>>>>> +
>>>>>           p = &xgmi_reg_state->xgmi_state_regs[0];
>>>>>           for_each_inst(i, adev->aid_mask) {
>>>>>                   for (j = 0; j < xgmi_inst; ++j) {
>>>>> @@ -776,6 +780,8 @@ static ssize_t aqua_vanjaram_read_wafl_state(struct amdgpu_device *adev,
>>>>>           if (max_size < szbuf)
>>>>>                   return -EOVERFLOW;
>>>>>
>>>>> +       memset(buf, 0, szbuf);
>>>>> +
>>>>>           p = &wafl_reg_state->wafl_state_regs[0];
>>>>>           for_each_inst(i, adev->aid_mask) {
>>>>>                   for (j = 0; j < wafl_inst; ++j) {
>>>>> @@ -902,6 +908,8 @@ static ssize_t aqua_vanjaram_read_usr_state(struct amdgpu_device *adev,
>>>>>           if (max_size < szbuf)
>>>>>                   return -EOVERFLOW;
>>>>>
>>>>> +       memset(buf, 0, szbuf);
>>>>> +
>>>>>           p = &usr_reg_state->usr_state_regs[0];
>>>>>           for_each_inst(i, adev->aid_mask) {
>>>>>                   usr_regs = (struct amdgpu_regs_usr_v1_0 *)p;
>>>>> -- 
>>>>> 2.49.0
>>>>>
>>>>
>>>
>>
>