Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220303172503.GA1731116@embeddedor> <202203030937.5BFE3EF@keescook>
 <20220303181957.GA1731711@embeddedor> <20220303204526.GA1733280@embeddedor>
In-Reply-To: <20220303204526.GA1733280@embeddedor>
From:   Alex Deucher <alexdeucher@gmail.com>
Date:   Fri, 4 Mar 2022 12:08:38 -0500
Message-ID: <CADnq5_M6bJUOOY3Uc-8+grGKQTL=AsXC9SXaO4KYdc=z7WVGtg@mail.gmail.com>
Subject: Re: [PATCH][next] drm/amd/display: Fix Wstringop-overflow warnings in dc_link_dp.c
To:     "Gustavo A. R. Silva" <gustavoars@kernel.org>
Cc:     Kees Cook <keescook@chromium.org>, Leo Li <sunpeng.li@amd.com>,
        "Pan, Xinhui" <Xinhui.Pan@amd.com>,
        Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>,
        LKML <linux-kernel@vger.kernel.org>,
        amd-gfx list <amd-gfx@lists.freedesktop.org>,
        David Airlie <airlied@linux.ie>,
        Maling list - DRI developers 
        <dri-devel@lists.freedesktop.org>, Daniel Vetter <daniel@ffwll.ch>,
        Alex Deucher <alexander.deucher@amd.com>,
        Harry Wentland <harry.wentland@amd.com>,
        =?UTF-8?Q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>,
        linux-hardening@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Mar 3, 2022 at 3:37 PM Gustavo A. R. Silva
<gustavoars@kernel.org> wrote:
>
> On Thu, Mar 03, 2022 at 12:19:57PM -0600, Gustavo A. R. Silva wrote:
> > On Thu, Mar 03, 2022 at 09:43:28AM -0800, Kees Cook wrote:
> > > On Thu, Mar 03, 2022 at 11:25:03AM -0600, Gustavo A. R. Silva wrote:
> > > > Fix the following Wstringop-overflow warnings when building with GC=
C-11:
> > > >
> > > > drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_link_dpia.c:493:17=
: warning: =E2=80=98dp_decide_lane_settings=E2=80=99 accessing 4 bytes in a=
 region of size 1 [-Wstringop-overflow=3D]
> > >
> > > Can you "show your work" a little more here? I don't actually see the
> > > what is getting fixed:
> > >
> > > enum dc_lane_count {
> > >     ...
> > >         LANE_COUNT_FOUR =3D 4,
> > >     ...
> > >         LANE_COUNT_DP_MAX =3D LANE_COUNT_FOUR
> > > };
> > >
> > > struct link_training_settings {
> > >     ...
> > >         union dpcd_training_lane dpcd_lane_settings[LANE_COUNT_DP_MAX=
];
> > > };
> > >
> > > void dp_hw_to_dpcd_lane_settings(
> > >             ...
> > >             union dpcd_training_lane dpcd_lane_settings[LANE_COUNT_DP=
_MAX])
> > > {
> > >     ...
> > > }
> > >
> > > static enum link_training_result dpia_training_cr_transparent(
> > >             ...
> > >                 struct link_training_settings *lt_settings)
> > > {
> > >     ...
> > >                 dp_decide_lane_settings(lt_settings, dpcd_lane_adjust=
,
> > >                                 lt_settings->hw_lane_settings, lt_set=
tings->dpcd_lane_settings);
> > >     ...
> > > }
> > >
> > > Everything looks to be the correct size?
> >
> > Yep; this fix is similar to the one for intel_pm.c in this
> >
> >       commit e7c6e405e171fb33990a12ecfd14e6500d9e5cf2
> >
> > where the array size of 8 seems to be fine for all the
> > struct members related (pri_latency, spr_latency, cur_latency
> > and skl_latency):
> >
> > drivers/gpu/drm/i915/i915_drv.h:465:struct drm_i915_private {
> > ...
> >
> > drivers/gpu/drm/i915/i915_drv.h-739-    struct {
> >
> > ...
> > drivers/gpu/drm/i915/i915_drv.h-745-            /* primary */
> > drivers/gpu/drm/i915/i915_drv.h-746-            u16 pri_latency[5];
> > drivers/gpu/drm/i915/i915_drv.h-747-            /* sprite */
> > drivers/gpu/drm/i915/i915_drv.h-748-            u16 spr_latency[5];
> > drivers/gpu/drm/i915/i915_drv.h-749-            /* cursor */
> > drivers/gpu/drm/i915/i915_drv.h-750-            u16 cur_latency[5];
> > drivers/gpu/drm/i915/i915_drv.h-751-            /*
> > drivers/gpu/drm/i915/i915_drv.h-752-             * Raw watermark memory=
 latency values
> > drivers/gpu/drm/i915/i915_drv.h-753-             * for SKL for all 8 le=
vels
> > drivers/gpu/drm/i915/i915_drv.h-754-             * in 1us units.
> > drivers/gpu/drm/i915/i915_drv.h-755-             */
> > drivers/gpu/drm/i915/i915_drv.h-756-            u16 skl_latency[8];
> >
> > ...
> > drivers/gpu/drm/i915/i915_drv.h-773-    } wm;
> > ...
> > }
>
> and in this case the ilk_wm_max_level() returns the right maximum size fo=
r the
> corresponding 'struct wm' member:
>
> drivers/gpu/drm/i915/intel_pm.c:2993:int ilk_wm_max_level(const struct dr=
m_i915_private *dev_priv)
> drivers/gpu/drm/i915/intel_pm.c-2994-{
> drivers/gpu/drm/i915/intel_pm.c-2995-   /* how many WM levels are we expe=
cting */
> drivers/gpu/drm/i915/intel_pm.c-2996-   if (HAS_HW_SAGV_WM(dev_priv))
> drivers/gpu/drm/i915/intel_pm.c-2997-           return 5;
> drivers/gpu/drm/i915/intel_pm.c-2998-   else if (DISPLAY_VER(dev_priv) >=
=3D 9)
> drivers/gpu/drm/i915/intel_pm.c-2999-           return 7;
> drivers/gpu/drm/i915/intel_pm.c-3000-   else if (IS_HASWELL(dev_priv) || =
IS_BROADWELL(dev_priv))
> drivers/gpu/drm/i915/intel_pm.c-3001-           return 4;
> drivers/gpu/drm/i915/intel_pm.c-3002-   else if (DISPLAY_VER(dev_priv) >=
=3D 6)
> drivers/gpu/drm/i915/intel_pm.c-3003-           return 3;
> drivers/gpu/drm/i915/intel_pm.c-3004-   else
> drivers/gpu/drm/i915/intel_pm.c-3005-           return 2;
> drivers/gpu/drm/i915/intel_pm.c-3006-}
>
> drivers/gpu/drm/i915/intel_pm.c:3009:static void intel_print_wm_latency(s=
truct drm_i915_private *dev_priv,
> drivers/gpu/drm/i915/intel_pm.c-3010-                              const =
char *name,
> drivers/gpu/drm/i915/intel_pm.c-3011-                              const =
u16 wm[])
> drivers/gpu/drm/i915/intel_pm.c-3012-{
> drivers/gpu/drm/i915/intel_pm.c-3013-   int level, max_level =3D ilk_wm_m=
ax_level(dev_priv);
> drivers/gpu/drm/i915/intel_pm.c-3014-
> drivers/gpu/drm/i915/intel_pm.c-3015-   for (level =3D 0; level <=3D max_=
level; level++) {
> drivers/gpu/drm/i915/intel_pm.c-3016-           unsigned int latency =3D =
wm[level];
> drivers/gpu/drm/i915/intel_pm.c-3017-
> ...
> }
>
> still GCC warns about this with Wstringop-overread, as it is explained
> in commit e7c6e405e171.

Did you want to respin with expanded explanation?

Alex

>
> --
> Gustavo
>
> >
> > however GCC warns about accessing bytes beyond the limits, and turning =
the
> > argument declarations into pointers (removing the over-specified array
> > size from the argument declaration) silence the warnings.
> >
> > --
> > Gustavo