linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Martin Sebor <msebor@gmail.com>
To: Arnd Bergmann <arnd@kernel.org>,
	Jani Nikula <jani.nikula@linux.intel.com>
Cc: "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	"Martin Sebor" <msebor@gcc.gnu.org>,
	"Joonas Lahtinen" <joonas.lahtinen@linux.intel.com>,
	"Rodrigo Vivi" <rodrigo.vivi@intel.com>,
	"David Airlie" <airlied@linux.ie>,
	"Daniel Vetter" <daniel@ffwll.ch>,
	"the arch/x86 maintainers" <x86@kernel.org>,
	"Ning Sun" <ning.sun@intel.com>,
	"Kalle Valo" <kvalo@codeaurora.org>,
	"Simon Kelley" <simon@thekelleys.org.uk>,
	"James Smart" <james.smart@broadcom.com>,
	"James E.J. Bottomley" <jejb@linux.ibm.com>,
	"Anders Larsen" <al@alarsen.net>, "Tejun Heo" <tj@kernel.org>,
	"Serge Hallyn" <serge@hallyn.com>,
	"Imre Deak" <imre.deak@intel.com>,
	"Linux ARM" <linux-arm-kernel@lists.infradead.org>,
	tboot-devel@lists.sourceforge.net,
	"Intel Graphics" <intel-gfx@lists.freedesktop.org>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	ath11k@lists.infradead.org,
	linux-wireless <linux-wireless@vger.kernel.org>,
	Networking <netdev@vger.kernel.org>,
	linux-scsi <linux-scsi@vger.kernel.org>,
	Cgroups <cgroups@vger.kernel.org>,
	"LSM List" <linux-security-module@vger.kernel.org>,
	"Ville Syrjälä" <ville.syrjala@linux.intel.com>,
	"Manasi Navare" <manasi.d.navare@intel.com>,
	"Uma Shankar" <uma.shankar@intel.com>,
	"Ankit Nautiyal" <ankit.k.nautiyal@intel.com>,
	"Gwan-gyeong Mun" <gwan-gyeong.mun@intel.com>,
	"Animesh Manna" <animesh.manna@intel.com>,
	"Sean Paul" <seanpaul@chromium.org>
Subject: Re: [PATCH 11/11] [RFC] drm/i915/dp: fix array overflow warning
Date: Thu, 25 Mar 2021 08:49:56 -0600	[thread overview]
Message-ID: <44ad545d-cc07-2e5f-9ec8-ad848f39268a@gmail.com> (raw)
In-Reply-To: <CAK8P3a0HGiPQ-k6t6roTgeUvVAMMY=fMnGV0+t48yJjz55XFAA@mail.gmail.com>

On 3/25/21 3:53 AM, Arnd Bergmann wrote:
> On Thu, Mar 25, 2021 at 9:05 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>> Clearly something is wrong here, but I can't quite figure out what.
>>> Changing the array size to 16 bytes avoids the warning, but is
>>> probably the wrong solution here.
>>
>> Ugh. drm_dp_channel_eq_ok() does not actually require more than
>> DP_LINK_STATUS_SIZE - 2 elements in the link_status. It's some other
>> related functions that do, and in most cases it's convenient to read all
>> those DP_LINK_STATUS_SIZE bytes.
>>
>> However, here the case is slightly different for DP MST, and the change
>> causes reserved DPCD addresses to be read. Not sure it matters, but
>> really I think the problem is what drm_dp_channel_eq_ok() advertizes.
>>
>> I also don't like the array notation with sizes in function parameters
>> in general, because I think it's misleading. Would gcc-11 warn if a
>> function actually accesses the memory out of bounds of the size?
> 
> Yes, that is the point of the warning. Using an explicit length in an
> array argument type tells gcc that the function will never access
> beyond the end of that bound, and that passing a short array
> is a bug.
> 
> I don't know if this /only/ means triggering a warning, or if gcc
> is also able to make optimizations after classifying this as undefined
> behavior that it would not make for an unspecified length.

GCC uses the array parameter notation as a hint for warnings but
it doesn't optimize on this basis and never will be able to because
code that accesses more elements from the array isn't invalid.
Adding static to the bound, as in void f (int[static N]) does
imply that the function won't access more than N elements and
C intends for optimizers to rely on it, although GCC doesn't yet.

The warning for the array notation is a more portable alternative
to explicitly annotating functions with attribute access, and to
-Wvla-parameter for VLA parameters.  The latter seem to be used
relatively rarely, sometimes deliberately because of the bad rap
of VLA objects, even though VLA parameters don't suffer from
the same problems.

Martin

> 
>> Anyway. I don't think we're going to get rid of the array notation
>> anytime soon, if ever, no matter how much I dislike it, so I think the
>> right fix would be to at least state the correct required size in
>> drm_dp_channel_eq_ok().
> 
> Ok. Just to confirm: Changing the declaration to an unspecified length
> avoids the warnings, as does the patch below:
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index eedbb48815b7..6ebeec3d88a7 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -46,12 +46,12 @@
>    */
> 
>   /* Helpers for DP link training */
> -static u8 dp_link_status(const u8 link_status[DP_LINK_STATUS_SIZE], int r)
> +static u8 dp_link_status(const u8 link_status[DP_LINK_STATUS_SIZE - 2], int r)
>   {
>          return link_status[r - DP_LANE0_1_STATUS];
>   }
> 
> -static u8 dp_get_lane_status(const u8 link_status[DP_LINK_STATUS_SIZE],
> +static u8 dp_get_lane_status(const u8 link_status[DP_LINK_STATUS_SIZE - 2],
>                               int lane)
>   {
>          int i = DP_LANE0_1_STATUS + (lane >> 1);
> @@ -61,7 +61,7 @@ static u8 dp_get_lane_status(const u8
> link_status[DP_LINK_STATUS_SIZE],
>          return (l >> s) & 0xf;
>   }
> 
> -bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE],
> +bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE - 2],
>                            int lane_count)
>   {
>          u8 lane_align;
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index edffd1dcca3e..160f7fd127b1 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -1456,7 +1456,7 @@ enum drm_dp_phy {
> 
>   #define DP_LINK_CONSTANT_N_VALUE 0x8000
>   #define DP_LINK_STATUS_SIZE       6
> -bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE],
> +bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE - 2],
>                            int lane_count);
>   bool drm_dp_clock_recovery_ok(const u8 link_status[DP_LINK_STATUS_SIZE],
>                                int lane_count);
> 
> 
> This obviously needs a good explanation in the code and the changelog text,
> which I don't have, but if the above is what you had in mind, please take that
> and add Reported-by/Tested-by: Arnd Bergmann <arnd@arndb.de>.
> 
>         Arnd
> 


  reply	other threads:[~2021-03-25 14:51 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-22 16:02 [PATCH 00/11] treewide: address gcc-11 -Wstringop-overread warnings Arnd Bergmann
2021-03-22 16:02 ` [PATCH 01/11] x86: compressed: avoid gcc-11 -Wstringop-overread warning Arnd Bergmann
2021-03-22 23:30   ` [tip: x86/boot] x86/boot/compressed: Avoid " tip-bot2 for Arnd Bergmann
2021-03-22 16:02 ` [PATCH 02/11] x86: tboot: avoid Wstringop-overread-warning Arnd Bergmann
2021-03-22 20:29   ` Ingo Molnar
2021-03-22 21:39     ` Arnd Bergmann
2021-03-22 22:07     ` Martin Sebor
2021-03-22 22:49       ` Arnd Bergmann
2021-03-22 23:13       ` Ingo Molnar
2021-03-24  9:11       ` David Laight
2021-03-24 10:39         ` David Laight
2021-03-22 23:30   ` [tip: x86/boot] x86/boot/tboot: Avoid Wstringop-overread-warning tip-bot2 for Arnd Bergmann
2021-03-22 16:02 ` [PATCH 03/11] security: commoncap: fix -Wstringop-overread warning Arnd Bergmann
2021-03-22 16:31   ` Christian Brauner
2021-03-24 20:50   ` James Morris
2021-03-22 16:02 ` [PATCH 04/11] ath11: Wstringop-overread warning Arnd Bergmann
2021-09-28  9:04   ` Kalle Valo
2021-03-22 16:02 ` [PATCH 05/11] qnx: avoid -Wstringop-overread warning Arnd Bergmann
2021-03-22 16:02 ` [PATCH 06/11] cgroup: fix -Wzero-length-bounds warnings Arnd Bergmann
2021-03-30  8:41   ` Michal Koutný
2021-03-30  9:00     ` Arnd Bergmann
2021-03-30 14:44       ` Michal Koutný
2021-03-22 16:02 ` [PATCH 07/11] ARM: sharpsl_param: work around -Wstringop-overread warning Arnd Bergmann
2021-03-22 16:02 ` [PATCH 08/11] atmel: avoid gcc -Wstringop-overflow warning Arnd Bergmann
2021-03-22 16:02 ` [PATCH 09/11] scsi: lpfc: fix gcc -Wstringop-overread warning Arnd Bergmann
2021-03-22 16:02 ` [PATCH 10/11] drm/i915: avoid stringop-overread warning on pri_latency Arnd Bergmann
2021-03-24 15:30   ` Jani Nikula
2021-03-24 17:22     ` Ville Syrjälä
2021-03-22 16:02 ` [PATCH 11/11] [RFC] drm/i915/dp: fix array overflow warning Arnd Bergmann
2021-03-25  8:05   ` Jani Nikula
2021-03-25  9:53     ` Arnd Bergmann
2021-03-25 14:49       ` Martin Sebor [this message]
2021-03-30 10:56   ` Hans de Goede
2021-04-06  4:53 ` [PATCH 00/11] treewide: address gcc-11 -Wstringop-overread warnings Martin K. Petersen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=44ad545d-cc07-2e5f-9ec8-ad848f39268a@gmail.com \
    --to=msebor@gmail.com \
    --cc=airlied@linux.ie \
    --cc=al@alarsen.net \
    --cc=animesh.manna@intel.com \
    --cc=ankit.k.nautiyal@intel.com \
    --cc=arnd@kernel.org \
    --cc=ath11k@lists.infradead.org \
    --cc=cgroups@vger.kernel.org \
    --cc=daniel@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=gwan-gyeong.mun@intel.com \
    --cc=imre.deak@intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=james.smart@broadcom.com \
    --cc=jani.nikula@linux.intel.com \
    --cc=jejb@linux.ibm.com \
    --cc=joonas.lahtinen@linux.intel.com \
    --cc=kvalo@codeaurora.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=manasi.d.navare@intel.com \
    --cc=msebor@gcc.gnu.org \
    --cc=netdev@vger.kernel.org \
    --cc=ning.sun@intel.com \
    --cc=rodrigo.vivi@intel.com \
    --cc=seanpaul@chromium.org \
    --cc=serge@hallyn.com \
    --cc=simon@thekelleys.org.uk \
    --cc=tboot-devel@lists.sourceforge.net \
    --cc=tj@kernel.org \
    --cc=uma.shankar@intel.com \
    --cc=ville.syrjala@linux.intel.com \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).