* [PATCH] drm/amdgpu/pptable: Fix UBSAN array-index-out-of-bounds
@ 2024-05-23 7:16 Tasos Sahanidis
2024-05-23 14:52 ` Alex Deucher
0 siblings, 1 reply; 6+ messages in thread
From: Tasos Sahanidis @ 2024-05-23 7:16 UTC (permalink / raw)
To: amd-gfx; +Cc: Tasos Sahanidis
Dyanmically sized arrays used [1] instead of []. Replacing the former
with the latter resolves multiple warnings observed on boot with a
BONAIRE card.
Signed-off-by: Tasos Sahanidis <tasos@tasossah.com>
---
drivers/gpu/drm/amd/include/pptable.h | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/amd/include/pptable.h b/drivers/gpu/drm/amd/include/pptable.h
index 2e8e6c9875f6..d1dec880d2d6 100644
--- a/drivers/gpu/drm/amd/include/pptable.h
+++ b/drivers/gpu/drm/amd/include/pptable.h
@@ -480,7 +480,7 @@ typedef struct _StateArray{
//how many states we have
UCHAR ucNumEntries;
- ATOM_PPLIB_STATE_V2 states[1];
+ ATOM_PPLIB_STATE_V2 states[];
}StateArray;
@@ -514,7 +514,7 @@ typedef struct _ATOM_PPLIB_Clock_Voltage_Dependency_Record
typedef struct _ATOM_PPLIB_Clock_Voltage_Dependency_Table
{
UCHAR ucNumEntries; // Number of entries.
- ATOM_PPLIB_Clock_Voltage_Dependency_Record entries[1]; // Dynamically allocate entries.
+ ATOM_PPLIB_Clock_Voltage_Dependency_Record entries[]; // Dynamically allocate entries.
}ATOM_PPLIB_Clock_Voltage_Dependency_Table;
typedef struct _ATOM_PPLIB_Clock_Voltage_Limit_Record
@@ -530,7 +530,7 @@ typedef struct _ATOM_PPLIB_Clock_Voltage_Limit_Record
typedef struct _ATOM_PPLIB_Clock_Voltage_Limit_Table
{
UCHAR ucNumEntries; // Number of entries.
- ATOM_PPLIB_Clock_Voltage_Limit_Record entries[1]; // Dynamically allocate entries.
+ ATOM_PPLIB_Clock_Voltage_Limit_Record entries[]; // Dynamically allocate entries.
}ATOM_PPLIB_Clock_Voltage_Limit_Table;
union _ATOM_PPLIB_CAC_Leakage_Record
@@ -554,7 +554,7 @@ typedef union _ATOM_PPLIB_CAC_Leakage_Record ATOM_PPLIB_CAC_Leakage_Record;
typedef struct _ATOM_PPLIB_CAC_Leakage_Table
{
UCHAR ucNumEntries; // Number of entries.
- ATOM_PPLIB_CAC_Leakage_Record entries[1]; // Dynamically allocate entries.
+ ATOM_PPLIB_CAC_Leakage_Record entries[]; // Dynamically allocate entries.
}ATOM_PPLIB_CAC_Leakage_Table;
typedef struct _ATOM_PPLIB_PhaseSheddingLimits_Record
@@ -569,7 +569,7 @@ typedef struct _ATOM_PPLIB_PhaseSheddingLimits_Record
typedef struct _ATOM_PPLIB_PhaseSheddingLimits_Table
{
UCHAR ucNumEntries; // Number of entries.
- ATOM_PPLIB_PhaseSheddingLimits_Record entries[1]; // Dynamically allocate entries.
+ ATOM_PPLIB_PhaseSheddingLimits_Record entries[]; // Dynamically allocate entries.
}ATOM_PPLIB_PhaseSheddingLimits_Table;
typedef struct _VCEClockInfo{
@@ -581,7 +581,7 @@ typedef struct _VCEClockInfo{
typedef struct _VCEClockInfoArray{
UCHAR ucNumEntries;
- VCEClockInfo entries[1];
+ VCEClockInfo entries[];
}VCEClockInfoArray;
typedef struct _ATOM_PPLIB_VCE_Clock_Voltage_Limit_Record
@@ -593,7 +593,7 @@ typedef struct _ATOM_PPLIB_VCE_Clock_Voltage_Limit_Record
typedef struct _ATOM_PPLIB_VCE_Clock_Voltage_Limit_Table
{
UCHAR numEntries;
- ATOM_PPLIB_VCE_Clock_Voltage_Limit_Record entries[1];
+ ATOM_PPLIB_VCE_Clock_Voltage_Limit_Record entries[];
}ATOM_PPLIB_VCE_Clock_Voltage_Limit_Table;
typedef struct _ATOM_PPLIB_VCE_State_Record
@@ -605,7 +605,7 @@ typedef struct _ATOM_PPLIB_VCE_State_Record
typedef struct _ATOM_PPLIB_VCE_State_Table
{
UCHAR numEntries;
- ATOM_PPLIB_VCE_State_Record entries[1];
+ ATOM_PPLIB_VCE_State_Record entries[];
}ATOM_PPLIB_VCE_State_Table;
@@ -627,7 +627,7 @@ typedef struct _UVDClockInfo{
typedef struct _UVDClockInfoArray{
UCHAR ucNumEntries;
- UVDClockInfo entries[1];
+ UVDClockInfo entries[];
}UVDClockInfoArray;
typedef struct _ATOM_PPLIB_UVD_Clock_Voltage_Limit_Record
@@ -639,7 +639,7 @@ typedef struct _ATOM_PPLIB_UVD_Clock_Voltage_Limit_Record
typedef struct _ATOM_PPLIB_UVD_Clock_Voltage_Limit_Table
{
UCHAR numEntries;
- ATOM_PPLIB_UVD_Clock_Voltage_Limit_Record entries[1];
+ ATOM_PPLIB_UVD_Clock_Voltage_Limit_Record entries[];
}ATOM_PPLIB_UVD_Clock_Voltage_Limit_Table;
typedef struct _ATOM_PPLIB_UVD_Table
@@ -676,7 +676,7 @@ typedef struct _ATOM_PPLIB_ACPClk_Voltage_Limit_Record
typedef struct _ATOM_PPLIB_ACPClk_Voltage_Limit_Table{
UCHAR numEntries;
- ATOM_PPLIB_ACPClk_Voltage_Limit_Record entries[1];
+ ATOM_PPLIB_ACPClk_Voltage_Limit_Record entries[];
}ATOM_PPLIB_ACPClk_Voltage_Limit_Table;
typedef struct _ATOM_PPLIB_ACP_Table
@@ -745,7 +745,7 @@ typedef struct ATOM_PPLIB_VQ_Budgeting_Record{
typedef struct ATOM_PPLIB_VQ_Budgeting_Table {
UCHAR revid;
UCHAR numEntries;
- ATOM_PPLIB_VQ_Budgeting_Record entries[1];
+ ATOM_PPLIB_VQ_Budgeting_Record entries[];
} ATOM_PPLIB_VQ_Budgeting_Table;
#pragma pack()
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] drm/amdgpu/pptable: Fix UBSAN array-index-out-of-bounds
2024-05-23 7:16 [PATCH] drm/amdgpu/pptable: Fix UBSAN array-index-out-of-bounds Tasos Sahanidis
@ 2024-05-23 14:52 ` Alex Deucher
2024-05-27 9:42 ` Tasos Sahanidis
0 siblings, 1 reply; 6+ messages in thread
From: Alex Deucher @ 2024-05-23 14:52 UTC (permalink / raw)
To: Tasos Sahanidis; +Cc: amd-gfx
On Thu, May 23, 2024 at 9:05 AM Tasos Sahanidis <tasos@tasossah.com> wrote:
>
> Dyanmically sized arrays used [1] instead of []. Replacing the former
> with the latter resolves multiple warnings observed on boot with a
> BONAIRE card.
>
> Signed-off-by: Tasos Sahanidis <tasos@tasossah.com>
> ---
> drivers/gpu/drm/amd/include/pptable.h | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/include/pptable.h b/drivers/gpu/drm/amd/include/pptable.h
> index 2e8e6c9875f6..d1dec880d2d6 100644
> --- a/drivers/gpu/drm/amd/include/pptable.h
> +++ b/drivers/gpu/drm/amd/include/pptable.h
> @@ -480,7 +480,7 @@ typedef struct _StateArray{
> //how many states we have
> UCHAR ucNumEntries;
>
> - ATOM_PPLIB_STATE_V2 states[1];
> + ATOM_PPLIB_STATE_V2 states[];
Can you add __counted_by(ucNumEntries) to the end of the line? E.g.,
ATOM_PPLIB_STATE_V2 states[] __counted_by(ucNumEntries);
Same comment for the other changes below.
Alex
> }StateArray;
>
>
> @@ -514,7 +514,7 @@ typedef struct _ATOM_PPLIB_Clock_Voltage_Dependency_Record
> typedef struct _ATOM_PPLIB_Clock_Voltage_Dependency_Table
> {
> UCHAR ucNumEntries; // Number of entries.
> - ATOM_PPLIB_Clock_Voltage_Dependency_Record entries[1]; // Dynamically allocate entries.
> + ATOM_PPLIB_Clock_Voltage_Dependency_Record entries[]; // Dynamically allocate entries.
> }ATOM_PPLIB_Clock_Voltage_Dependency_Table;>
> typedef struct _ATOM_PPLIB_Clock_Voltage_Limit_Record
> @@ -530,7 +530,7 @@ typedef struct _ATOM_PPLIB_Clock_Voltage_Limit_Record
> typedef struct _ATOM_PPLIB_Clock_Voltage_Limit_Table
> {
> UCHAR ucNumEntries; // Number of entries.
> - ATOM_PPLIB_Clock_Voltage_Limit_Record entries[1]; // Dynamically allocate entries.
> + ATOM_PPLIB_Clock_Voltage_Limit_Record entries[]; // Dynamically allocate entries.
> }ATOM_PPLIB_Clock_Voltage_Limit_Table;
>
> union _ATOM_PPLIB_CAC_Leakage_Record
> @@ -554,7 +554,7 @@ typedef union _ATOM_PPLIB_CAC_Leakage_Record ATOM_PPLIB_CAC_Leakage_Record;
> typedef struct _ATOM_PPLIB_CAC_Leakage_Table
> {
> UCHAR ucNumEntries; // Number of entries.
> - ATOM_PPLIB_CAC_Leakage_Record entries[1]; // Dynamically allocate entries.
> + ATOM_PPLIB_CAC_Leakage_Record entries[]; // Dynamically allocate entries.
> }ATOM_PPLIB_CAC_Leakage_Table;
>
> typedef struct _ATOM_PPLIB_PhaseSheddingLimits_Record
> @@ -569,7 +569,7 @@ typedef struct _ATOM_PPLIB_PhaseSheddingLimits_Record
> typedef struct _ATOM_PPLIB_PhaseSheddingLimits_Table
> {
> UCHAR ucNumEntries; // Number of entries.
> - ATOM_PPLIB_PhaseSheddingLimits_Record entries[1]; // Dynamically allocate entries.
> + ATOM_PPLIB_PhaseSheddingLimits_Record entries[]; // Dynamically allocate entries.
> }ATOM_PPLIB_PhaseSheddingLimits_Table;
>
> typedef struct _VCEClockInfo{
> @@ -581,7 +581,7 @@ typedef struct _VCEClockInfo{
>
> typedef struct _VCEClockInfoArray{
> UCHAR ucNumEntries;
> - VCEClockInfo entries[1];
> + VCEClockInfo entries[];
> }VCEClockInfoArray;
>
> typedef struct _ATOM_PPLIB_VCE_Clock_Voltage_Limit_Record
> @@ -593,7 +593,7 @@ typedef struct _ATOM_PPLIB_VCE_Clock_Voltage_Limit_Record
> typedef struct _ATOM_PPLIB_VCE_Clock_Voltage_Limit_Table
> {
> UCHAR numEntries;
> - ATOM_PPLIB_VCE_Clock_Voltage_Limit_Record entries[1];
> + ATOM_PPLIB_VCE_Clock_Voltage_Limit_Record entries[];
> }ATOM_PPLIB_VCE_Clock_Voltage_Limit_Table;
>
> typedef struct _ATOM_PPLIB_VCE_State_Record
> @@ -605,7 +605,7 @@ typedef struct _ATOM_PPLIB_VCE_State_Record
> typedef struct _ATOM_PPLIB_VCE_State_Table
> {
> UCHAR numEntries;
> - ATOM_PPLIB_VCE_State_Record entries[1];
> + ATOM_PPLIB_VCE_State_Record entries[];
> }ATOM_PPLIB_VCE_State_Table;
>
>
> @@ -627,7 +627,7 @@ typedef struct _UVDClockInfo{
>
> typedef struct _UVDClockInfoArray{
> UCHAR ucNumEntries;
> - UVDClockInfo entries[1];
> + UVDClockInfo entries[];
> }UVDClockInfoArray;
>
> typedef struct _ATOM_PPLIB_UVD_Clock_Voltage_Limit_Record
> @@ -639,7 +639,7 @@ typedef struct _ATOM_PPLIB_UVD_Clock_Voltage_Limit_Record
> typedef struct _ATOM_PPLIB_UVD_Clock_Voltage_Limit_Table
> {
> UCHAR numEntries;
> - ATOM_PPLIB_UVD_Clock_Voltage_Limit_Record entries[1];
> + ATOM_PPLIB_UVD_Clock_Voltage_Limit_Record entries[];
> }ATOM_PPLIB_UVD_Clock_Voltage_Limit_Table;
>
> typedef struct _ATOM_PPLIB_UVD_Table
> @@ -676,7 +676,7 @@ typedef struct _ATOM_PPLIB_ACPClk_Voltage_Limit_Record
>
> typedef struct _ATOM_PPLIB_ACPClk_Voltage_Limit_Table{
> UCHAR numEntries;
> - ATOM_PPLIB_ACPClk_Voltage_Limit_Record entries[1];
> + ATOM_PPLIB_ACPClk_Voltage_Limit_Record entries[];
> }ATOM_PPLIB_ACPClk_Voltage_Limit_Table;
>
> typedef struct _ATOM_PPLIB_ACP_Table
> @@ -745,7 +745,7 @@ typedef struct ATOM_PPLIB_VQ_Budgeting_Record{
> typedef struct ATOM_PPLIB_VQ_Budgeting_Table {
> UCHAR revid;
> UCHAR numEntries;
> - ATOM_PPLIB_VQ_Budgeting_Record entries[1];
> + ATOM_PPLIB_VQ_Budgeting_Record entries[];
> } ATOM_PPLIB_VQ_Budgeting_Table;
>
> #pragma pack()
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] drm/amdgpu/pptable: Fix UBSAN array-index-out-of-bounds
2024-05-23 14:52 ` Alex Deucher
@ 2024-05-27 9:42 ` Tasos Sahanidis
2024-05-27 14:47 ` Alex Deucher
0 siblings, 1 reply; 6+ messages in thread
From: Tasos Sahanidis @ 2024-05-27 9:42 UTC (permalink / raw)
To: Alex Deucher; +Cc: amd-gfx
On 2024-05-23 17:52, Alex Deucher wrote:
> On Thu, May 23, 2024 at 9:05 AM Tasos Sahanidis <tasos@tasossah.com> wrote:
>>
>> Dyanmically sized arrays used [1] instead of []. Replacing the former
>> with the latter resolves multiple warnings observed on boot with a
>> BONAIRE card.
>>
>> Signed-off-by: Tasos Sahanidis <tasos@tasossah.com>
>> ---
>> drivers/gpu/drm/amd/include/pptable.h | 24 ++++++++++++------------
>> 1 file changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/include/pptable.h b/drivers/gpu/drm/amd/include/pptable.h
>> index 2e8e6c9875f6..d1dec880d2d6 100644
>> --- a/drivers/gpu/drm/amd/include/pptable.h
>> +++ b/drivers/gpu/drm/amd/include/pptable.h
>> @@ -480,7 +480,7 @@ typedef struct _StateArray{
>> //how many states we have
>> UCHAR ucNumEntries;
>>
>> - ATOM_PPLIB_STATE_V2 states[1];
>> + ATOM_PPLIB_STATE_V2 states[];
>
> Can you add __counted_by(ucNumEntries) to the end of the line? E.g.,
>
> ATOM_PPLIB_STATE_V2 states[] __counted_by(ucNumEntries);
>
> Same comment for the other changes below.
>
> Alex
Sure thing! I have the v2 ready and will submit after testing it on
hardware. I do have a question though. The following already uses [].
Would it be acceptable to also modify it in the same patch?
@@ -658,7 +658,7 @@ typedef struct _ATOM_PPLIB_SAMClk_Voltage_Limit_Record
typedef struct _ATOM_PPLIB_SAMClk_Voltage_Limit_Table{
UCHAR numEntries;
- ATOM_PPLIB_SAMClk_Voltage_Limit_Record entries[];
+ ATOM_PPLIB_SAMClk_Voltage_Limit_Record entries[] __counted_by(numEntries);
}ATOM_PPLIB_SAMClk_Voltage_Limit_Table;
typedef struct _ATOM_PPLIB_SAMU_Table
There's also this, which I think __counted_by can be used here as well:
diff --git a/drivers/gpu/drm/amd/include/pptable.h b/drivers/gpu/drm/amd/include/pptable.h
index febc853d2a07..341d4a4e8d98 100644
--- a/drivers/gpu/drm/amd/include/pptable.h
+++ b/drivers/gpu/drm/amd/include/pptable.h
@@ -497,15 +497,15 @@ typedef struct _ClockInfoArray{
typedef struct _NonClockInfoArray{
//how many non-clock levels we have. normally should be same as number of states
UCHAR ucNumEntries;
//sizeof(ATOM_PPLIB_NONCLOCK_INFO)
UCHAR ucEntrySize;
- ATOM_PPLIB_NONCLOCK_INFO nonClockInfo[];
+ ATOM_PPLIB_NONCLOCK_INFO nonClockInfo[] __counted_by(ucNumEntries);
}NonClockInfoArray;
typedef struct _ATOM_PPLIB_Clock_Voltage_Dependency_Record
{
USHORT usClockLow;
UCHAR ucClockHigh;
USHORT usVoltage;
All the other ones are UCHAR, so __counted_by can not be used on them.
Please let me know what you think.
Thanks!
--
Tasos
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] drm/amdgpu/pptable: Fix UBSAN array-index-out-of-bounds
2024-05-27 9:42 ` Tasos Sahanidis
@ 2024-05-27 14:47 ` Alex Deucher
2024-05-30 10:31 ` Tasos Sahanidis
0 siblings, 1 reply; 6+ messages in thread
From: Alex Deucher @ 2024-05-27 14:47 UTC (permalink / raw)
To: Tasos Sahanidis; +Cc: amd-gfx
On Mon, May 27, 2024 at 5:42 AM Tasos Sahanidis <tasos@tasossah.com> wrote:
>
> On 2024-05-23 17:52, Alex Deucher wrote:
> > On Thu, May 23, 2024 at 9:05 AM Tasos Sahanidis <tasos@tasossah.com> wrote:
> >>
> >> Dyanmically sized arrays used [1] instead of []. Replacing the former
> >> with the latter resolves multiple warnings observed on boot with a
> >> BONAIRE card.
> >>
> >> Signed-off-by: Tasos Sahanidis <tasos@tasossah.com>
> >> ---
> >> drivers/gpu/drm/amd/include/pptable.h | 24 ++++++++++++------------
> >> 1 file changed, 12 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/include/pptable.h b/drivers/gpu/drm/amd/include/pptable.h
> >> index 2e8e6c9875f6..d1dec880d2d6 100644
> >> --- a/drivers/gpu/drm/amd/include/pptable.h
> >> +++ b/drivers/gpu/drm/amd/include/pptable.h
> >> @@ -480,7 +480,7 @@ typedef struct _StateArray{
> >> //how many states we have
> >> UCHAR ucNumEntries;
> >>
> >> - ATOM_PPLIB_STATE_V2 states[1];
> >> + ATOM_PPLIB_STATE_V2 states[];
> >
> > Can you add __counted_by(ucNumEntries) to the end of the line? E.g.,
> >
> > ATOM_PPLIB_STATE_V2 states[] __counted_by(ucNumEntries);
> >
> > Same comment for the other changes below.
> >
> > Alex
>
> Sure thing! I have the v2 ready and will submit after testing it on
> hardware. I do have a question though. The following already uses [].
> Would it be acceptable to also modify it in the same patch?
Yes, that's fine. Thanks!
>
> @@ -658,7 +658,7 @@ typedef struct _ATOM_PPLIB_SAMClk_Voltage_Limit_Record
>
> typedef struct _ATOM_PPLIB_SAMClk_Voltage_Limit_Table{
> UCHAR numEntries;
> - ATOM_PPLIB_SAMClk_Voltage_Limit_Record entries[];
> + ATOM_PPLIB_SAMClk_Voltage_Limit_Record entries[] __counted_by(numEntries);
> }ATOM_PPLIB_SAMClk_Voltage_Limit_Table;
>
> typedef struct _ATOM_PPLIB_SAMU_Table
>
> There's also this, which I think __counted_by can be used here as well:
yes.
>
> diff --git a/drivers/gpu/drm/amd/include/pptable.h b/drivers/gpu/drm/amd/include/pptable.h
> index febc853d2a07..341d4a4e8d98 100644
> --- a/drivers/gpu/drm/amd/include/pptable.h
> +++ b/drivers/gpu/drm/amd/include/pptable.h
> @@ -497,15 +497,15 @@ typedef struct _ClockInfoArray{
> typedef struct _NonClockInfoArray{
>
> //how many non-clock levels we have. normally should be same as number of states
> UCHAR ucNumEntries;
> //sizeof(ATOM_PPLIB_NONCLOCK_INFO)
> UCHAR ucEntrySize;
>
> - ATOM_PPLIB_NONCLOCK_INFO nonClockInfo[];
> + ATOM_PPLIB_NONCLOCK_INFO nonClockInfo[] __counted_by(ucNumEntries);
Yes.
> }NonClockInfoArray;
>
> typedef struct _ATOM_PPLIB_Clock_Voltage_Dependency_Record
> {
> USHORT usClockLow;
> UCHAR ucClockHigh;
> USHORT usVoltage;
>
> All the other ones are UCHAR, so __counted_by can not be used on them.
I'm not following. Why not?
Alex
>
> Please let me know what you think.
>
> Thanks!
>
> --
> Tasos
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] drm/amdgpu/pptable: Fix UBSAN array-index-out-of-bounds
2024-05-27 14:47 ` Alex Deucher
@ 2024-05-30 10:31 ` Tasos Sahanidis
2024-05-30 15:24 ` Alex Deucher
0 siblings, 1 reply; 6+ messages in thread
From: Tasos Sahanidis @ 2024-05-30 10:31 UTC (permalink / raw)
To: Alex Deucher; +Cc: amd-gfx
On Mon, May 27, 2024 at 10:47:39AM -0400, Alex Deucher wrote:
> On Mon, May 27, 2024 at 5:42 AM Tasos Sahanidis <tasos@tasossah.com> wrote:
> >
> > On 2024-05-23 17:52, Alex Deucher wrote:
> > > On Thu, May 23, 2024 at 9:05 AM Tasos Sahanidis <tasos@tasossah.com> wrote:
> > >>
> > >> Dyanmically sized arrays used [1] instead of []. Replacing the former
> > >> with the latter resolves multiple warnings observed on boot with a
> > >> BONAIRE card.
> > >>
> > >> Signed-off-by: Tasos Sahanidis <tasos@tasossah.com>
> > >> ---
> > >> drivers/gpu/drm/amd/include/pptable.h | 24 ++++++++++++------------
> > >> 1 file changed, 12 insertions(+), 12 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/amd/include/pptable.h b/drivers/gpu/drm/amd/include/pptable.h
> > >> index 2e8e6c9875f6..d1dec880d2d6 100644
> > >> --- a/drivers/gpu/drm/amd/include/pptable.h
> > >> +++ b/drivers/gpu/drm/amd/include/pptable.h
> > >> @@ -480,7 +480,7 @@ typedef struct _StateArray{
> > >> //how many states we have
> > >> UCHAR ucNumEntries;
> > >>
> > >> - ATOM_PPLIB_STATE_V2 states[1];
> > >> + ATOM_PPLIB_STATE_V2 states[];
> > >
> > > Can you add __counted_by(ucNumEntries) to the end of the line? E.g.,
> > >
> > > ATOM_PPLIB_STATE_V2 states[] __counted_by(ucNumEntries);
> > >
> > > Same comment for the other changes below.
> > >
> > > Alex
> >
Apologies for the delay. I realised my compiler actually didn't support
the attribute, so I had to install clang 19 from git.
After doing so, I saw this warning about ATOM_PPLIB_STATE_V2.
drivers/gpu/drm/amd/amdgpu/../include/pptable.h:483:5: warning: 'counted_by' should not be applied to an array with element of unknown size because 'ATOM_PPLIB_STATE_V2'
(aka 'struct _ATOM_PPLIB_STATE_V2') is a struct type with a flexible array member. This will be an error in a future compiler version [-Wbounds-safety-counted-by-elt-type-unknown-size]
483 | ATOM_PPLIB_STATE_V2 states[] __counted_by(ucNumEntries);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
Thus I'll remove the __counted_by() from that one.
> > }NonClockInfoArray;
> >
> > typedef struct _ATOM_PPLIB_Clock_Voltage_Dependency_Record
> > {
> > USHORT usClockLow;
> > UCHAR ucClockHigh;
> > USHORT usVoltage;
> >
> > All the other ones are UCHAR, so __counted_by can not be used on them.
>
> I'm not following. Why not?
>
> Alex
If I'm not mistaken, for a UCHAR flexible array such as clockInfo[],
we would then need to multiply the count by the size, which results in:
drivers/gpu/drm/amd/amdgpu/../include/pptable.h:494:36: error: 'counted_by' argument must be a simple declaration reference
494 | UCHAR clockInfo[] __counted_by(ucEntrySize * ucNumEntries);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
./include/linux/compiler_attributes.h:105:62: note: expanded from macro '__counted_by'
105 | # define __counted_by(member) __attribute__((__counted_by__(member)))
| ^~~~~~
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] drm/amdgpu/pptable: Fix UBSAN array-index-out-of-bounds
2024-05-30 10:31 ` Tasos Sahanidis
@ 2024-05-30 15:24 ` Alex Deucher
0 siblings, 0 replies; 6+ messages in thread
From: Alex Deucher @ 2024-05-30 15:24 UTC (permalink / raw)
To: Tasos Sahanidis; +Cc: amd-gfx
On Thu, May 30, 2024 at 6:31 AM Tasos Sahanidis <tasos@tasossah.com> wrote:
>
> On Mon, May 27, 2024 at 10:47:39AM -0400, Alex Deucher wrote:
> > On Mon, May 27, 2024 at 5:42 AM Tasos Sahanidis <tasos@tasossah.com> wrote:
> > >
> > > On 2024-05-23 17:52, Alex Deucher wrote:
> > > > On Thu, May 23, 2024 at 9:05 AM Tasos Sahanidis <tasos@tasossah.com> wrote:
> > > >>
> > > >> Dyanmically sized arrays used [1] instead of []. Replacing the former
> > > >> with the latter resolves multiple warnings observed on boot with a
> > > >> BONAIRE card.
> > > >>
> > > >> Signed-off-by: Tasos Sahanidis <tasos@tasossah.com>
> > > >> ---
> > > >> drivers/gpu/drm/amd/include/pptable.h | 24 ++++++++++++------------
> > > >> 1 file changed, 12 insertions(+), 12 deletions(-)
> > > >>
> > > >> diff --git a/drivers/gpu/drm/amd/include/pptable.h b/drivers/gpu/drm/amd/include/pptable.h
> > > >> index 2e8e6c9875f6..d1dec880d2d6 100644
> > > >> --- a/drivers/gpu/drm/amd/include/pptable.h
> > > >> +++ b/drivers/gpu/drm/amd/include/pptable.h
> > > >> @@ -480,7 +480,7 @@ typedef struct _StateArray{
> > > >> //how many states we have
> > > >> UCHAR ucNumEntries;
> > > >>
> > > >> - ATOM_PPLIB_STATE_V2 states[1];
> > > >> + ATOM_PPLIB_STATE_V2 states[];
> > > >
> > > > Can you add __counted_by(ucNumEntries) to the end of the line? E.g.,
> > > >
> > > > ATOM_PPLIB_STATE_V2 states[] __counted_by(ucNumEntries);
> > > >
> > > > Same comment for the other changes below.
> > > >
> > > > Alex
> > >
>
> Apologies for the delay. I realised my compiler actually didn't support
> the attribute, so I had to install clang 19 from git.
>
> After doing so, I saw this warning about ATOM_PPLIB_STATE_V2.
>
> drivers/gpu/drm/amd/amdgpu/../include/pptable.h:483:5: warning: 'counted_by' should not be applied to an array with element of unknown size because 'ATOM_PPLIB_STATE_V2'
> (aka 'struct _ATOM_PPLIB_STATE_V2') is a struct type with a flexible array member. This will be an error in a future compiler version [-Wbounds-safety-counted-by-elt-type-unknown-size]
> 483 | ATOM_PPLIB_STATE_V2 states[] __counted_by(ucNumEntries);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Thus I'll remove the __counted_by() from that one.
just leave it there commented out. E.g.,
ATOM_PPLIB_STATE_V2 states[] /* __counted_by(ucNumEntries) */;
for reference so if the compiler ever gets updated to handle this, it
can be re-enabled.
>
> > > }NonClockInfoArray;
> > >
> > > typedef struct _ATOM_PPLIB_Clock_Voltage_Dependency_Record
> > > {
> > > USHORT usClockLow;
> > > UCHAR ucClockHigh;
> > > USHORT usVoltage;
> > >
> > > All the other ones are UCHAR, so __counted_by can not be used on them.
> >
> > I'm not following. Why not?
> >
> > Alex
>
> If I'm not mistaken, for a UCHAR flexible array such as clockInfo[],
> we would then need to multiply the count by the size, which results in:
>
> drivers/gpu/drm/amd/amdgpu/../include/pptable.h:494:36: error: 'counted_by' argument must be a simple declaration reference
> 494 | UCHAR clockInfo[] __counted_by(ucEntrySize * ucNumEntries);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~
> ./include/linux/compiler_attributes.h:105:62: note: expanded from macro '__counted_by'
> 105 | # define __counted_by(member) __attribute__((__counted_by__(member)))
> | ^~~~~~
Ah, yes, we can skip those.
Alex
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-05-30 15:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-23 7:16 [PATCH] drm/amdgpu/pptable: Fix UBSAN array-index-out-of-bounds Tasos Sahanidis
2024-05-23 14:52 ` Alex Deucher
2024-05-27 9:42 ` Tasos Sahanidis
2024-05-27 14:47 ` Alex Deucher
2024-05-30 10:31 ` Tasos Sahanidis
2024-05-30 15:24 ` Alex Deucher
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.