* Re: [PATCH] drm/amd/pm: And destination bounds checking to struct copy
2021-08-19 20:14 [PATCH] drm/amd/pm: And destination bounds checking to struct copy Kees Cook
@ 2021-08-20 15:27 ` Alex Deucher
2021-08-23 5:36 ` Lazar, Lijo
2021-08-23 6:28 ` Christian König
2 siblings, 0 replies; 7+ messages in thread
From: Alex Deucher @ 2021-08-20 15:27 UTC (permalink / raw)
To: Kees Cook
Cc: Lijo Lazar, 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,
Alex Deucher, Luben Tuikov, Andrey Grodzovsky, Dennis Li,
Sathishkumar S, Jonathan Kim, Kevin Wang, David M Nieto,
Kenneth Feng, Lee Jones, John Clements, LKML, linux-hardening
On Thu, Aug 19, 2021 at 4:14 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, memcpy_trailing(), to perform the
> bounds checking at compile time. Replace the open-coded memcpy()s with
> 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: Lijo Lazar <lijo.lazar@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
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Link: https://lore.kernel.org/lkml/CADnq5_Npb8uYvd+R4UHgf-w8-cQj3JoODjviJR_Y9w9wqJ71mQ@mail.gmail.com
> ---
> Alex, I dropped your prior Acked-by, since the implementation is very
> different. If you're still happy with it, I can add it back. :)
This looks reasonable to me:
Acked-by: Alex Deucher <alexander.deucher@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 25 +++++++++++++++++++
> .../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, 33 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 96e895d6be35..4605934a4fb7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1446,4 +1446,29 @@ static inline int amdgpu_in_reset(struct amdgpu_device *adev)
> {
> return atomic_read(&adev->in_gpu_reset);
> }
> +
> +/**
> + * 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 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); \
> +})
> +
> #endif
> 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 8ab58781ae13..1918e6232319 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
> @@ -465,10 +465,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));
> -
> + 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 2e5d3669652b..b738042e064d 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));
> + 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));
> + 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 c8eefacfdd37..a6fd7ee314a9 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));
> + memcpy_trailing(smc_pptable, GfxMaxCurrent, reserved,
> + smc_dpm_table, GfxMaxCurrent);
> return 0;
> }
>
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/amd/pm: And destination bounds checking to struct copy
2021-08-19 20:14 [PATCH] drm/amd/pm: And destination bounds checking to struct copy Kees Cook
2021-08-20 15:27 ` Alex Deucher
@ 2021-08-23 5:36 ` Lazar, Lijo
2021-08-23 6:28 ` Christian König
2 siblings, 0 replies; 7+ messages in thread
From: Lazar, Lijo @ 2021-08-23 5:36 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, dri-devel, Alex Deucher, Luben Tuikov,
Andrey Grodzovsky, Dennis Li, Sathishkumar S, Jonathan Kim,
Kevin Wang, David M Nieto, Kenneth Feng, Lee Jones,
John Clements, linux-kernel, linux-hardening
Thanks Kees!
Reviewed-by: Lijo Lazar <lijo.lazar@amd.com>
Thanks,
Lijo
On 8/20/2021 1:44 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.
>
> 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, memcpy_trailing(), to perform the
> bounds checking at compile time. Replace the open-coded memcpy()s with
> memcpy_trailing() which includes enough context for the bounds checking.
>
> "objdump -d" shows no object code changes.
>
> [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2Fe56aad3c-a06f-da07-f491-a894a570d78f%40amd.com&data=04%7C01%7Clijo.lazar%40amd.com%7Cb0567a0252604c8c84cd08d9634dfe39%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637650008892964983%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=UikQRh9WCwr8H29bBqZu3iLJt095Es9mIG5mVwPJpC0%3D&reserved=0
>
> Cc: Lijo Lazar <lijo.lazar@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
> Signed-off-by: Kees Cook <keescook@chromium.org>
> 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%7Cb0567a0252604c8c84cd08d9634dfe39%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637650008892964983%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=LVHdf9C1jx6eIB2BQ%2Bm5q4o5KcRzBWJi7PhNviKzKGM%3D&reserved=0
> ---
> Alex, I dropped your prior Acked-by, since the implementation is very
> different. If you're still happy with it, I can add it back. :)
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 25 +++++++++++++++++++
> .../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, 33 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 96e895d6be35..4605934a4fb7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1446,4 +1446,29 @@ static inline int amdgpu_in_reset(struct amdgpu_device *adev)
> {
> return atomic_read(&adev->in_gpu_reset);
> }
> +
> +/**
> + * 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 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); \
> +})
> +
> #endif
> 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 8ab58781ae13..1918e6232319 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
> @@ -465,10 +465,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));
> -
> + 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 2e5d3669652b..b738042e064d 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));
> + 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));
> + 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 c8eefacfdd37..a6fd7ee314a9 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));
> + memcpy_trailing(smc_pptable, GfxMaxCurrent, reserved,
> + smc_dpm_table, GfxMaxCurrent);
> return 0;
> }
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/amd/pm: And destination bounds checking to struct copy
2021-08-19 20:14 [PATCH] drm/amd/pm: And destination bounds checking to struct copy Kees Cook
2021-08-20 15:27 ` Alex Deucher
2021-08-23 5:36 ` Lazar, Lijo
@ 2021-08-23 6:28 ` Christian König
2021-08-23 14:23 ` Kees Cook
2 siblings, 1 reply; 7+ messages in thread
From: Christian König @ 2021-08-23 6:28 UTC (permalink / raw)
To: Kees Cook, Lijo Lazar
Cc: Pan, Xinhui, David Airlie, Daniel Vetter, Hawking Zhang,
Feifei Xu, Likun Gao, Jiawei Gu, Evan Quan, amd-gfx, dri-devel,
Alex Deucher, Luben Tuikov, Andrey Grodzovsky, Dennis Li,
Sathishkumar S, Jonathan Kim, Kevin Wang, David M Nieto,
Kenneth Feng, Lee Jones, John Clements, linux-kernel,
linux-hardening
Am 19.08.21 um 22:14 schrieb Kees Cook:
> 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, memcpy_trailing(), to perform the
> bounds checking at compile time. Replace the open-coded memcpy()s with
> memcpy_trailing() which includes enough context for the bounds checking.
>
> "objdump -d" shows no object code changes.
>
> [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2Fe56aad3c-a06f-da07-f491-a894a570d78f%40amd.com&data=04%7C01%7Cchristian.koenig%40amd.com%7C63c17764a7c84cc85d7a08d9634dfe37%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637650008924776466%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=TbxGZSLP0HegTcF4qFn0cnFW8SR47X0wmuf1y75ygFw%3D&reserved=0
>
> Cc: Lijo Lazar <lijo.lazar@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
> Signed-off-by: Kees Cook <keescook@chromium.org>
> 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%7Cchristian.koenig%40amd.com%7C63c17764a7c84cc85d7a08d9634dfe37%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637650008924786462%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=yCGfiaYVLayZKc9Ahq1axpjztJQ8KVIJ4tMI6Z7LoMQ%3D&reserved=0
> ---
> Alex, I dropped your prior Acked-by, since the implementation is very
> different. If you're still happy with it, I can add it back. :)
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 25 +++++++++++++++++++
> .../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, 33 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 96e895d6be35..4605934a4fb7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1446,4 +1446,29 @@ static inline int amdgpu_in_reset(struct amdgpu_device *adev)
> {
> return atomic_read(&adev->in_gpu_reset);
> }
> +
> +/**
> + * 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 memcpy_trailing(dst, first_dst_member, last_dst_member, \
> + src, first_src_member) \
Please don't add a function like this into amdgpu.h, especially when it
is only used by the SMU code.
And please give it an amdgpu_ prefix so that we are not confusing it
with a core function.
Apart from that looks good to me.
Thanks,
Christian.
> +({ \
> + 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); \
> +})
> +
> #endif
> 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 8ab58781ae13..1918e6232319 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
> @@ -465,10 +465,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));
> -
> + 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 2e5d3669652b..b738042e064d 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));
> + 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));
> + 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 c8eefacfdd37..a6fd7ee314a9 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));
> + memcpy_trailing(smc_pptable, GfxMaxCurrent, reserved,
> + smc_dpm_table, GfxMaxCurrent);
> return 0;
> }
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/amd/pm: And destination bounds checking to struct copy
2021-08-23 6:28 ` Christian König
@ 2021-08-23 14:23 ` Kees Cook
2021-08-23 19:01 ` Christian König
0 siblings, 1 reply; 7+ messages in thread
From: Kees Cook @ 2021-08-23 14:23 UTC (permalink / raw)
To: Christian König, Lijo Lazar
Cc: Pan, Xinhui, David Airlie, Daniel Vetter, Hawking Zhang,
Feifei Xu, Likun Gao, Jiawei Gu, Evan Quan, amd-gfx, dri-devel,
Alex Deucher, Luben Tuikov, Andrey Grodzovsky, Dennis Li,
Sathishkumar S, Jonathan Kim, Kevin Wang, David M Nieto,
Kenneth Feng, Lee Jones, John Clements, linux-kernel,
linux-hardening
On August 22, 2021 11:28:54 PM PDT, "Christian König" <christian.koenig@amd.com> wrote:
>
>
>Am 19.08.21 um 22:14 schrieb Kees Cook:
>> [...]
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> index 96e895d6be35..4605934a4fb7 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> @@ -1446,4 +1446,29 @@ static inline int amdgpu_in_reset(struct amdgpu_device *adev)
>> {
>> return atomic_read(&adev->in_gpu_reset);
>> }
>> +
>> +/**
>> + * 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 memcpy_trailing(dst, first_dst_member, last_dst_member, \
>> + src, first_src_member) \
>
>Please don't add a function like this into amdgpu.h, especially when it
>is only used by the SMU code.
Sure, I'm happy to move it. It wasn't clear to me which headers were considered "immutable". Which header should I put this in?
>And please give it an amdgpu_ prefix so that we are not confusing it
>with a core function.
Sure, I will include that.
>Apart from that looks good to me.
Thanks!
-Kees
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/amd/pm: And destination bounds checking to struct copy
2021-08-23 14:23 ` Kees Cook
@ 2021-08-23 19:01 ` Christian König
2021-08-23 19:13 ` Deucher, Alexander
0 siblings, 1 reply; 7+ messages in thread
From: Christian König @ 2021-08-23 19:01 UTC (permalink / raw)
To: Kees Cook, Lijo Lazar
Cc: Pan, Xinhui, David Airlie, Daniel Vetter, Hawking Zhang,
Feifei Xu, Likun Gao, Jiawei Gu, Evan Quan, amd-gfx, dri-devel,
Alex Deucher, Luben Tuikov, Andrey Grodzovsky, Dennis Li,
Sathishkumar S, Jonathan Kim, Kevin Wang, David M Nieto,
Kenneth Feng, Lee Jones, John Clements, linux-kernel,
linux-hardening
Am 23.08.21 um 16:23 schrieb Kees Cook:
>
> On August 22, 2021 11:28:54 PM PDT, "Christian König" <christian.koenig@amd.com> wrote:
>>
>> Am 19.08.21 um 22:14 schrieb Kees Cook:
>>> [...]
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> index 96e895d6be35..4605934a4fb7 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> @@ -1446,4 +1446,29 @@ static inline int amdgpu_in_reset(struct amdgpu_device *adev)
>>> {
>>> return atomic_read(&adev->in_gpu_reset);
>>> }
>>> +
>>> +/**
>>> + * 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 memcpy_trailing(dst, first_dst_member, last_dst_member, \
>>> + src, first_src_member) \
>> Please don't add a function like this into amdgpu.h, especially when it
>> is only used by the SMU code.
> Sure, I'm happy to move it. It wasn't clear to me which headers were considered "immutable". Which header should I put this in?
I think amdgpu_smuio.h, but I'm not 100% sure. Alex do you have a better
idea?
We don't want to put anything new into amdgpu.h any more since this is
basically only a legacy leftover.
Thanks,
Christian.
>
>> And please give it an amdgpu_ prefix so that we are not confusing it
>> with a core function.
> Sure, I will include that.
>
>> Apart from that looks good to me.
> Thanks!
>
> -Kees
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] drm/amd/pm: And destination bounds checking to struct copy
2021-08-23 19:01 ` Christian König
@ 2021-08-23 19:13 ` Deucher, Alexander
0 siblings, 0 replies; 7+ messages in thread
From: Deucher, Alexander @ 2021-08-23 19:13 UTC (permalink / raw)
To: Koenig, Christian, Kees Cook, Lazar, Lijo
Cc: Pan, Xinhui, David Airlie, Daniel Vetter, Zhang, Hawking, Xu,
Feifei, Gao, Likun, Gu, JiaWei (Will),
Quan, Evan, amd-gfx, dri-devel, Tuikov, Luben, Grodzovsky,
Andrey, Li, Dennis, Sundararaju, Sathishkumar, Kim, Jonathan,
Wang, Kevin(Yang),
Nieto, David M, Feng, Kenneth, Lee Jones, Clements, John,
linux-kernel, linux-hardening
[Public]
> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Monday, August 23, 2021 3:02 PM
> To: Kees Cook <keescook@chromium.org>; Lazar, Lijo
> <Lijo.Lazar@amd.com>
> Cc: Pan, Xinhui <Xinhui.Pan@amd.com>; David Airlie <airlied@linux.ie>;
> Daniel Vetter <daniel@ffwll.ch>; Zhang, Hawking
> <Hawking.Zhang@amd.com>; Xu, Feifei <Feifei.Xu@amd.com>; Gao, Likun
> <Likun.Gao@amd.com>; Gu, JiaWei (Will) <JiaWei.Gu@amd.com>; Quan,
> Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org; dri-
> devel@lists.freedesktop.org; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Tuikov, Luben
> <Luben.Tuikov@amd.com>; Grodzovsky, Andrey
> <Andrey.Grodzovsky@amd.com>; Li, Dennis <Dennis.Li@amd.com>;
> Sundararaju, Sathishkumar <Sathishkumar.Sundararaju@amd.com>; Kim,
> Jonathan <Jonathan.Kim@amd.com>; Wang, Kevin(Yang)
> <Kevin1.Wang@amd.com>; Nieto, David M <David.Nieto@amd.com>; Feng,
> Kenneth <Kenneth.Feng@amd.com>; Lee Jones <lee.jones@linaro.org>;
> Clements, John <John.Clements@amd.com>; linux-kernel@vger.kernel.org;
> linux-hardening@vger.kernel.org
> Subject: Re: [PATCH] drm/amd/pm: And destination bounds checking to
> struct copy
>
> Am 23.08.21 um 16:23 schrieb Kees Cook:
> >
> > On August 22, 2021 11:28:54 PM PDT, "Christian König"
> <christian.koenig@amd.com> wrote:
> >>
> >> Am 19.08.21 um 22:14 schrieb Kees Cook:
> >>> [...]
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> index 96e895d6be35..4605934a4fb7 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> @@ -1446,4 +1446,29 @@ static inline int amdgpu_in_reset(struct
> amdgpu_device *adev)
> >>> {
> >>> return atomic_read(&adev->in_gpu_reset);
> >>> }
> >>> +
> >>> +/**
> >>> + * 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 memcpy_trailing(dst, first_dst_member, last_dst_member,
> \
> >>> + src, first_src_member) \
> >> Please don't add a function like this into amdgpu.h, especially when
> >> it is only used by the SMU code.
> > Sure, I'm happy to move it. It wasn't clear to me which headers were
> considered "immutable". Which header should I put this in?
>
> I think amdgpu_smuio.h, but I'm not 100% sure. Alex do you have a better
> idea?
>
No, that's for the SMUIO callbacks for the IP block. Please use drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
Alex
> We don't want to put anything new into amdgpu.h any more since this is
> basically only a legacy leftover.
>
> Thanks,
> Christian.
>
> >
> >> And please give it an amdgpu_ prefix so that we are not confusing it
> >> with a core function.
> > Sure, I will include that.
> >
> >> Apart from that looks good to me.
> > Thanks!
> >
> > -Kees
^ permalink raw reply [flat|nested] 7+ messages in thread