Received: by 2002:a05:6a10:1d13:0:0:0:0 with SMTP id pp19csp1567788pxb; Fri, 20 Aug 2021 08:31:28 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxkG8tP7zMnOIR83SJkYOWue8Z1lxi8vCfTpUzTjB7nXDzQmuYFcoGuUhwsk0AOuOYozmvb X-Received: by 2002:a17:906:25d7:: with SMTP id n23mr1008828ejb.322.1629473488248; Fri, 20 Aug 2021 08:31:28 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1629473488; cv=none; d=google.com; s=arc-20160816; b=oyV6yrfTBUWYaLZbOciN9SelbXiIEk0HnJDTKCUCTlePkunZBKfUZIFS7xza21tm52 fhK7ZXeU9ms28I+zYs7x3dALlkRNyNB9e5OsVCDaZu65RH8kwNE9UaGxpCOqQTeKX8pE IAXUABUBO1kEBeh51xuJ7jZwTzcgiPH4/Q4p1ci3jlWKmJhbOOlQZ/GOiI2egC/qjQV7 dywSpQAon7RCVapTvLzzPVC7T+sg68gTiGl0+VyyGHvDiqeIkbA8FO4/FDxGIxzEdKku TXxRP76WMnzdPMyFp9n/HxMVLlWUIPEigRqrFVdNS7b9NsHWA6M2LTVkBcp9c0QhXdqP DZUA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:cc:to:subject :message-id:date:from:in-reply-to:references:mime-version :dkim-signature; bh=2eS3QJuOoUmoESeURE1tHhGFVya6B6JGJbYvqJ7L3mA=; b=EPG6rqVjRebzhmfQNp7AX9pmqnFtS/5Yph4F20Mu9JXWE0Ev81HYhj15pvE82p8XK5 SiJEQhBEpod6CMZpMN8iK/xX5rUhqs4R/efd0eXV/GBJ1KCmRKaYWzbSiEUyVkk5UU/p 7AvXlW57HMdfh9IK/G1lDJ5j33jq5DMOJTBDTeroaVp5P739zpUt0ViCVlE4wsHQ98V0 Y48qrwvDxIsOM83V6FjH3FNYA95H/zditApMwVyoXZkP+GJB4baei9zRBTwyZXHIOw98 VT35qYT7YvP6LpOQ3PdmEqhwyIyVG0HU8XsFTqx/DUpCRYWQgDPKiAiyqQassF8j9nJ/ JZkw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=FZ30HpWs; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id h20si6740812edw.295.2021.08.20.08.30.52; Fri, 20 Aug 2021 08:31:28 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=FZ30HpWs; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S241078AbhHTP2r (ORCPT + 99 others); Fri, 20 Aug 2021 11:28:47 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48410 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S238130AbhHTP2q (ORCPT ); Fri, 20 Aug 2021 11:28:46 -0400 Received: from mail-ot1-x333.google.com (mail-ot1-x333.google.com [IPv6:2607:f8b0:4864:20::333]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E313DC061575; Fri, 20 Aug 2021 08:28:08 -0700 (PDT) Received: by mail-ot1-x333.google.com with SMTP id r17-20020a0568302371b0290504f3f418fbso14442351oth.12; Fri, 20 Aug 2021 08:28:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=2eS3QJuOoUmoESeURE1tHhGFVya6B6JGJbYvqJ7L3mA=; b=FZ30HpWsbdlvXLmE01aRY6a5J0fsLSGL6MIQb3E4ZaPfC2mLNqAtbEhCO5fgoSiLs3 oa53tBxjoVERf3EJVHYSJKtQNlGGR3nf9CeQQntTgmqxipOigi9bAvc+Ztab/7qEMHC4 vuWtBbUXz7e33mw4bWrwS9kDvLC/SDhFAvKfK0Ar+XyubJ1C4m9+t2tpzkxsgYr0haWp 3+V073rf9UyztDAC4/gpGYkwNxeth0CkkYDvxeGkzmWFMwUOCK0rg0YlsvITRIseNsGT Z1yXHtFwLVaMCClbJMNxkAHywYEQszfcH25Uy5kQTRECBWb8ORudoaaFqgGiUy2q2dQM Ft6Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=2eS3QJuOoUmoESeURE1tHhGFVya6B6JGJbYvqJ7L3mA=; b=Kk1wcfRi18AaatXUusjwxHAK4eaHohBmuwcQ6QnPjlno3HBXjzKlMQJkEAtnWtCQ1S AZzFB6E65Gk9B26RQlIDmfqgd+WXj2UrwQMbf5dHnZ85dbnvOYRMn+C9T2HwQuSLkOnG 56LjIf3golUgLBMmSyk9Qu86khyF4I+CUUmPQ1XQTM8dS2XXDkGpYYAZC4qJwr0I3V3p twbYlRaSY4dw/eU2TwA6mppf4UiSPmssiEMDd3ShyztNITnrrR8v2Qz58eA4O73MTzy9 6G+X0r8wjOAFKjCsqThenZTigxb+tTqz5pnoU5wbXGdQFgKTp8uyfnGIe8Bl3Sw4sEcH tmYA== X-Gm-Message-State: AOAM533xLaPnUgQEWi8rLGJjtBhF7+1wcQ21mI8KeuLArVhppW8tTLzU fW6/bYNV9DCSErqEuoLOjHeaI8ZzOC36xvPrfV4= X-Received: by 2002:a05:6808:483:: with SMTP id z3mr3318825oid.5.1629473288249; Fri, 20 Aug 2021 08:28:08 -0700 (PDT) MIME-Version: 1.0 References: <20210819201441.3545027-1-keescook@chromium.org> In-Reply-To: <20210819201441.3545027-1-keescook@chromium.org> From: Alex Deucher Date: Fri, 20 Aug 2021 11:27:57 -0400 Message-ID: Subject: Re: [PATCH] drm/amd/pm: And destination bounds checking to struct copy To: Kees Cook Cc: Lijo Lazar , =?UTF-8?Q?Christian_K=C3=B6nig?= , "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@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Aug 19, 2021 at 4:14 PM 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://lore.kernel.org/lkml/e56aad3c-a06f-da07-f491-a894a570d78f@amd= .com > > Cc: Lijo Lazar > Cc: "Christian K=C3=B6nig" > Cc: "Pan, Xinhui" > Cc: David Airlie > Cc: Daniel Vetter > Cc: Hawking Zhang > Cc: Feifei Xu > Cc: Likun Gao > Cc: Jiawei Gu > Cc: Evan Quan > Cc: amd-gfx@lists.freedesktop.org > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Kees Cook > Link: https://lore.kernel.org/lkml/CADnq5_Npb8uYvd+R4UHgf-w8-cQj3JoODjviJ= R_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 > --- > 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/am= dgpu/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_de= vice *adev) > { > return atomic_read(&adev->in_gpu_reset); > } > + > +/** > + * memcpy_trailing - Copy the end of one structure into the middle of an= other > + * > + * @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 af= ter > + * @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 =3D offsetof(typeof(*(src)), first_src_member= ); \ > + size_t __src_size =3D sizeof(*(src)) - __src_offset; = \ > + size_t __dst_offset =3D offsetof(typeof(*(dst)), first_dst_member= ); \ > + size_t __dst_size =3D offsetofend(typeof(*(dst)), last_dst_member= ) - \ > + __dst_offset; = \ > + BUILD_BUG_ON(__src_size !=3D __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 sm= u_context *smu) > > if ((smc_dpm_table->table_header.format_revision =3D=3D 4) && > (smc_dpm_table->table_header.content_revision =3D=3D 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, BoardRese= rved, > + smc_dpm_table, maxvoltagestepgfx); > return 0; > } > > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c b/drivers/gp= u/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->I2cCon= trollers, > - sizeof(*smc_dpm_table) - sizeof(smc_dpm_table->ta= ble_header)); > + memcpy_trailing(smc_pptable, I2cControllers, BoardReserve= d, > + smc_dpm_table, I2cControllers); > break; > case 7: /* nv12 */ > ret =3D 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->I= 2cControllers, > - sizeof(*smc_dpm_table_v4_7) - sizeof(smc_dpm_tabl= e_v4_7->table_header)); > + memcpy_trailing(smc_pptable, I2cControllers, BoardReserve= d, > + smc_dpm_table_v4_7, I2cControllers); > break; > default: > dev_err(smu->adev->dev, "smc_dpm_info with unsupported co= ntent 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 sm= u_context *smu) > > if ((smc_dpm_table->table_header.format_revision =3D=3D 4) && > (smc_dpm_table->table_header.content_revision =3D=3D 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 >