* VCN_INFO_TABLE_MAX_NUM_INSTANCES vs AMDGPU_MAX_VCN_INSTANCES
@ 2022-05-15 15:46 Ernst Sjöstrand
2022-05-16 15:13 ` Alex Deucher
0 siblings, 1 reply; 7+ messages in thread
From: Ernst Sjöstrand @ 2022-05-15 15:46 UTC (permalink / raw)
To: amd-gfx mailing list
[-- Attachment #1: Type: text/plain, Size: 437 bytes --]
smatch found this problem on amd-staging-drm-next:
drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c:1443
amdgpu_discovery_get_vcn_info() error: buffer overflow
'adev->vcn.vcn_codec_disable_mask' 2 <= 3
This is caused by:
#define AMDGPU_MAX_VCN_INSTANCES 2
#define VCN_INFO_TABLE_MAX_NUM_INSTANCES 4
Can we just drop VCN_INFO_TABLE_MAX_NUM_INSTANCES completely and use
AMDGPU_MAX_VCN_INSTANCES everywhere instead (and bump it to 4)?
//E
[-- Attachment #2: Type: text/html, Size: 1401 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: VCN_INFO_TABLE_MAX_NUM_INSTANCES vs AMDGPU_MAX_VCN_INSTANCES
2022-05-15 15:46 VCN_INFO_TABLE_MAX_NUM_INSTANCES vs AMDGPU_MAX_VCN_INSTANCES Ernst Sjöstrand
@ 2022-05-16 15:13 ` Alex Deucher
2022-05-16 17:49 ` Ernst Sjöstrand
0 siblings, 1 reply; 7+ messages in thread
From: Alex Deucher @ 2022-05-16 15:13 UTC (permalink / raw)
To: Ernst Sjöstrand; +Cc: amd-gfx mailing list
On Sun, May 15, 2022 at 11:46 AM Ernst Sjöstrand <ernstp@gmail.com> wrote:
>
> smatch found this problem on amd-staging-drm-next:
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c:1443 amdgpu_discovery_get_vcn_info() error: buffer overflow 'adev->vcn.vcn_codec_disable_mask' 2 <= 3
>
> This is caused by:
> #define AMDGPU_MAX_VCN_INSTANCES 2
> #define VCN_INFO_TABLE_MAX_NUM_INSTANCES 4
>
> Can we just drop VCN_INFO_TABLE_MAX_NUM_INSTANCES completely and use AMDGPU_MAX_VCN_INSTANCES everywhere instead (and bump it to 4)?
We should be able to bump AMDGPU_MAX_VCN_INSTANCES to 4 (although it
would waste some memory in the places it is used at this point).
VCN_INFO_TABLE_MAX_NUM_INSTANCES is part of a firmware structure so we
can't change that without breaking the firmware structure.
Alex
>
> //E
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: VCN_INFO_TABLE_MAX_NUM_INSTANCES vs AMDGPU_MAX_VCN_INSTANCES
2022-05-16 15:13 ` Alex Deucher
@ 2022-05-16 17:49 ` Ernst Sjöstrand
2022-05-16 18:10 ` Christian König
0 siblings, 1 reply; 7+ messages in thread
From: Ernst Sjöstrand @ 2022-05-16 17:49 UTC (permalink / raw)
To: Alex Deucher; +Cc: amd-gfx mailing list
[-- Attachment #1: Type: text/plain, Size: 1121 bytes --]
Den mån 16 maj 2022 kl 17:13 skrev Alex Deucher <alexdeucher@gmail.com>:
> On Sun, May 15, 2022 at 11:46 AM Ernst Sjöstrand <ernstp@gmail.com> wrote:
> >
> > smatch found this problem on amd-staging-drm-next:
> >
> > drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c:1443
> amdgpu_discovery_get_vcn_info() error: buffer overflow
> 'adev->vcn.vcn_codec_disable_mask' 2 <= 3
> >
> > This is caused by:
> > #define AMDGPU_MAX_VCN_INSTANCES 2
> > #define VCN_INFO_TABLE_MAX_NUM_INSTANCES 4
> >
> > Can we just drop VCN_INFO_TABLE_MAX_NUM_INSTANCES completely and use
> AMDGPU_MAX_VCN_INSTANCES everywhere instead (and bump it to 4)?
>
> We should be able to bump AMDGPU_MAX_VCN_INSTANCES to 4 (although it
> would waste some memory in the places it is used at this point).
> VCN_INFO_TABLE_MAX_NUM_INSTANCES is part of a firmware structure so we
> can't change that without breaking the firmware structure.
>
> Alex
>
It would be nice to get rid of this pattern and make sure it doesn't happen
again when the VCN info table is raised to 5.
It's very similar to the HWIP_MAX_INSTANCE issue.
//E
[-- Attachment #2: Type: text/html, Size: 1970 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: VCN_INFO_TABLE_MAX_NUM_INSTANCES vs AMDGPU_MAX_VCN_INSTANCES
2022-05-16 17:49 ` Ernst Sjöstrand
@ 2022-05-16 18:10 ` Christian König
2022-05-16 18:15 ` Alex Deucher
0 siblings, 1 reply; 7+ messages in thread
From: Christian König @ 2022-05-16 18:10 UTC (permalink / raw)
To: Ernst Sjöstrand, Alex Deucher; +Cc: amd-gfx mailing list
[-- Attachment #1: Type: text/plain, Size: 1499 bytes --]
Am 16.05.22 um 19:49 schrieb Ernst Sjöstrand:
> Den mån 16 maj 2022 kl 17:13 skrev Alex Deucher <alexdeucher@gmail.com>:
>
> On Sun, May 15, 2022 at 11:46 AM Ernst Sjöstrand
> <ernstp@gmail.com> wrote:
> >
> > smatch found this problem on amd-staging-drm-next:
> >
> > drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c:1443
> amdgpu_discovery_get_vcn_info() error: buffer overflow
> 'adev->vcn.vcn_codec_disable_mask' 2 <= 3
> >
> > This is caused by:
> > #define AMDGPU_MAX_VCN_INSTANCES 2
> > #define VCN_INFO_TABLE_MAX_NUM_INSTANCES 4
> >
> > Can we just drop VCN_INFO_TABLE_MAX_NUM_INSTANCES completely and
> use AMDGPU_MAX_VCN_INSTANCES everywhere instead (and bump it to 4)?
>
> We should be able to bump AMDGPU_MAX_VCN_INSTANCES to 4 (although it
> would waste some memory in the places it is used at this point).
> VCN_INFO_TABLE_MAX_NUM_INSTANCES is part of a firmware structure so we
> can't change that without breaking the firmware structure.
>
> Alex
>
>
> It would be nice to get rid of this pattern and make sure it doesn't
> happen again when the VCN info table is raised to 5.
> It's very similar to the HWIP_MAX_INSTANCE issue.
No, as Alex explained that distinction is intentional.
The firmware definition is 4 for future extensions, that doesn't mean
that this is currently used.
There is currently simply no need to set AMDGPU_MAX_VCN_INSTANCES to
more than 2.
Regards,
Christian.
>
> //E
>
[-- Attachment #2: Type: text/html, Size: 3566 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: VCN_INFO_TABLE_MAX_NUM_INSTANCES vs AMDGPU_MAX_VCN_INSTANCES
2022-05-16 18:10 ` Christian König
@ 2022-05-16 18:15 ` Alex Deucher
2022-05-16 18:18 ` Ernst Sjöstrand
2022-05-16 19:27 ` Christian König
0 siblings, 2 replies; 7+ messages in thread
From: Alex Deucher @ 2022-05-16 18:15 UTC (permalink / raw)
To: Christian König; +Cc: Ernst Sjöstrand, amd-gfx mailing list
[-- Attachment #1: Type: text/plain, Size: 1683 bytes --]
On Mon, May 16, 2022 at 2:10 PM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Am 16.05.22 um 19:49 schrieb Ernst Sjöstrand:
>
> Den mån 16 maj 2022 kl 17:13 skrev Alex Deucher <alexdeucher@gmail.com>:
>>
>> On Sun, May 15, 2022 at 11:46 AM Ernst Sjöstrand <ernstp@gmail.com> wrote:
>> >
>> > smatch found this problem on amd-staging-drm-next:
>> >
>> > drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c:1443 amdgpu_discovery_get_vcn_info() error: buffer overflow 'adev->vcn.vcn_codec_disable_mask' 2 <= 3
>> >
>> > This is caused by:
>> > #define AMDGPU_MAX_VCN_INSTANCES 2
>> > #define VCN_INFO_TABLE_MAX_NUM_INSTANCES 4
>> >
>> > Can we just drop VCN_INFO_TABLE_MAX_NUM_INSTANCES completely and use AMDGPU_MAX_VCN_INSTANCES everywhere instead (and bump it to 4)?
>>
>> We should be able to bump AMDGPU_MAX_VCN_INSTANCES to 4 (although it
>> would waste some memory in the places it is used at this point).
>> VCN_INFO_TABLE_MAX_NUM_INSTANCES is part of a firmware structure so we
>> can't change that without breaking the firmware structure.
>>
>> Alex
>
>
> It would be nice to get rid of this pattern and make sure it doesn't happen again when the VCN info table is raised to 5.
> It's very similar to the HWIP_MAX_INSTANCE issue.
>
>
> No, as Alex explained that distinction is intentional.
>
> The firmware definition is 4 for future extensions, that doesn't mean that this is currently used.
>
> There is currently simply no need to set AMDGPU_MAX_VCN_INSTANCES to more than 2.
Right. The attached patch should protect against the scenario you are
envisioning.
Alex
>
> Regards,
> Christian.
>
>
> //E
>
>
[-- Attachment #2: 0001-drm-amdgpu-discovery-validate-VCN-and-SDMA-instances.patch --]
[-- Type: text/x-patch, Size: 1919 bytes --]
From 6fecd3c86fbb7b89d59d16cffea438e5b2a5915c Mon Sep 17 00:00:00 2001
From: Alex Deucher <alexander.deucher@amd.com>
Date: Mon, 16 May 2022 14:12:33 -0400
Subject: [PATCH] drm/amdgpu/discovery: validate VCN and SDMA instances
Validate the VCN and SDMA instances against the driver
structure sizes to make sure we don't get into a
situation where the firmware reports more instances than
the driver supports.
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
index 1f4e07a32445..2c4f1adb5343 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
@@ -1137,13 +1137,24 @@ static int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev)
adev->vcn.vcn_config[adev->vcn.num_vcn_inst] =
ip->revision & 0xc0;
ip->revision &= ~0xc0;
- adev->vcn.num_vcn_inst++;
+ if (adev->vcn.num_vcn_inst < AMDGPU_MAX_VCN_INSTANCES)
+ adev->vcn.num_vcn_inst++;
+ else
+ dev_err(adev->dev, "Too many VCN instances: %d vs %d\n",
+ adev->vcn.num_vcn_inst + 1,
+ AMDGPU_MAX_VCN_INSTANCES);
}
if (le16_to_cpu(ip->hw_id) == SDMA0_HWID ||
le16_to_cpu(ip->hw_id) == SDMA1_HWID ||
le16_to_cpu(ip->hw_id) == SDMA2_HWID ||
- le16_to_cpu(ip->hw_id) == SDMA3_HWID)
- adev->sdma.num_instances++;
+ le16_to_cpu(ip->hw_id) == SDMA3_HWID) {
+ if (adev->sdma.num_instances < AMDGPU_MAX_SDMA_INSTANCES)
+ adev->sdma.num_instances++;
+ else
+ dev_err(adev->dev, "Too many SDMA instances: %d vs %d\n",
+ adev->sdma.num_instances + 1,
+ AMDGPU_MAX_SDMA_INSTANCES);
+ }
if (le16_to_cpu(ip->hw_id) == UMC_HWID)
adev->gmc.num_umc++;
--
2.35.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: VCN_INFO_TABLE_MAX_NUM_INSTANCES vs AMDGPU_MAX_VCN_INSTANCES
2022-05-16 18:15 ` Alex Deucher
@ 2022-05-16 18:18 ` Ernst Sjöstrand
2022-05-16 19:27 ` Christian König
1 sibling, 0 replies; 7+ messages in thread
From: Ernst Sjöstrand @ 2022-05-16 18:18 UTC (permalink / raw)
To: Alex Deucher; +Cc: Christian König, amd-gfx mailing list
[-- Attachment #1: Type: text/plain, Size: 1902 bytes --]
Looks good to me!
Den mån 16 maj 2022 kl 20:15 skrev Alex Deucher <alexdeucher@gmail.com>:
> On Mon, May 16, 2022 at 2:10 PM Christian König
> <ckoenig.leichtzumerken@gmail.com> wrote:
> >
> > Am 16.05.22 um 19:49 schrieb Ernst Sjöstrand:
> >
> > Den mån 16 maj 2022 kl 17:13 skrev Alex Deucher <alexdeucher@gmail.com>:
> >>
> >> On Sun, May 15, 2022 at 11:46 AM Ernst Sjöstrand <ernstp@gmail.com>
> wrote:
> >> >
> >> > smatch found this problem on amd-staging-drm-next:
> >> >
> >> > drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c:1443
> amdgpu_discovery_get_vcn_info() error: buffer overflow
> 'adev->vcn.vcn_codec_disable_mask' 2 <= 3
> >> >
> >> > This is caused by:
> >> > #define AMDGPU_MAX_VCN_INSTANCES 2
> >> > #define VCN_INFO_TABLE_MAX_NUM_INSTANCES 4
> >> >
> >> > Can we just drop VCN_INFO_TABLE_MAX_NUM_INSTANCES completely and use
> AMDGPU_MAX_VCN_INSTANCES everywhere instead (and bump it to 4)?
> >>
> >> We should be able to bump AMDGPU_MAX_VCN_INSTANCES to 4 (although it
> >> would waste some memory in the places it is used at this point).
> >> VCN_INFO_TABLE_MAX_NUM_INSTANCES is part of a firmware structure so we
> >> can't change that without breaking the firmware structure.
> >>
> >> Alex
> >
> >
> > It would be nice to get rid of this pattern and make sure it doesn't
> happen again when the VCN info table is raised to 5.
> > It's very similar to the HWIP_MAX_INSTANCE issue.
> >
> >
> > No, as Alex explained that distinction is intentional.
> >
> > The firmware definition is 4 for future extensions, that doesn't mean
> that this is currently used.
> >
> > There is currently simply no need to set AMDGPU_MAX_VCN_INSTANCES to
> more than 2.
>
> Right. The attached patch should protect against the scenario you are
> envisioning.
>
> Alex
>
> >
> > Regards,
> > Christian.
> >
> >
> > //E
> >
> >
>
[-- Attachment #2: Type: text/html, Size: 2787 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: VCN_INFO_TABLE_MAX_NUM_INSTANCES vs AMDGPU_MAX_VCN_INSTANCES
2022-05-16 18:15 ` Alex Deucher
2022-05-16 18:18 ` Ernst Sjöstrand
@ 2022-05-16 19:27 ` Christian König
1 sibling, 0 replies; 7+ messages in thread
From: Christian König @ 2022-05-16 19:27 UTC (permalink / raw)
To: Alex Deucher; +Cc: Ernst Sjöstrand, amd-gfx mailing list
Am 16.05.22 um 20:15 schrieb Alex Deucher:
> On Mon, May 16, 2022 at 2:10 PM Christian König
> <ckoenig.leichtzumerken@gmail.com> wrote:
>> Am 16.05.22 um 19:49 schrieb Ernst Sjöstrand:
>>
>> Den mån 16 maj 2022 kl 17:13 skrev Alex Deucher <alexdeucher@gmail.com>:
>>> On Sun, May 15, 2022 at 11:46 AM Ernst Sjöstrand <ernstp@gmail.com> wrote:
>>>> smatch found this problem on amd-staging-drm-next:
>>>>
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c:1443 amdgpu_discovery_get_vcn_info() error: buffer overflow 'adev->vcn.vcn_codec_disable_mask' 2 <= 3
>>>>
>>>> This is caused by:
>>>> #define AMDGPU_MAX_VCN_INSTANCES 2
>>>> #define VCN_INFO_TABLE_MAX_NUM_INSTANCES 4
>>>>
>>>> Can we just drop VCN_INFO_TABLE_MAX_NUM_INSTANCES completely and use AMDGPU_MAX_VCN_INSTANCES everywhere instead (and bump it to 4)?
>>> We should be able to bump AMDGPU_MAX_VCN_INSTANCES to 4 (although it
>>> would waste some memory in the places it is used at this point).
>>> VCN_INFO_TABLE_MAX_NUM_INSTANCES is part of a firmware structure so we
>>> can't change that without breaking the firmware structure.
>>>
>>> Alex
>>
>> It would be nice to get rid of this pattern and make sure it doesn't happen again when the VCN info table is raised to 5.
>> It's very similar to the HWIP_MAX_INSTANCE issue.
>>
>>
>> No, as Alex explained that distinction is intentional.
>>
>> The firmware definition is 4 for future extensions, that doesn't mean that this is currently used.
>>
>> There is currently simply no need to set AMDGPU_MAX_VCN_INSTANCES to more than 2.
> Right. The attached patch should protect against the scenario you are
> envisioning.
Acked-by: Christian König <christian.koenig@amd.com>
>
> Alex
>
>> Regards,
>> Christian.
>>
>>
>> //E
>>
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-05-16 19:27 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-15 15:46 VCN_INFO_TABLE_MAX_NUM_INSTANCES vs AMDGPU_MAX_VCN_INSTANCES Ernst Sjöstrand
2022-05-16 15:13 ` Alex Deucher
2022-05-16 17:49 ` Ernst Sjöstrand
2022-05-16 18:10 ` Christian König
2022-05-16 18:15 ` Alex Deucher
2022-05-16 18:18 ` Ernst Sjöstrand
2022-05-16 19:27 ` Christian König
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).