On Wed, Aug 18, 2021 at 05:12:28PM +0530, Lazar, Lijo wrote:
On 8/18/2021 11:34 AM, Kees Cook wrote:
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() in structs:
struct atom_smc_dpm_info_v4_5
struct atom_smc_dpm_info_v4_6
struct atom_smc_dpm_info_v4_7
struct atom_smc_dpm_info_v4_10
PPTable_t
so the grouped members can be referenced together. This will allow
memcpy() and sizeof() to more easily reason about sizes, improve
readability, and avoid future warnings about writing beyond the end of
the first member.
"pahole" shows no size nor member offset changes to any structs.
"objdump -d" shows no object code changes.
Cc: "Christian König" <christian.koenig@xxxxxxx>
Cc: "Pan, Xinhui" <Xinhui.Pan@xxxxxxx>
Cc: David Airlie <airlied@xxxxxxxx>
Cc: Daniel Vetter <daniel@xxxxxxxx>
Cc: Hawking Zhang <Hawking.Zhang@xxxxxxx>
Cc: Feifei Xu <Feifei.Xu@xxxxxxx>
Cc: Lijo Lazar <lijo.lazar@xxxxxxx>
Cc: Likun Gao <Likun.Gao@xxxxxxx>
Cc: Jiawei Gu <Jiawei.Gu@xxxxxxx>
Cc: Evan Quan <evan.quan@xxxxxxx>
Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
Acked-by: Alex Deucher <alexander.deucher@xxxxxxx>
Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2FCADnq5_Npb8uYvd%2BR4UHgf-w8-cQj3JoODjviJR_Y9w9wqJ71mQ%40mail.gmail.com&data=04%7C01%7Clijo.lazar%40amd.com%7C3861f20094074bf7328808d962a433f2%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637649279701053991%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=386LcfJJGfQfHsXBuK17LMqxJ2nFtGoj%2FUjoN2ZtJd0%3D&reserved=0
---
drivers/gpu/drm/amd/include/atomfirmware.h | 9 ++++++++-
.../gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h | 3 ++-
drivers/gpu/drm/amd/pm/inc/smu11_driver_if_navi10.h | 3 ++-
.../gpu/drm/amd/pm/inc/smu13_driver_if_aldebaran.h | 3 ++-
Hi Kees,
Hi! Thanks for looking into this.
The headers which define these structs are firmware/VBIOS interfaces and are
picked directly from those components. There are difficulties in grouping
them to structs at the original source as that involves other component
changes.
So, can you help me understand this a bit more? It sounds like these are
generated headers, yes? I'd like to understand your constraints and
weight them against various benefits that could be achieved here.
The groupings I made do appear to be roughly documented already,
for example:
struct atom_common_table_header table_header;
// SECTION: BOARD PARAMETERS
+ struct_group(dpm_info,
Something emitted the "BOARD PARAMETERS" section heading as a comment,
so it likely also would know where it ends, yes? The good news here is
that for the dpm_info groups, they all end at the end of the existing
structs, see:
struct atom_smc_dpm_info_v4_5
struct atom_smc_dpm_info_v4_6
struct atom_smc_dpm_info_v4_7
struct atom_smc_dpm_info_v4_10
The matching regions in the PPTable_t structs are similarly marked with a
"BOARD PARAMETERS" section heading comment:
--- a/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h
+++ b/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h
@@ -643,6 +643,7 @@ typedef struct {
// SECTION: BOARD PARAMETERS
// SVI2 Board Parameters
+ struct_group(v4_6,
uint16_t MaxVoltageStepGfx; // In mV(Q2) Max voltage step that SMU will request. Multiple steps are taken if voltage change exceeds this value.
uint16_t MaxVoltageStepSoc; // In mV(Q2) Max voltage step that SMU will request. Multiple steps are taken if voltage change exceeds this value.
@@ -728,10 +729,10 @@ typedef struct {
uint32_t BoardVoltageCoeffB; // decode by /1000
uint32_t BoardReserved[7];
+ );
// Padding for MMHUB - do not modify this
uint32_t MmHubPadding[8]; // SMU internal use
-
} PPTable_t;
Where they end seems known as well (the padding switches from a "Board"
to "MmHub" prefix at exactly the matching size).
So, given that these regions are already known by the export tool, how
about just updating the export tool to emit a struct there? I imagine
the problem with this would be the identifier churn needed, but that's
entirely mechanical.
However, I'm curious about another aspect of these regions: they are,
by definition, the same. Why isn't there a single struct describing
them already, given the existing redundancy? For example, look at the
member names: maxvoltagestepgfx vs MaxVoltageStepGfx. Why aren't these
the same? And then why aren't they described separately?
Fixing that would cut down on the redundancy here, and in the renaming,
you can fix the identifiers as well. It should be straight forward to
write a Coccinelle script to do this renaming for you after extracting
the structure.
The driver_if_* files updates are frequent and it is error prone to manually
group them each time we pick them for any update.
Why are these structs updated? It looks like they're specifically
versioned, and aren't expected to change (i.e. v4.5, v4.6, v4.10, etc).
Our usage of memcpy in this way is restricted only to a very few places.
True, there's 1 per PPTable_t duplication. With a proper struct, you
wouldn't even need a memcpy().
Instead of the existing:
memcpy(smc_pptable->I2cControllers, smc_dpm_table_v4_7->I2cControllers,
sizeof(*smc_dpm_table_v4_7) - sizeof(smc_dpm_table_v4_7->table_header));
or my proposed:
memcpy(&smc_pptable->v4, &smc_dpm_table_v4_7->dpm_info,
sizeof(smc_dpm_table_v4_7->dpm_info));
you could just have:
smc_pptable->v4 = smc_dpm_table_v4_7->dpm_info;
since they'd be explicitly the same type.
That looks like a much cleaner solution to this. It greatly improves
readability, reduces the redundancy in the headers, and should be a
simple mechanical refactoring.
Oh my, I just noticed append_vbios_pptable() in
drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega12_processpptables.c
which does an open-coded assignment of the entire PPTable_t, including
padding, and, apparently, the i2c address twice:
ppsmc_pptable->Vr2_I2C_address = smc_dpm_table.Vr2_I2C_address;
ppsmc_pptable->Vr2_I2C_address = smc_dpm_table.Vr2_I2C_address;
As another option - is it possible to have a helper function/macro like
memcpy_fortify() which takes the extra arguments and does the extra compile
time checks? We will use the helper whenever we have such kind of usage.
I'd rather avoid special cases just for this, especially when the code
here is already doing a couple things we try to avoid in the rest of
the kernel (i.e. open coded redundant struct contents, etc).
If something mechanically produced append_vbios_pptable() above, I bet
we can get rid of the memcpy()s entirely and save a lot of code doing a
member-to-member assignment.
What do you think?
