* [PATCH] drm/amdgpu: replace 1-element arrays with flexible arrays
@ 2023-07-12 8:31 Ricardo Cañuelo
2023-07-12 14:12 ` Alex Deucher
0 siblings, 1 reply; 5+ messages in thread
From: Ricardo Cañuelo @ 2023-07-12 8:31 UTC (permalink / raw)
To: alexander.deucher; +Cc: kernel, amd-gfx, linux-kernel
UBSAN complains about out-of-bounds array indexes on all 1-element
arrays defined on this driver:
UBSAN: array-index-out-of-bounds in /home/rcn/work/repos/kernelci/kernelci-core/linux_kernel_mainline/drivers/gpu/drm/amd/amdgpu/../pm/powerplay/hwmgr/processpptables.c:1249:61
Substitute them with proper flexible arrays.
Tested on an Acer R721T (grunt) Chromebook.
Signed-off-by: Ricardo Cañuelo <ricardo.canuelo@collabora.com>
---
drivers/gpu/drm/amd/include/pptable.h | 36 +++++++++++++++------------
1 file changed, 20 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/amd/include/pptable.h b/drivers/gpu/drm/amd/include/pptable.h
index 0b6a057e0a4c..a65e2807dc06 100644
--- a/drivers/gpu/drm/amd/include/pptable.h
+++ b/drivers/gpu/drm/amd/include/pptable.h
@@ -473,14 +473,14 @@ typedef struct _ATOM_PPLIB_STATE_V2
/**
* Driver will read the first ucNumDPMLevels in this array
*/
- UCHAR clockInfoIndex[1];
+ __DECLARE_FLEX_ARRAY(UCHAR, clockInfoIndex);
} ATOM_PPLIB_STATE_V2;
typedef struct _StateArray{
//how many states we have
UCHAR ucNumEntries;
- ATOM_PPLIB_STATE_V2 states[1];
+ __DECLARE_FLEX_ARRAY(ATOM_PPLIB_STATE_V2, states);
}StateArray;
@@ -491,7 +491,7 @@ typedef struct _ClockInfoArray{
//sizeof(ATOM_PPLIB_CLOCK_INFO)
UCHAR ucEntrySize;
- UCHAR clockInfo[1];
+ __DECLARE_FLEX_ARRAY(UCHAR, clockInfo);
}ClockInfoArray;
typedef struct _NonClockInfoArray{
@@ -501,7 +501,7 @@ typedef struct _NonClockInfoArray{
//sizeof(ATOM_PPLIB_NONCLOCK_INFO)
UCHAR ucEntrySize;
- ATOM_PPLIB_NONCLOCK_INFO nonClockInfo[1];
+ __DECLARE_FLEX_ARRAY(ATOM_PPLIB_NONCLOCK_INFO, nonClockInfo);
}NonClockInfoArray;
typedef struct _ATOM_PPLIB_Clock_Voltage_Dependency_Record
@@ -514,7 +514,8 @@ 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.
+ /* Dynamically allocate entries. */
+ __DECLARE_FLEX_ARRAY(ATOM_PPLIB_Clock_Voltage_Dependency_Record, entries);
}ATOM_PPLIB_Clock_Voltage_Dependency_Table;
typedef struct _ATOM_PPLIB_Clock_Voltage_Limit_Record
@@ -530,7 +531,8 @@ 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.
+ /* Dynamically allocate entries. */
+ __DECLARE_FLEX_ARRAY(ATOM_PPLIB_Clock_Voltage_Limit_Record, entries);
}ATOM_PPLIB_Clock_Voltage_Limit_Table;
union _ATOM_PPLIB_CAC_Leakage_Record
@@ -554,7 +556,8 @@ 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.
+ /* Dynamically allocate entries. */
+ __DECLARE_FLEX_ARRAY(ATOM_PPLIB_CAC_Leakage_Record, entries);
}ATOM_PPLIB_CAC_Leakage_Table;
typedef struct _ATOM_PPLIB_PhaseSheddingLimits_Record
@@ -569,7 +572,8 @@ 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.
+ /* Dynamically allocate entries. */
+ __DECLARE_FLEX_ARRAY(ATOM_PPLIB_PhaseSheddingLimits_Record, entries);
}ATOM_PPLIB_PhaseSheddingLimits_Table;
typedef struct _VCEClockInfo{
@@ -581,7 +585,7 @@ typedef struct _VCEClockInfo{
typedef struct _VCEClockInfoArray{
UCHAR ucNumEntries;
- VCEClockInfo entries[1];
+ __DECLARE_FLEX_ARRAY(VCEClockInfo, entries);
}VCEClockInfoArray;
typedef struct _ATOM_PPLIB_VCE_Clock_Voltage_Limit_Record
@@ -593,7 +597,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];
+ __DECLARE_FLEX_ARRAY(ATOM_PPLIB_VCE_Clock_Voltage_Limit_Record, entries);
}ATOM_PPLIB_VCE_Clock_Voltage_Limit_Table;
typedef struct _ATOM_PPLIB_VCE_State_Record
@@ -605,7 +609,7 @@ typedef struct _ATOM_PPLIB_VCE_State_Record
typedef struct _ATOM_PPLIB_VCE_State_Table
{
UCHAR numEntries;
- ATOM_PPLIB_VCE_State_Record entries[1];
+ __DECLARE_FLEX_ARRAY(ATOM_PPLIB_VCE_State_Record, entries);
}ATOM_PPLIB_VCE_State_Table;
@@ -627,7 +631,7 @@ typedef struct _UVDClockInfo{
typedef struct _UVDClockInfoArray{
UCHAR ucNumEntries;
- UVDClockInfo entries[1];
+ __DECLARE_FLEX_ARRAY(UVDClockInfo, entries);
}UVDClockInfoArray;
typedef struct _ATOM_PPLIB_UVD_Clock_Voltage_Limit_Record
@@ -639,7 +643,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];
+ __DECLARE_FLEX_ARRAY(ATOM_PPLIB_UVD_Clock_Voltage_Limit_Record, entries);
}ATOM_PPLIB_UVD_Clock_Voltage_Limit_Table;
typedef struct _ATOM_PPLIB_UVD_Table
@@ -658,7 +662,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[1];
+ __DECLARE_FLEX_ARRAY(ATOM_PPLIB_SAMClk_Voltage_Limit_Record, entries);
}ATOM_PPLIB_SAMClk_Voltage_Limit_Table;
typedef struct _ATOM_PPLIB_SAMU_Table
@@ -676,7 +680,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];
+ __DECLARE_FLEX_ARRAY(ATOM_PPLIB_ACPClk_Voltage_Limit_Record, entries);
}ATOM_PPLIB_ACPClk_Voltage_Limit_Table;
typedef struct _ATOM_PPLIB_ACP_Table
@@ -745,7 +749,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];
+ __DECLARE_FLEX_ARRAY(ATOM_PPLIB_VQ_Budgeting_Record, entries);
} ATOM_PPLIB_VQ_Budgeting_Table;
#pragma pack()
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] drm/amdgpu: replace 1-element arrays with flexible arrays
2023-07-12 8:31 [PATCH] drm/amdgpu: replace 1-element arrays with flexible arrays Ricardo Cañuelo
@ 2023-07-12 14:12 ` Alex Deucher
2023-07-14 10:40 ` Paulo Miguel Almeida
2023-07-14 18:13 ` Gustavo A. R. Silva
0 siblings, 2 replies; 5+ messages in thread
From: Alex Deucher @ 2023-07-12 14:12 UTC (permalink / raw)
To: Ricardo Cañuelo, Gustavo A. R. Silva, Paulo Miguel Almeida
Cc: alexander.deucher, kernel, linux-kernel, amd-gfx
On Wed, Jul 12, 2023 at 8:04 AM Ricardo Cañuelo
<ricardo.canuelo@collabora.com> wrote:
>
> UBSAN complains about out-of-bounds array indexes on all 1-element
> arrays defined on this driver:
>
> UBSAN: array-index-out-of-bounds in /home/rcn/work/repos/kernelci/kernelci-core/linux_kernel_mainline/drivers/gpu/drm/amd/amdgpu/../pm/powerplay/hwmgr/processpptables.c:1249:61
>
> Substitute them with proper flexible arrays.
+ Gustavo, Paulo
I haven't kept up with the flexible arrays stuff. Is this equivalent
to a zero sized array? We've been bitten by these kind of changes in
the past. These structures define the layout of data in a rom image
on the board. If the struct size changes, that could lead to errors
in the code that deals with these structures.
Alex
>
> Tested on an Acer R721T (grunt) Chromebook.
>
> Signed-off-by: Ricardo Cañuelo <ricardo.canuelo@collabora.com>
> ---
> drivers/gpu/drm/amd/include/pptable.h | 36 +++++++++++++++------------
> 1 file changed, 20 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/include/pptable.h b/drivers/gpu/drm/amd/include/pptable.h
> index 0b6a057e0a4c..a65e2807dc06 100644
> --- a/drivers/gpu/drm/amd/include/pptable.h
> +++ b/drivers/gpu/drm/amd/include/pptable.h
> @@ -473,14 +473,14 @@ typedef struct _ATOM_PPLIB_STATE_V2
> /**
> * Driver will read the first ucNumDPMLevels in this array
> */
> - UCHAR clockInfoIndex[1];
> + __DECLARE_FLEX_ARRAY(UCHAR, clockInfoIndex);
> } ATOM_PPLIB_STATE_V2;
>
> typedef struct _StateArray{
> //how many states we have
> UCHAR ucNumEntries;
>
> - ATOM_PPLIB_STATE_V2 states[1];
> + __DECLARE_FLEX_ARRAY(ATOM_PPLIB_STATE_V2, states);
> }StateArray;
>
>
> @@ -491,7 +491,7 @@ typedef struct _ClockInfoArray{
> //sizeof(ATOM_PPLIB_CLOCK_INFO)
> UCHAR ucEntrySize;
>
> - UCHAR clockInfo[1];
> + __DECLARE_FLEX_ARRAY(UCHAR, clockInfo);
> }ClockInfoArray;
>
> typedef struct _NonClockInfoArray{
> @@ -501,7 +501,7 @@ typedef struct _NonClockInfoArray{
> //sizeof(ATOM_PPLIB_NONCLOCK_INFO)
> UCHAR ucEntrySize;
>
> - ATOM_PPLIB_NONCLOCK_INFO nonClockInfo[1];
> + __DECLARE_FLEX_ARRAY(ATOM_PPLIB_NONCLOCK_INFO, nonClockInfo);
> }NonClockInfoArray;
>
> typedef struct _ATOM_PPLIB_Clock_Voltage_Dependency_Record
> @@ -514,7 +514,8 @@ 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.
> + /* Dynamically allocate entries. */
> + __DECLARE_FLEX_ARRAY(ATOM_PPLIB_Clock_Voltage_Dependency_Record, entries);
> }ATOM_PPLIB_Clock_Voltage_Dependency_Table;
>
> typedef struct _ATOM_PPLIB_Clock_Voltage_Limit_Record
> @@ -530,7 +531,8 @@ 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.
> + /* Dynamically allocate entries. */
> + __DECLARE_FLEX_ARRAY(ATOM_PPLIB_Clock_Voltage_Limit_Record, entries);
> }ATOM_PPLIB_Clock_Voltage_Limit_Table;
>
> union _ATOM_PPLIB_CAC_Leakage_Record
> @@ -554,7 +556,8 @@ 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.
> + /* Dynamically allocate entries. */
> + __DECLARE_FLEX_ARRAY(ATOM_PPLIB_CAC_Leakage_Record, entries);
> }ATOM_PPLIB_CAC_Leakage_Table;
>
> typedef struct _ATOM_PPLIB_PhaseSheddingLimits_Record
> @@ -569,7 +572,8 @@ 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.
> + /* Dynamically allocate entries. */
> + __DECLARE_FLEX_ARRAY(ATOM_PPLIB_PhaseSheddingLimits_Record, entries);
> }ATOM_PPLIB_PhaseSheddingLimits_Table;
>
> typedef struct _VCEClockInfo{
> @@ -581,7 +585,7 @@ typedef struct _VCEClockInfo{
>
> typedef struct _VCEClockInfoArray{
> UCHAR ucNumEntries;
> - VCEClockInfo entries[1];
> + __DECLARE_FLEX_ARRAY(VCEClockInfo, entries);
> }VCEClockInfoArray;
>
> typedef struct _ATOM_PPLIB_VCE_Clock_Voltage_Limit_Record
> @@ -593,7 +597,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];
> + __DECLARE_FLEX_ARRAY(ATOM_PPLIB_VCE_Clock_Voltage_Limit_Record, entries);
> }ATOM_PPLIB_VCE_Clock_Voltage_Limit_Table;
>
> typedef struct _ATOM_PPLIB_VCE_State_Record
> @@ -605,7 +609,7 @@ typedef struct _ATOM_PPLIB_VCE_State_Record
> typedef struct _ATOM_PPLIB_VCE_State_Table
> {
> UCHAR numEntries;
> - ATOM_PPLIB_VCE_State_Record entries[1];
> + __DECLARE_FLEX_ARRAY(ATOM_PPLIB_VCE_State_Record, entries);
> }ATOM_PPLIB_VCE_State_Table;
>
>
> @@ -627,7 +631,7 @@ typedef struct _UVDClockInfo{
>
> typedef struct _UVDClockInfoArray{
> UCHAR ucNumEntries;
> - UVDClockInfo entries[1];
> + __DECLARE_FLEX_ARRAY(UVDClockInfo, entries);
> }UVDClockInfoArray;
>
> typedef struct _ATOM_PPLIB_UVD_Clock_Voltage_Limit_Record
> @@ -639,7 +643,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];
> + __DECLARE_FLEX_ARRAY(ATOM_PPLIB_UVD_Clock_Voltage_Limit_Record, entries);
> }ATOM_PPLIB_UVD_Clock_Voltage_Limit_Table;
>
> typedef struct _ATOM_PPLIB_UVD_Table
> @@ -658,7 +662,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[1];
> + __DECLARE_FLEX_ARRAY(ATOM_PPLIB_SAMClk_Voltage_Limit_Record, entries);
> }ATOM_PPLIB_SAMClk_Voltage_Limit_Table;
>
> typedef struct _ATOM_PPLIB_SAMU_Table
> @@ -676,7 +680,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];
> + __DECLARE_FLEX_ARRAY(ATOM_PPLIB_ACPClk_Voltage_Limit_Record, entries);
> }ATOM_PPLIB_ACPClk_Voltage_Limit_Table;
>
> typedef struct _ATOM_PPLIB_ACP_Table
> @@ -745,7 +749,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];
> + __DECLARE_FLEX_ARRAY(ATOM_PPLIB_VQ_Budgeting_Record, entries);
> } ATOM_PPLIB_VQ_Budgeting_Table;
>
> #pragma pack()
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/amdgpu: replace 1-element arrays with flexible arrays
2023-07-12 14:12 ` Alex Deucher
@ 2023-07-14 10:40 ` Paulo Miguel Almeida
2023-07-14 12:20 ` Ricardo Cañuelo
2023-07-14 18:13 ` Gustavo A. R. Silva
1 sibling, 1 reply; 5+ messages in thread
From: Paulo Miguel Almeida @ 2023-07-14 10:40 UTC (permalink / raw)
To: Alex Deucher
Cc: Ricardo Cañuelo, Gustavo A. R. Silva, alexander.deucher,
kernel, linux-kernel, amd-gfx, linux-hardening
On Wed, Jul 12, 2023 at 10:12:08AM -0400, Alex Deucher wrote:
> On Wed, Jul 12, 2023 at 8:04 AM Ricardo Cañuelo
> <ricardo.canuelo@collabora.com> wrote:
> >
> > UBSAN complains about out-of-bounds array indexes on all 1-element
> > arrays defined on this driver:
> >
> > UBSAN: array-index-out-of-bounds in /home/rcn/work/repos/kernelci/kernelci-core/linux_kernel_mainline/drivers/gpu/drm/amd/amdgpu/../pm/powerplay/hwmgr/processpptables.c:1249:61
> >
> > Substitute them with proper flexible arrays.
>
Hi Ricardo,
thanks for your patch! when submitting patches regarding flex-arrays
please cc linux-hardening@vger.kernel.org too :-)
I added my considerations in-line.
> + Gustavo, Paulo
>
> I haven't kept up with the flexible arrays stuff. Is this equivalent
> to a zero sized array? We've been bitten by these kind of changes in
> the past. These structures define the layout of data in a rom image
> on the board. If the struct size changes, that could lead to errors
> in the code that deals with these structures.
>
> Alex
Hi Alex,
correct, both zero-sized and one-sized arrays (in some contexts)
are deprecated [1] and being replaced with flexible arrays as part of the
kernel self protection project (KSSP) led by Kees Cook and Gustavo Silva.
Converting away from them can be tricky, and I think such conversions need
to explicitly show how they were checked for binary differences[2].
Ricardo, can you please check for deltas and report your findings in the
commit log?
[1] https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
[2] https://outflux.net/blog/archives/2022/06/24/finding-binary-differences/
>
> >
> > Tested on an Acer R721T (grunt) Chromebook.
> >
> > Signed-off-by: Ricardo Cañuelo <ricardo.canuelo@collabora.com>
> > ---
> > drivers/gpu/drm/amd/include/pptable.h | 36 +++++++++++++++------------
> > 1 file changed, 20 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/include/pptable.h b/drivers/gpu/drm/amd/include/pptable.h
> > index 0b6a057e0a4c..a65e2807dc06 100644
> > --- a/drivers/gpu/drm/amd/include/pptable.h
> > +++ b/drivers/gpu/drm/amd/include/pptable.h
> > @@ -473,14 +473,14 @@ typedef struct _ATOM_PPLIB_STATE_V2
> > /**
> > * Driver will read the first ucNumDPMLevels in this array
> > */
> > - UCHAR clockInfoIndex[1];
> > + __DECLARE_FLEX_ARRAY(UCHAR, clockInfoIndex);
_All_ references of __DECLARE_FLEX_ARRAY in this patch should be replaced
with DECLARE_FLEX_ARRAY macro (without __).
While they converge to the same code today, __DECLARE_FLEX_ARRAY macro
is specific for UAPI while DECLARE_FLEX_ARRAY isn't as seen in this
patch:
https://github.com/torvalds/linux/commit/3080ea5553cc909b000d1f1d964a9041962f2c5b
> > } ATOM_PPLIB_STATE_V2;
> >
[... snip for brevity ]
> >
> > typedef struct _VCEClockInfo{
> > @@ -581,7 +585,7 @@ typedef struct _VCEClockInfo{
> >
> > typedef struct _VCEClockInfoArray{
> > UCHAR ucNumEntries;
> > - VCEClockInfo entries[1];
> > + __DECLARE_FLEX_ARRAY(VCEClockInfo, entries);
> > }VCEClockInfoArray;
this would cause a binary difference if the driver code used sizeof().
Currently, the source code is calculating the struct size manually at
drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c:86
86 static uint16_t get_vce_clock_info_array_size(struct pp_hwmgr *hwmgr,
87 const ATOM_PPLIB_POWERPLAYTABLE *powerplay_table)
88 {
89 uint16_t table_offset = get_vce_clock_info_array_offset(hwmgr,
90 powerplay_table);
91 uint16_t table_size = 0;
92
93 if (table_offset > 0) {
94 const VCEClockInfoArray *p = (const VCEClockInfoArray *)
95 (((unsigned long) powerplay_table) + table_offset);
96 table_size = sizeof(uint8_t) + p->ucNumEntries * sizeof(VCEClockInfo);
97 }
98
99 return table_size;
100 }
Ricardo, please replace table_size assigment with struct_size(p, entries, p->ucNumEntries)
> >
> > typedef struct _ATOM_PPLIB_VCE_Clock_Voltage_Limit_Record
> > @@ -593,7 +597,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];
> > + __DECLARE_FLEX_ARRAY(ATOM_PPLIB_VCE_Clock_Voltage_Limit_Record, entries);
> > }ATOM_PPLIB_VCE_Clock_Voltage_Limit_Table;
similar situation as above.. needs struct_size
drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c:115
115 static uint16_t get_vce_clock_voltage_limit_table_size(struct pp_hwmgr *hwmgr,
116 const ATOM_PPLIB_POWERPLAYTABLE *powerplay_table)
117 {
118 uint16_t table_offset = get_vce_clock_voltage_limit_table_offset(hwmgr, powerplay_table);
119 uint16_t table_size = 0;
120
121 if (table_offset > 0) {
122 const ATOM_PPLIB_VCE_Clock_Voltage_Limit_Table *ptable =
123 (const ATOM_PPLIB_VCE_Clock_Voltage_Limit_Table *)(((unsigned long) powerplay_table) + table_offset);
124
125 table_size = sizeof(uint8_t) + ptable->numEntries * sizeof(ATOM_PPLIB_VCE_Clock_Voltage_Limit_Record);
126 }
127 return table_size;
128 }
> >
> > typedef struct _ATOM_PPLIB_VCE_State_Record
> > @@ -605,7 +609,7 @@ typedef struct _ATOM_PPLIB_VCE_State_Record
> > typedef struct _ATOM_PPLIB_VCE_State_Table
> > {
> > UCHAR numEntries;
> > - ATOM_PPLIB_VCE_State_Record entries[1];
> > + __DECLARE_FLEX_ARRAY(ATOM_PPLIB_VCE_State_Record, entries);
> > }ATOM_PPLIB_VCE_State_Table;
> >
> >
> >
On drivers/gpu/drm/amd/pm/legacy-dpm:420, there are many references that
are calculated manually and could possibly benefit from this
housekeeping patch about flex arrays.
In most cases, changing the struct like you did would create binary
differences but given the way that this driver was implemented, they can
slip through the cracks and make those lines more cryptic and
subsequently, less likely to be fixed in the future....
my 2 cents would be to tidy this up alongside the struct changes whether
that's in the same patch or a series of patches.
420 VCEClockInfoArray *array = (VCEClockInfoArray *)
421 (mode_info->atom_context->bios + data_offset +
422 le16_to_cpu(ext_hdr->usVCETableOffset) + 1);
423 ATOM_PPLIB_VCE_Clock_Voltage_Limit_Table *limits =
424 (ATOM_PPLIB_VCE_Clock_Voltage_Limit_Table *)
425 (mode_info->atom_context->bios + data_offset +
426 le16_to_cpu(ext_hdr->usVCETableOffset) + 1 +
427 1 + array->ucNumEntries * sizeof(VCEClockInfo));
428 ATOM_PPLIB_VCE_State_Table *states =
429 (ATOM_PPLIB_VCE_State_Table *)
430 (mode_info->atom_context->bios + data_offset +
431 le16_to_cpu(ext_hdr->usVCETableOffset) + 1 +
432 1 + (array->ucNumEntries * sizeof (VCEClockInfo)) +
433 1 + (limits->numEntries * sizeof(ATOM_PPLIB_VCE_Clock_Voltage_Limit_Record)));
> > #pragma pack()
> > --
> > 2.25.1
> >
I didn't review all struct changes but I reckon that you got the train
of thought to be followed by now. Please count on me for reviewing those
changes :-)
Thanks!
- Paulo A.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/amdgpu: replace 1-element arrays with flexible arrays
2023-07-14 10:40 ` Paulo Miguel Almeida
@ 2023-07-14 12:20 ` Ricardo Cañuelo
0 siblings, 0 replies; 5+ messages in thread
From: Ricardo Cañuelo @ 2023-07-14 12:20 UTC (permalink / raw)
To: Paulo Miguel Almeida, Alex Deucher
Cc: Gustavo A. R. Silva, alexander.deucher, kernel, linux-kernel,
amd-gfx, linux-hardening
Hi Paulo,
> I didn't review all struct changes but I reckon that you got the train
> of thought to be followed by now. Please count on me for reviewing those
> changes :-)
Thanks for reviewing the patch. It turned out that making these changes
properly isn't so trivial as I thought and I'll need to spend more time
working on it. Unfortunately I can't right now and I'll be away next
week. Since this isn't really urgent I guess I can keep working on it
after I come back. I'll take your suggestions into consideration for v2.
Cheers,
Ricardo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/amdgpu: replace 1-element arrays with flexible arrays
2023-07-12 14:12 ` Alex Deucher
2023-07-14 10:40 ` Paulo Miguel Almeida
@ 2023-07-14 18:13 ` Gustavo A. R. Silva
1 sibling, 0 replies; 5+ messages in thread
From: Gustavo A. R. Silva @ 2023-07-14 18:13 UTC (permalink / raw)
To: Alex Deucher, Ricardo Cañuelo, Gustavo A. R. Silva,
Paulo Miguel Almeida
Cc: alexander.deucher, kernel, linux-kernel, amd-gfx, linux-hardening
On 7/12/23 08:12, Alex Deucher wrote:
> On Wed, Jul 12, 2023 at 8:04 AM Ricardo Cañuelo
> <ricardo.canuelo@collabora.com> wrote:
>>
>> UBSAN complains about out-of-bounds array indexes on all 1-element
>> arrays defined on this driver:
>>
>> UBSAN: array-index-out-of-bounds in /home/rcn/work/repos/kernelci/kernelci-core/linux_kernel_mainline/drivers/gpu/drm/amd/amdgpu/../pm/powerplay/hwmgr/processpptables.c:1249:61
>>
>> Substitute them with proper flexible arrays.
>
> + Gustavo, Paulo
>
> I haven't kept up with the flexible arrays stuff. Is this equivalent
> to a zero sized array? We've been bitten by these kind of changes in
In terms of size, yes: the size of each array declaration does not
contribute to the overall size of its containing structure.
However, in these cases, using the DECLARE_FLEX_ARRAY() helper is not
required. Simply removing the '1' from the array declaration will suffice.
This helper was created to declare flex-array members in unions, as well
as in structs that contain no other members aside from the array.
In any case, these changes are not complete, as they're only modifying
the struct declaration, hence the size of the struct is affected. Now
the rest of the code where these structs are involved should be audited
and adjusted to accommodate the change in the sizes of the structs.
> the past. These structures define the layout of data in a rom image
> on the board. If the struct size changes, that could lead to errors
> in the code that deals with these structures.
>
> Alex
>
Thanks
--
Gustavo
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-07-14 18:13 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-12 8:31 [PATCH] drm/amdgpu: replace 1-element arrays with flexible arrays Ricardo Cañuelo
2023-07-12 14:12 ` Alex Deucher
2023-07-14 10:40 ` Paulo Miguel Almeida
2023-07-14 12:20 ` Ricardo Cañuelo
2023-07-14 18:13 ` Gustavo A. R. Silva
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox