Messages in this thread | | | Date | Tue, 22 Aug 2023 09:39:22 +0800 | Subject | Re: [PATCH v2] drm/amdgpu: Avoid possible buffer overflow | From | Su Hui <> |
| |
On 2023/8/21 17:31, Christian König wrote: > Am 21.08.23 um 09:37 schrieb Su Hui: >> smatch error: >> drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c:1257 >> amdgpu_discovery_reg_base_init() error: >> testing array offset 'adev->vcn.num_vcn_inst' after use. >> >> change the assignment order to avoid buffer overflow. >> >> Fixes: c40bdfb2ffa4 ("drm/amdgpu: fix incorrect VCN revision in SRIOV") >> Signed-off-by: Su Hui <suhui@nfschina.com> >> --- >> changes in v2: >> - fix the error about ip->revision (thanks to Christophe JAILLET). >> drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c >> index 8e1cfc87122d..b07bfd106a9b 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c >> @@ -1250,11 +1250,10 @@ static int >> amdgpu_discovery_reg_base_init(struct amdgpu_device *adev) >> * 0b10 : encode is disabled >> * 0b01 : decode is disabled >> */ >> - adev->vcn.vcn_config[adev->vcn.num_vcn_inst] = >> - ip->revision & 0xc0; >> - ip->revision &= ~0xc0; >> if (adev->vcn.num_vcn_inst < >> AMDGPU_MAX_VCN_INSTANCES) { >> + adev->vcn.vcn_config[adev->vcn.num_vcn_inst] = >> + ip->revision & 0xc0; >> adev->vcn.num_vcn_inst++; >> adev->vcn.inst_mask |= >> (1U << ip->instance_number); >> @@ -1265,6 +1264,7 @@ static int >> amdgpu_discovery_reg_base_init(struct amdgpu_device *adev) >> adev->vcn.num_vcn_inst + 1, >> AMDGPU_MAX_VCN_INSTANCES); >> } >> + ip->revision &= ~0xc0; > > That doesn't looks correct either. The assignment is intentionally > outside of the "if". > > See "adev->vcn.vcn_config[adev->vcn.num_vcn_inst] = ip->revision & > 0xc0;" is always valid.
Hi,
if "adev->vcn.vcn_config[adev->vcn.num_vcn_inst] = ip->revision & 0xc0;" is always valid, then
"adev->vcn.num_vcn_inst< AMDGPU_MAX_VCN_INSTANCES " is always true. So the below judgement has
no sense.
if (adev->vcn.num_vcn_inst < AMDGPU_MAX_VCN_INSTANCES) {
On the contrary, if we need this judgement, then "adev->vcn.vcn_config[adev->vcn.num_vcn_inst] = ip->revision & 0xc0;"is not
always valid, because "adev->vcn.num_vcn_inst >= AMDGPU_MAX_VCN_INSTANCES" can be true, which cause buffer overflow.
So I think this patch has some sense if I don't make some mistakes.
Su Hui
> > We just avoid incrementing num_vcn_inst when we already have to many. > > Regards, > Christian. > > >> } >> if (le16_to_cpu(ip->hw_id) == SDMA0_HWID || >> le16_to_cpu(ip->hw_id) == SDMA1_HWID || >
| |