Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Expanding fixed-size arrays in packed HFI structures silently shifts the offsets of subsequent fields, breaking communication with older GMU firmwares. Pre-existing issues: - [Critical] Out-of-bounds stack write in `a6xx_hfi_send_perf_table` and `a6xx_hfi_send_perf_table_v1` due to a mismatch between `GMU_MAX_CX_FREQS` and the `cx_votes` array size. -- commit b7dcd380638139f2d4f6982c947797f34b9ff256 Author: Akhil P Oommen <[email protected]> drm/msm/a6xx: Increase HFI GX perf level limit to 24 Newer Adreno GPUs expose more than 16 GX frequency levels. Introduce HFI_MAX_GX_LEVELS and use it to size the perf-table and bandwidth-table GX vote arrays, and to derive GMU_MAX_GX_FREQS, so these levels are not truncated. > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h > b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h > index 3f96b10b5f61a..62e3fb230cd3a 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h > @@ -20,7 +20,7 @@ struct a6xx_gmu_bo { > u64 iova; > }; > > -#define GMU_MAX_GX_FREQS 32 > +#define GMU_MAX_GX_FREQS HFI_MAX_GX_LEVELS > #define GMU_MAX_CX_FREQS 6 > #define GMU_MAX_BCMS 3 > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_hfi.h > b/drivers/gpu/drm/msm/adreno/a6xx_hfi.h > index e10d32ce93e0c..f4da73a555c65 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_hfi.h > +++ b/drivers/gpu/drm/msm/adreno/a6xx_hfi.h > @@ -5,6 +5,7 @@ > #define _A6XX_HFI_H_ > > #define HFI_MAX_QUEUES 3 > +#define HFI_MAX_GX_LEVELS 24 > > struct a6xx_hfi_queue_table_header { > u32 version; [ ... ] > @@ -119,7 +120,7 @@ struct a6xx_hfi_msg_perf_table_v1 { > u32 num_gpu_levels; > u32 num_gmu_levels; > > - struct perf_level gx_votes[16]; > + struct perf_level gx_votes[HFI_MAX_GX_LEVELS]; > struct perf_level cx_votes[4]; [Severity: High] Does expanding this fixed-size array in a packed structure break compatibility with older GMU firmwares? Because a6xx_hfi_msg_perf_table_v1 is a packed structure, increasing the size of gx_votes from 16 to 24 elements shifts the offset of cx_votes. During driver initialization on older Adreno GPUs, firmwares expecting the previous fixed-size v1 performance tables might read cx_votes at the old offset and receive uninitialized zeros instead of the actual frequencies, which can cause system instability. This same offset shift appears to happen below in struct a6xx_hfi_msg_perf_table as well. [Severity: Critical] This is a pre-existing issue, but can this cause a stack buffer overflow? The cx_votes array is hardcoded to a size of 4 here. However, in drivers/gpu/drm/msm/adreno/a6xx_gmu.h, GMU_MAX_CX_FREQS is defined as 6. During initialization, a6xx_hfi_send_perf_table_v1() iterates up to gmu->nr_gmu_freqs (which can be up to 6 on newer platforms with more OPPs) and writes to msg.cx_votes[i] on the stack: drivers/gpu/drm/msm/adreno/a6xx_hfi.c:a6xx_hfi_send_perf_table_v1() { ... for (i = 0; i < gmu->nr_gmu_freqs; i++) { msg.cx_votes[i].vote = gmu->cx_arc_votes[i]; msg.cx_votes[i].freq = gmu->gmu_freqs[i] / 1000; } ... } If gmu->nr_gmu_freqs exceeds 4, this loop appears to write out-of-bounds on the stack, which can corrupt memory. This identical loop is also present in a6xx_hfi_send_perf_table(). > } __packed; > > @@ -128,7 +129,7 @@ struct a6xx_hfi_msg_perf_table { > u32 num_gpu_levels; > u32 num_gmu_levels; > > - struct perf_gx_level gx_votes[16]; > + struct perf_gx_level gx_votes[HFI_MAX_GX_LEVELS]; > struct perf_level cx_votes[4]; > } __packed; > > @@ -144,7 +145,7 @@ struct a6xx_hfi_msg_bw_table { > u32 cnoc_cmds_addrs[6]; > u32 cnoc_cmds_data[2][6]; > u32 ddr_cmds_addrs[8]; > - u32 ddr_cmds_data[16][8]; > + u32 ddr_cmds_data[HFI_MAX_GX_LEVELS][8]; [Severity: High] Does increasing the size of ddr_cmds_data risk breaking length validation on older firmwares? Expanding this array increases the overall message size by 256 bytes, which older GMU firmwares might reject if they strictly validate the length of the bandwidth table message. > } __packed; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
