* [PATCH v2] drm/amd/pm: And destination bounds checking to struct copy
@ 2021-08-25 16:19 Kees Cook
2021-08-26 19:51 ` Alex Deucher
0 siblings, 1 reply; 3+ messages in thread
From: Kees Cook @ 2021-08-25 16:19 UTC (permalink / raw)
To: Christian König
Cc: Kees Cook, Pan, Xinhui, David Airlie, Daniel Vetter,
Hawking Zhang, Feifei Xu, Likun Gao, Jiawei Gu, Evan Quan,
amd-gfx, dri-devel, Lijo Lazar, Alex Deucher, Andrey Grodzovsky,
Luben Tuikov, Sathishkumar S, Jonathan Kim, Darren Powell,
Huang Rui, Xiaojian Du, Ryan Taylor, Graham Sider, Kevin Wang,
David M Nieto, Lee Jones, John Clements, linux-kernel,
linux-hardening
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>
---
v2:
- rename and move helper to drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
- add reviews/acks
v1: https://lore.kernel.org/lkml/20210819201441.3545027-1-keescook@chromium.org/
---
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 +
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 ++--
5 files changed, 33 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index dc3c6b3a00e5..c911387045e2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1452,4 +1452,5 @@ static inline int amdgpu_in_reset(struct amdgpu_device *adev)
{
return atomic_read(&adev->in_gpu_reset);
}
+
#endif
diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
index 715b4225f5ee..29031eb11d39 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)}
+/**
+ * amdgpu_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 amdgpu_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..bda8fc12c91f 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));
-
+ amdgpu_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..88a4a2aed48e 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));
+ amdgpu_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));
+ amdgpu_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..d46b892846f6 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));
+ amdgpu_memcpy_trailing(smc_pptable, GfxMaxCurrent, reserved,
+ smc_dpm_table, GfxMaxCurrent);
return 0;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] drm/amd/pm: And destination bounds checking to struct copy
2021-08-25 16:19 [PATCH v2] drm/amd/pm: And destination bounds checking to struct copy Kees Cook
@ 2021-08-26 19:51 ` Alex Deucher
2021-08-27 3:09 ` Kees Cook
0 siblings, 1 reply; 3+ messages in thread
From: Alex Deucher @ 2021-08-26 19:51 UTC (permalink / raw)
To: Kees Cook
Cc: Christian König, Pan, Xinhui, David Airlie, Daniel Vetter,
Hawking Zhang, Feifei Xu, Likun Gao, Jiawei Gu, Evan Quan,
amd-gfx list, Maling list - DRI developers, Lijo Lazar,
Alex Deucher, Andrey Grodzovsky, Luben Tuikov, Sathishkumar S,
Jonathan Kim, Darren Powell, Huang Rui, Xiaojian Du, Ryan Taylor,
Graham Sider, Kevin Wang, David M Nieto, Lee Jones,
John Clements, LKML, linux-hardening
On Wed, Aug 25, 2021 at 12:20 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>
> ---
> v2:
> - rename and move helper to drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> - add reviews/acks
> v1: https://lore.kernel.org/lkml/20210819201441.3545027-1-keescook@chromium.org/
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 +
> 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 ++--
> 5 files changed, 33 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index dc3c6b3a00e5..c911387045e2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1452,4 +1452,5 @@ static inline int amdgpu_in_reset(struct amdgpu_device *adev)
> {
> return atomic_read(&adev->in_gpu_reset);
> }
> +
> #endif
> diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> index 715b4225f5ee..29031eb11d39 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)}
>
> +/**
> + * amdgpu_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 amdgpu_memcpy_trailing(dst, first_dst_member, last_dst_member, \
I would change this to smu_memcpy_trailing() for consistency. Other
than that, it the patch looks good to me. Did you want me to pick
this up or do you want to keep this with the other patches in your
series?
Thanks!
Alex
> + 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..bda8fc12c91f 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));
> -
> + amdgpu_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..88a4a2aed48e 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));
> + amdgpu_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));
> + amdgpu_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..d46b892846f6 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));
> + amdgpu_memcpy_trailing(smc_pptable, GfxMaxCurrent, reserved,
> + smc_dpm_table, GfxMaxCurrent);
> return 0;
> }
>
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] drm/amd/pm: And destination bounds checking to struct copy
2021-08-26 19:51 ` Alex Deucher
@ 2021-08-27 3:09 ` Kees Cook
0 siblings, 0 replies; 3+ messages in thread
From: Kees Cook @ 2021-08-27 3:09 UTC (permalink / raw)
To: Alex Deucher
Cc: Christian König, Pan, Xinhui, David Airlie, Daniel Vetter,
Hawking Zhang, Feifei Xu, Likun Gao, Jiawei Gu, Evan Quan,
amd-gfx list, Maling list - DRI developers, Lijo Lazar,
Alex Deucher, Andrey Grodzovsky, Luben Tuikov, Sathishkumar S,
Jonathan Kim, Darren Powell, Huang Rui, Xiaojian Du, Ryan Taylor,
Graham Sider, Kevin Wang, David M Nieto, Lee Jones,
John Clements, LKML, linux-hardening
On Thu, Aug 26, 2021 at 03:51:29PM -0400, Alex Deucher wrote:
> On Wed, Aug 25, 2021 at 12:20 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>
> > ---
> > v2:
> > - rename and move helper to drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > - add reviews/acks
> > v1: https://lore.kernel.org/lkml/20210819201441.3545027-1-keescook@chromium.org/
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 +
> > 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 ++--
> > 5 files changed, 33 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > index dc3c6b3a00e5..c911387045e2 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > @@ -1452,4 +1452,5 @@ static inline int amdgpu_in_reset(struct amdgpu_device *adev)
> > {
> > return atomic_read(&adev->in_gpu_reset);
> > }
> > +
> > #endif
> > diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > index 715b4225f5ee..29031eb11d39 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)}
> >
> > +/**
> > + * amdgpu_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 amdgpu_memcpy_trailing(dst, first_dst_member, last_dst_member, \
>
> I would change this to smu_memcpy_trailing() for consistency. Other
Sure; I will send a v3.
> than that, it the patch looks good to me. Did you want me to pick
> this up or do you want to keep this with the other patches in your
> series?
Since this has no external dependencies, it's probably best to go via
your tree.
Thanks!
-Kees
>
> Thanks!
>
> Alex
>
> > + 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..bda8fc12c91f 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));
> > -
> > + amdgpu_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..88a4a2aed48e 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));
> > + amdgpu_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));
> > + amdgpu_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..d46b892846f6 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));
> > + amdgpu_memcpy_trailing(smc_pptable, GfxMaxCurrent, reserved,
> > + smc_dpm_table, GfxMaxCurrent);
> > return 0;
> > }
> >
> > --
> > 2.30.2
> >
--
Kees Cook
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-08-27 3:10 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-25 16:19 [PATCH v2] drm/amd/pm: And destination bounds checking to struct copy Kees Cook
2021-08-26 19:51 ` Alex Deucher
2021-08-27 3:09 ` Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).