lkml.org 
[lkml]   [2021]   [Aug]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v3] drm/amd/pm: And destination bounds checking to struct copy
    Applied.  Thanks!

    Alex

    On Thu, Aug 26, 2021 at 11:16 PM Kees Cook <keescook@chromium.org> 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.
    >
    > The "Board Parameters" members of the 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
    > are written to the corresponding members of the corresponding PPTable_t
    > variables, but they lack destination size bounds checking, which means
    > the compiler cannot verify at compile time that this is an intended and
    > safe memcpy().
    >
    > Since the header files are effectively immutable[1] and a struct_group()
    > cannot be used, nor a common struct referenced by both sides of the
    > memcpy() arguments, add a new helper, amdgpu_memcpy_trailing(), to
    > perform the bounds checking at compile time. Replace the open-coded
    > memcpy()s with amdgpu_memcpy_trailing() which includes enough context
    > for the bounds checking.
    >
    > "objdump -d" shows no object code changes.
    >
    > [1] https://lore.kernel.org/lkml/e56aad3c-a06f-da07-f491-a894a570d78f@amd.com
    >
    > Cc: "Christian König" <christian.koenig@amd.com>
    > Cc: "Pan, Xinhui" <Xinhui.Pan@amd.com>
    > Cc: David Airlie <airlied@linux.ie>
    > Cc: Daniel Vetter <daniel@ffwll.ch>
    > Cc: Hawking Zhang <Hawking.Zhang@amd.com>
    > Cc: Feifei Xu <Feifei.Xu@amd.com>
    > Cc: Likun Gao <Likun.Gao@amd.com>
    > Cc: Jiawei Gu <Jiawei.Gu@amd.com>
    > Cc: Evan Quan <evan.quan@amd.com>
    > Cc: amd-gfx@lists.freedesktop.org
    > Cc: dri-devel@lists.freedesktop.org
    > Reviewed-by: Lijo Lazar <lijo.lazar@amd.com>
    > Acked-by: Alex Deucher <alexander.deucher@amd.com>
    > Signed-off-by: Kees Cook <keescook@chromium.org>
    > ---
    > v3: rename amdgpu_memcpy_trailing() to smu_memcpy_trailing()
    > v2: https://lore.kernel.org/lkml/20210825161957.3904130-1-keescook@chromium.org
    > v1: https://lore.kernel.org/lkml/20210819201441.3545027-1-keescook@chromium.org
    > ---
    > drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h | 24 +++++++++++++++++++
    > .../gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c | 6 ++---
    > .../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 8 +++----
    > .../drm/amd/pm/swsmu/smu13/aldebaran_ppt.c | 5 ++--
    > 4 files changed, 32 insertions(+), 11 deletions(-)
    >
    > diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
    > index 715b4225f5ee..8156729c370b 100644
    > --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
    > +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
    > @@ -1335,6 +1335,30 @@ enum smu_cmn2asic_mapping_type {
    > #define WORKLOAD_MAP(profile, workload) \
    > [profile] = {1, (workload)}
    >
    > +/**
    > + * smu_memcpy_trailing - Copy the end of one structure into the middle of another
    > + *
    > + * @dst: Pointer to destination struct
    > + * @first_dst_member: The member name in @dst where the overwrite begins
    > + * @last_dst_member: The member name in @dst where the overwrite ends after
    > + * @src: Pointer to the source struct
    > + * @first_src_member: The member name in @src where the copy begins
    > + *
    > + */
    > +#define smu_memcpy_trailing(dst, first_dst_member, last_dst_member, \
    > + src, first_src_member) \
    > +({ \
    > + size_t __src_offset = offsetof(typeof(*(src)), first_src_member); \
    > + size_t __src_size = sizeof(*(src)) - __src_offset; \
    > + size_t __dst_offset = offsetof(typeof(*(dst)), first_dst_member); \
    > + size_t __dst_size = offsetofend(typeof(*(dst)), last_dst_member) - \
    > + __dst_offset; \
    > + BUILD_BUG_ON(__src_size != __dst_size); \
    > + __builtin_memcpy((u8 *)(dst) + __dst_offset, \
    > + (u8 *)(src) + __src_offset, \
    > + __dst_size); \
    > +})
    > +
    > #if !defined(SWSMU_CODE_LAYER_L2) && !defined(SWSMU_CODE_LAYER_L3) && !defined(SWSMU_CODE_LAYER_L4)
    > int smu_get_power_limit(void *handle,
    > uint32_t *limit,
    > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
    > index 273df66cac14..e343cc218990 100644
    > --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
    > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
    > @@ -483,10 +483,8 @@ static int arcturus_append_powerplay_table(struct smu_context *smu)
    >
    > if ((smc_dpm_table->table_header.format_revision == 4) &&
    > (smc_dpm_table->table_header.content_revision == 6))
    > - memcpy(&smc_pptable->MaxVoltageStepGfx,
    > - &smc_dpm_table->maxvoltagestepgfx,
    > - sizeof(*smc_dpm_table) - offsetof(struct atom_smc_dpm_info_v4_6, maxvoltagestepgfx));
    > -
    > + smu_memcpy_trailing(smc_pptable, MaxVoltageStepGfx, BoardReserved,
    > + smc_dpm_table, maxvoltagestepgfx);
    > return 0;
    > }
    >
    > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
    > index f96681700c41..a5fc5d7cb6c7 100644
    > --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
    > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
    > @@ -431,16 +431,16 @@ static int navi10_append_powerplay_table(struct smu_context *smu)
    >
    > switch (smc_dpm_table->table_header.content_revision) {
    > case 5: /* nv10 and nv14 */
    > - memcpy(smc_pptable->I2cControllers, smc_dpm_table->I2cControllers,
    > - sizeof(*smc_dpm_table) - sizeof(smc_dpm_table->table_header));
    > + smu_memcpy_trailing(smc_pptable, I2cControllers, BoardReserved,
    > + smc_dpm_table, I2cControllers);
    > break;
    > case 7: /* nv12 */
    > ret = amdgpu_atombios_get_data_table(adev, index, NULL, NULL, NULL,
    > (uint8_t **)&smc_dpm_table_v4_7);
    > if (ret)
    > return ret;
    > - memcpy(smc_pptable->I2cControllers, smc_dpm_table_v4_7->I2cControllers,
    > - sizeof(*smc_dpm_table_v4_7) - sizeof(smc_dpm_table_v4_7->table_header));
    > + smu_memcpy_trailing(smc_pptable, I2cControllers, BoardReserved,
    > + smc_dpm_table_v4_7, I2cControllers);
    > break;
    > default:
    > dev_err(smu->adev->dev, "smc_dpm_info with unsupported content revision %d!\n",
    > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
    > index ec8c30daf31c..ab652028e003 100644
    > --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
    > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
    > @@ -409,9 +409,8 @@ static int aldebaran_append_powerplay_table(struct smu_context *smu)
    >
    > if ((smc_dpm_table->table_header.format_revision == 4) &&
    > (smc_dpm_table->table_header.content_revision == 10))
    > - memcpy(&smc_pptable->GfxMaxCurrent,
    > - &smc_dpm_table->GfxMaxCurrent,
    > - sizeof(*smc_dpm_table) - offsetof(struct atom_smc_dpm_info_v4_10, GfxMaxCurrent));
    > + smu_memcpy_trailing(smc_pptable, GfxMaxCurrent, reserved,
    > + smc_dpm_table, GfxMaxCurrent);
    > return 0;
    > }
    >
    > --
    > 2.30.2
    >

    \
     
     \ /
      Last update: 2021-08-27 20:23    [W:5.704 / U:1.236 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site