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

[Lazar, Lijo]

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@xxxxxxxxxxxxxxx>
>
> 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.

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