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

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

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;
};

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