| Message ID | 20230821073727.225341-1-suhui@nfschina.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/amdgpu: Avoid possible buffer overflow | expand |
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. 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 ||
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 || >
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; } if (le16_to_cpu(ip->hw_id) == SDMA0_HWID || le16_to_cpu(ip->hw_id) == SDMA1_HWID ||
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(-)