* [PATCH v2] drm/amdgpu: Avoid possible buffer overflow
@ 2023-08-21 7:37 Su Hui
2023-08-21 9:31 ` Christian König
0 siblings, 1 reply; 3+ messages in thread
From: Su Hui @ 2023-08-21 7:37 UTC (permalink / raw)
To: alexander.deucher, christian.koenig, Xinhui.Pan, airlied, daniel
Cc: yifan1.zhang, Su Hui, guchun.chen, lijo.lazar, kernel-janitors,
linux-kernel, amd-gfx, le.ma, christophe.jaillet, dri-devel,
Yuliang.Shi, candice.li, Hawking.Zhang
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;
}
if (le16_to_cpu(ip->hw_id) == SDMA0_HWID ||
le16_to_cpu(ip->hw_id) == SDMA1_HWID ||
--
2.30.2
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v2] drm/amdgpu: Avoid possible buffer overflow
2023-08-21 7:37 [PATCH v2] drm/amdgpu: Avoid possible buffer overflow Su Hui
@ 2023-08-21 9:31 ` Christian König
2023-08-22 1:39 ` Su Hui
0 siblings, 1 reply; 3+ messages in thread
From: Christian König @ 2023-08-21 9:31 UTC (permalink / raw)
To: Su Hui, alexander.deucher, Xinhui.Pan, airlied, daniel
Cc: yifan1.zhang, guchun.chen, lijo.lazar, kernel-janitors,
linux-kernel, amd-gfx, le.ma, christophe.jaillet, dri-devel,
Yuliang.Shi, candice.li, Hawking.Zhang
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 ||
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v2] drm/amdgpu: Avoid possible buffer overflow
2023-08-21 9:31 ` Christian König
@ 2023-08-22 1:39 ` Su Hui
0 siblings, 0 replies; 3+ messages in thread
From: Su Hui @ 2023-08-22 1:39 UTC (permalink / raw)
To: Christian König, alexander.deucher, Xinhui.Pan, airlied,
daniel
Cc: yifan1.zhang, guchun.chen, lijo.lazar, kernel-janitors,
linux-kernel, amd-gfx, le.ma, christophe.jaillet, dri-devel,
Yuliang.Shi, candice.li, Hawking.Zhang
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 ||
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-08-22 13:09 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-21 7:37 [PATCH v2] drm/amdgpu: Avoid possible buffer overflow Su Hui
2023-08-21 9:31 ` Christian König
2023-08-22 1:39 ` Su Hui
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox