[PATCH 00/11] treewide: address gcc-11 -Wstringop-overread warnings

[PATCH 00/11] treewide: address gcc-11 -Wstringop-overread warnings

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

The coming gcc release introduces a new warning for string operations
reading beyond the end of a fixed-length object. After testing
randconfig kernels for a while, think I have patches for any such
warnings that came up on x86, arm and arm64.

Most of these warnings are false-positive ones, either gcc warning
about something that is entirely correct, or about something that
looks suspicious but turns out to be correct after all.

The two patches for the i915 driver look like something that might
be actual bugs, but I am not sure about those either.

We probably want some combination of workaround like the ones I
post here and changes to gcc to have fewer false positives in the
release. I'm posting the entire set of workaround that give me
a cleanly building kernel for reference here.

        Arnd

Arnd Bergmann (11):
  x86: compressed: avoid gcc-11 -Wstringop-overread warning
  x86: tboot: avoid Wstringop-overread-warning
  security: commoncap: fix -Wstringop-overread warning
  ath11: Wstringop-overread warning
  qnx: avoid -Wstringop-overread warning
  cgroup: fix -Wzero-length-bounds warnings
  ARM: sharpsl_param: work around -Wstringop-overread warning
  atmel: avoid gcc -Wstringop-overflow warning
  scsi: lpfc: fix gcc -Wstringop-overread warning
  drm/i915: avoid stringop-overread warning on pri_latency
  [RFC] drm/i915/dp: fix array overflow warning

 arch/arm/common/sharpsl_param.c         |  4 ++-
 arch/x86/boot/compressed/misc.c         |  2 --
 arch/x86/kernel/tboot.c                 | 44 +++++++++++++++----------
 drivers/gpu/drm/i915/display/intel_dp.c |  2 +-
 drivers/gpu/drm/i915/i915_drv.h         |  6 ++--
 drivers/net/wireless/ath/ath11k/mac.c   |  2 +-
 drivers/net/wireless/atmel/atmel.c      | 25 ++++++++------
 drivers/scsi/lpfc/lpfc_attr.c           |  6 ++--
 fs/qnx4/dir.c                           | 11 +++----
 kernel/cgroup/cgroup.c                  | 15 +++++++--
 security/commoncap.c                    |  2 +-
 11 files changed, 69 insertions(+), 50 deletions(-)

Cc: x86@kernel.org
Cc: Ning Sun <ning.sun@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Kalle Valo <kvalo@codeaurora.org>
Cc: Simon Kelley <simon@thekelleys.org.uk>
Cc: James Smart <james.smart@broadcom.com>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: Anders Larsen <al@alarsen.net>
Cc: Tejun Heo <tj@kernel.org>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: tboot-devel@lists.sourceforge.net
Cc: intel-gfx@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Cc: ath11k@lists.infradead.org
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
Cc: linux-scsi@vger.kernel.org
Cc: cgroups@vger.kernel.org
Cc: linux-security-module@vger.kernel.org


-- 
2.29.2

[PATCH 01/11] x86: compressed: avoid gcc-11 -Wstringop-overread warning

[Arnd Bergmann]

From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>

gcc gets confused by the comparison of a pointer to an integer listeral,
with the assumption that this is an offset from a NULL pointer and that
dereferencing it is invalid:

In file included from arch/x86/boot/compressed/misc.c:18:
In function ‘parse_elf’,
    inlined from ‘extract_kernel’ at arch/x86/boot/compressed/misc.c:442:2:
arch/x86/boot/compressed/../string.h:15:23: error: ‘__builtin_memcpy’ reading 64 bytes from a region of size 0 [-Werror=stringop-overread]
   15 | #define memcpy(d,s,l) __builtin_memcpy(d,s,l)
      |                       ^~~~~~~~~~~~~~~~~~~~~~~
arch/x86/boot/compressed/misc.c:283:9: note: in expansion of macro ‘memcpy’
  283 |         memcpy(&ehdr, output, sizeof(ehdr));
      |         ^~~~~~

I could not find any good workaround for this, but as this is only
a warning for a failure during early boot, removing the line entirely
works around the warning.

This should probably get addressed in gcc instead, before 11.1 gets
released.

Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
---
 arch/x86/boot/compressed/misc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index 3a214cc3239f..9ada64e66cb7 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -430,8 +430,6 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
 		error("Destination address too large");
 #endif
 #ifndef CONFIG_RELOCATABLE
-	if ((unsigned long)output != LOAD_PHYSICAL_ADDR)
-		error("Destination address does not match LOAD_PHYSICAL_ADDR");
 	if (virt_addr != LOAD_PHYSICAL_ADDR)
 		error("Destination virtual address changed when not relocatable");
 #endif
-- 
2.29.2

[PATCH 04/11] ath11: Wstringop-overread warning

[Arnd Bergmann]

From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>

gcc-11 with the kernel address sanitizer prints a warning for this
driver:

In function 'ath11k_peer_assoc_h_vht',
    inlined from 'ath11k_peer_assoc_prepare' at drivers/net/wireless/ath/ath11k/mac.c:1632:2:
drivers/net/wireless/ath/ath11k/mac.c:1164:13: error: 'ath11k_peer_assoc_h_vht_masked' reading 16 bytes from a region of size 4 [-Werror=stringop-overread]
 1164 |         if (ath11k_peer_assoc_h_vht_masked(vht_mcs_mask))
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/wireless/ath/ath11k/mac.c: In function 'ath11k_peer_assoc_prepare':
drivers/net/wireless/ath/ath11k/mac.c:1164:13: note: referencing argument 1 of type 'const u16 *' {aka 'const short unsigned int *'}
drivers/net/wireless/ath/ath11k/mac.c:969:1: note: in a call to function 'ath11k_peer_assoc_h_vht_masked'
  969 | ath11k_peer_assoc_h_vht_masked(const u16 vht_mcs_mask[NL80211_VHT_NSS_MAX])
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

According to analysis from gcc developers, this is a glitch in the
way gcc tracks the size of struct members. This should really get
fixed in gcc, but it's also easy to work around this instance
by changing the function prototype to no include the length of
the array.

Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99673
Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
---
 drivers/net/wireless/ath/ath11k/mac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c
index b391169576e2..5cb7ed53f3c4 100644
--- a/drivers/net/wireless/ath/ath11k/mac.c
+++ b/drivers/net/wireless/ath/ath11k/mac.c
@@ -966,7 +966,7 @@ ath11k_peer_assoc_h_ht_masked(const u8 ht_mcs_mask[IEEE80211_HT_MCS_MASK_LEN])
 }
 
 static bool
-ath11k_peer_assoc_h_vht_masked(const u16 vht_mcs_mask[NL80211_VHT_NSS_MAX])
+ath11k_peer_assoc_h_vht_masked(const u16 vht_mcs_mask[])
 {
 	int nss;
 
-- 
2.29.2

Re: [PATCH 04/11] ath11: Wstringop-overread warning

[Kalle Valo]

Arnd Bergmann <arnd@kernel.org> wrote:

> gcc-11 with the kernel address sanitizer prints a warning for this
> driver:
> 
> In function 'ath11k_peer_assoc_h_vht',
>     inlined from 'ath11k_peer_assoc_prepare' at drivers/net/wireless/ath/ath11k/mac.c:1632:2:
> drivers/net/wireless/ath/ath11k/mac.c:1164:13: error: 'ath11k_peer_assoc_h_vht_masked' reading 16 bytes from a region of size 4 [-Werror=stringop-overread]
>  1164 |         if (ath11k_peer_assoc_h_vht_masked(vht_mcs_mask))
>       |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/net/wireless/ath/ath11k/mac.c: In function 'ath11k_peer_assoc_prepare':
> drivers/net/wireless/ath/ath11k/mac.c:1164:13: note: referencing argument 1 of type 'const u16 *' {aka 'const short unsigned int *'}
> drivers/net/wireless/ath/ath11k/mac.c:969:1: note: in a call to function 'ath11k_peer_assoc_h_vht_masked'
>   969 | ath11k_peer_assoc_h_vht_masked(const u16 vht_mcs_mask[NL80211_VHT_NSS_MAX])
>       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> According to analysis from gcc developers, this is a glitch in the
> way gcc tracks the size of struct members. This should really get
> fixed in gcc, but it's also easy to work around this instance
> by changing the function prototype to no include the length of
> the array.
> 
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99673
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

Patch applied to ath-next branch of ath.git, thanks.

eb19efed836a ath11k: Wstringop-overread warning

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20210322160253.4032422-5-arnd@kernel.org/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 04/11] ath11: Wstringop-overread warning

[Kalle Valo]

Arnd Bergmann <arnd@kernel.org> wrote:

> gcc-11 with the kernel address sanitizer prints a warning for this
> driver:
> 
> In function 'ath11k_peer_assoc_h_vht',
>     inlined from 'ath11k_peer_assoc_prepare' at drivers/net/wireless/ath/ath11k/mac.c:1632:2:
> drivers/net/wireless/ath/ath11k/mac.c:1164:13: error: 'ath11k_peer_assoc_h_vht_masked' reading 16 bytes from a region of size 4 [-Werror=stringop-overread]
>  1164 |         if (ath11k_peer_assoc_h_vht_masked(vht_mcs_mask))
>       |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/net/wireless/ath/ath11k/mac.c: In function 'ath11k_peer_assoc_prepare':
> drivers/net/wireless/ath/ath11k/mac.c:1164:13: note: referencing argument 1 of type 'const u16 *' {aka 'const short unsigned int *'}
> drivers/net/wireless/ath/ath11k/mac.c:969:1: note: in a call to function 'ath11k_peer_assoc_h_vht_masked'
>   969 | ath11k_peer_assoc_h_vht_masked(const u16 vht_mcs_mask[NL80211_VHT_NSS_MAX])
>       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> According to analysis from gcc developers, this is a glitch in the
> way gcc tracks the size of struct members. This should really get
> fixed in gcc, but it's also easy to work around this instance
> by changing the function prototype to no include the length of
> the array.
> 
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99673
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

Patch applied to ath-next branch of ath.git, thanks.

eb19efed836a ath11k: Wstringop-overread warning

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20210322160253.4032422-5-arnd@kernel.org/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

[PATCH 05/11] qnx: avoid -Wstringop-overread warning

[Arnd Bergmann]

From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>

gcc-11 warns that the size of the link name is longer than the di_fname
field:

fs/qnx4/dir.c: In function ‘qnx4_readdir’:
fs/qnx4/dir.c:51:32: error: ‘strnlen’ specified bound 48 exceeds source size 16 [-Werror=stringop-overread]
   51 |                         size = strnlen(de->di_fname, size);
      |                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from fs/qnx4/qnx4.h:3,
                 from fs/qnx4/dir.c:16:
include/uapi/linux/qnx4_fs.h:45:25: note: source object declared here
   45 |         char            di_fname[QNX4_SHORT_NAME_MAX];

The problem here is that we access the same pointer using two different
structure layouts, but gcc determines the object size based on
whatever it encounters first.

Change the strnlen to use the correct field size in each case, and
change the first access to be on the longer field.

Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
---
 fs/qnx4/dir.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/fs/qnx4/dir.c b/fs/qnx4/dir.c
index a6ee23aadd28..68046450e543 100644
--- a/fs/qnx4/dir.c
+++ b/fs/qnx4/dir.c
@@ -39,21 +39,20 @@ static int qnx4_readdir(struct file *file, struct dir_context *ctx)
 		ix = (ctx->pos >> QNX4_DIR_ENTRY_SIZE_BITS) % QNX4_INODES_PER_BLOCK;
 		for (; ix < QNX4_INODES_PER_BLOCK; ix++, ctx->pos += QNX4_DIR_ENTRY_SIZE) {
 			offset = ix * QNX4_DIR_ENTRY_SIZE;
-			de = (struct qnx4_inode_entry *) (bh->b_data + offset);
-			if (!de->di_fname[0])
+			le = (struct qnx4_link_info *)(bh->b_data + offset);
+			de = (struct qnx4_inode_entry *)(bh->b_data + offset);
+			if (!le->dl_fname[0])
 				continue;
 			if (!(de->di_status & (QNX4_FILE_USED|QNX4_FILE_LINK)))
 				continue;
 			if (!(de->di_status & QNX4_FILE_LINK))
-				size = QNX4_SHORT_NAME_MAX;
+				size = strnlen(de->di_fname, sizeof(de->di_fname));
 			else
-				size = QNX4_NAME_MAX;
-			size = strnlen(de->di_fname, size);
+				size = strnlen(le->dl_fname, sizeof(le->dl_fname));
 			QNX4DEBUG((KERN_INFO "qnx4_readdir:%.*s\n", size, de->di_fname));
 			if (!(de->di_status & QNX4_FILE_LINK))
 				ino = blknum * QNX4_INODES_PER_BLOCK + ix - 1;
 			else {
-				le  = (struct qnx4_link_info*)de;
 				ino = ( le32_to_cpu(le->dl_inode_blk) - 1 ) *
 					QNX4_INODES_PER_BLOCK +
 					le->dl_inode_ndx;
-- 
2.29.2

[PATCH 06/11] cgroup: fix -Wzero-length-bounds warnings

[Arnd Bergmann]

From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>

When cgroups are enabled, but every single subsystem is turned off,
CGROUP_SUBSYS_COUNT is zero, and the cgrp->subsys[] array has no
members.

gcc-11 points out that this leads to an invalid access in any function
that might access this array:

kernel/cgroup/cgroup.c: In function 'cgroup_addrm_files':
kernel/cgroup/cgroup.c:460:58: warning: array subscript '<unknown>' is outside the bounds of an interior zero-length array 'struct cgroup_subsys_state *[0]' [-Wzero-length-bounds]
kernel/cgroup/cgroup.c:460:24: note: in expansion of macro 'rcu_dereference_check'
  460 |                 return rcu_dereference_check(cgrp->subsys[ss->id],
      |                        ^~~~~~~~~~~~~~~~~~~~~
In file included from include/linux/cgroup.h:28,
                 from kernel/cgroup/cgroup-internal.h:5,
                 from kernel/cgroup/cgroup.c:31:
include/linux/cgroup-defs.h:422:43: note: while referencing 'subsys'
  422 |         struct cgroup_subsys_state __rcu *subsys[CGROUP_SUBSYS_COUNT];

I'm not sure what is expected to happen for such a configuration,
presumably these functions are never calls in that case. Adding a
sanity check in each function we get the warning for manages to shut
up the warnings and do nothing instead.

Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
---
I'm grouping this together with the -Wstringop-overread warnings,
since the underlying logic in gcc seems to be the same.
---
 kernel/cgroup/cgroup.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 9153b20e5cc6..3477f1dc7872 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -456,7 +456,7 @@ static u16 cgroup_ss_mask(struct cgroup *cgrp)
 static struct cgroup_subsys_state *cgroup_css(struct cgroup *cgrp,
 					      struct cgroup_subsys *ss)
 {
-	if (ss)
+	if (ss && (CGROUP_SUBSYS_COUNT > 0))
 		return rcu_dereference_check(cgrp->subsys[ss->id],
 					lockdep_is_held(&cgroup_mutex));
 	else
@@ -534,6 +534,9 @@ struct cgroup_subsys_state *cgroup_e_css(struct cgroup *cgrp,
 {
 	struct cgroup_subsys_state *css;
 
+	if (CGROUP_SUBSYS_COUNT == 0)
+		return NULL;
+
 	do {
 		css = cgroup_css(cgrp, ss);
 
@@ -561,6 +564,9 @@ struct cgroup_subsys_state *cgroup_get_e_css(struct cgroup *cgrp,
 {
 	struct cgroup_subsys_state *css;
 
+	if (CGROUP_SUBSYS_COUNT == 0)
+		return NULL;
+
 	rcu_read_lock();
 
 	do {
@@ -630,7 +636,7 @@ struct cgroup_subsys_state *of_css(struct kernfs_open_file *of)
 	 * the matching css from the cgroup's subsys table is guaranteed to
 	 * be and stay valid until the enclosing operation is complete.
 	 */
-	if (cft->ss)
+	if (cft->ss && CGROUP_SUBSYS_COUNT > 0)
 		return rcu_dereference_raw(cgrp->subsys[cft->ss->id]);
 	else
 		return &cgrp->self;
@@ -2343,6 +2349,9 @@ struct task_struct *cgroup_taskset_next(struct cgroup_taskset *tset,
 	struct css_set *cset = tset->cur_cset;
 	struct task_struct *task = tset->cur_task;
 
+	if (CGROUP_SUBSYS_COUNT == 0)
+		return NULL;
+
 	while (&cset->mg_node != tset->csets) {
 		if (!task)
 			task = list_first_entry(&cset->mg_tasks,
@@ -4523,7 +4532,7 @@ void css_task_iter_start(struct cgroup_subsys_state *css, unsigned int flags,
 	it->ss = css->ss;
 	it->flags = flags;
 
-	if (it->ss)
+	if (it->ss && CGROUP_SUBSYS_COUNT > 0)
 		it->cset_pos = &css->cgroup->e_csets[css->ss->id];
 	else
 		it->cset_pos = &css->cgroup->cset_links;
-- 
2.29.2

Re: [PATCH 06/11] cgroup: fix -Wzero-length-bounds warnings

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 705 bytes --]

On Mon, Mar 22, 2021 at 05:02:44PM +0100, Arnd Bergmann <arnd-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> I'm not sure what is expected to happen for such a configuration,
> presumably these functions are never calls in that case.
Yes, the functions you patched would only be called from subsystems or
there should be no way to obtain a struct cgroup_subsys reference
anyway (hence it's ok to always branch as if ss==NULL).

I'd prefer a variant that wouldn't compile the affected codepaths when
there are no subsystems registered, however, I couldn't come up with a
way how to do it without some preprocessor ugliness.

Reviewed-by: Michal Koutný <mkoutny-IBi9RG/b67k@public.gmane.org>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 06/11] cgroup: fix -Wzero-length-bounds warnings

[Arnd Bergmann]

On Tue, Mar 30, 2021 at 10:41 AM Michal Koutný <mkoutny-IBi9RG/b67k@public.gmane.org> wrote:
>
> On Mon, Mar 22, 2021 at 05:02:44PM +0100, Arnd Bergmann <arnd-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > I'm not sure what is expected to happen for such a configuration,
> > presumably these functions are never calls in that case.
> Yes, the functions you patched would only be called from subsystems or
> there should be no way to obtain a struct cgroup_subsys reference
> anyway (hence it's ok to always branch as if ss==NULL).
>
> I'd prefer a variant that wouldn't compile the affected codepaths when
> there are no subsystems registered, however, I couldn't come up with a
> way how to do it without some preprocessor ugliness.

Would it be possible to enclose most or all of kernel/cgroup/cgroup.c
in an #ifdef CGROUP_SUBSYS_COUNT block? I didn't try that
myself, but this might be a way to guarantee that there cannot
be any callers (it would cause a link error).

> Reviewed-by: Michal Koutný <mkoutny-IBi9RG/b67k@public.gmane.org>

Thanks

        Arnd

Re: [PATCH 06/11] cgroup: fix -Wzero-length-bounds warnings

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 854 bytes --]

On Tue, Mar 30, 2021 at 11:00:36AM +0200, Arnd Bergmann <arnd-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> Would it be possible to enclose most or all of kernel/cgroup/cgroup.c
> in an #ifdef CGROUP_SUBSYS_COUNT block?
Even without any controllers, there can still be named hierarchies (v1)
or the default hierarchy (v2) (for instance) for process tracking
purposes. So only parts of kernel/cgroup/cgroup.c could be ifdef'd.

Beware that CGROUP_SUBSYS_COUNT is not known at preprocessing stage (you
could have a macro alternative though).

> I didn't try that myself, but this might be a way to guarantee that
> there cannot be any callers (it would cause a link error).
Such a guarantee would be nicer, I agree. I tried a bit but anandoned it
when I saw macros proliferate (which I found less readable than your
current variant). But YMMV.

Michal

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 07/11] ARM: sharpsl_param: work around -Wstringop-overread warning

[Arnd Bergmann]

From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>

gcc warns that accessing a pointer based on a numeric constant may
be an offset into a NULL pointer, and would therefore has zero
accessible bytes:

arch/arm/common/sharpsl_param.c: In function ‘sharpsl_save_param’:
arch/arm/common/sharpsl_param.c:43:9: error: ‘memcpy’ reading 64 bytes from a region of size 0 [-Werror=stringop-overread]
   43 |         memcpy(&sharpsl_param, param_start(PARAM_BASE), sizeof(struct sharpsl_param_info));
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

In this particular case, the warning is bogus since this is the actual
pointer, not an offset on a NULL pointer. Add a local variable to shut
up the warning and hope it doesn't come back.

Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
---
 arch/arm/common/sharpsl_param.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm/common/sharpsl_param.c b/arch/arm/common/sharpsl_param.c
index efeb5724d9e9..6237ede2f0c7 100644
--- a/arch/arm/common/sharpsl_param.c
+++ b/arch/arm/common/sharpsl_param.c
@@ -40,7 +40,9 @@ EXPORT_SYMBOL(sharpsl_param);
 
 void sharpsl_save_param(void)
 {
-	memcpy(&sharpsl_param, param_start(PARAM_BASE), sizeof(struct sharpsl_param_info));
+	struct sharpsl_param_info *params = param_start(PARAM_BASE);
+
+	memcpy(&sharpsl_param, params, sizeof(*params));
 
 	if (sharpsl_param.comadj_keyword != COMADJ_MAGIC)
 		sharpsl_param.comadj=-1;
-- 
2.29.2

[PATCH 11/11] [RFC] drm/i915/dp: fix array overflow warning

[Arnd Bergmann]

From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>

gcc-11 warns that intel_dp_check_mst_status() has a local array of
fourteen bytes and passes the last four bytes into a function that
expects a six-byte array:

drivers/gpu/drm/i915/display/intel_dp.c: In function ‘intel_dp_check_mst_status’:
drivers/gpu/drm/i915/display/intel_dp.c:4556:22: error: ‘drm_dp_channel_eq_ok’ reading 6 bytes from a region of size 4 [-Werror=stringop-overread]
 4556 |                     !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
      |                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/i915/display/intel_dp.c:4556:22: note: referencing argument 1 of type ‘const u8 *’ {aka ‘const unsigned char *’}
In file included from drivers/gpu/drm/i915/display/intel_dp.c:38:
include/drm/drm_dp_helper.h:1459:6: note: in a call to function ‘drm_dp_channel_eq_ok’
 1459 | bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE],
      |      ^~~~~~~~~~~~~~~~~~~~

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.

Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 8c12d5375607..830e2515f119 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -65,7 +65,7 @@
 #include "intel_vdsc.h"
 #include "intel_vrr.h"
 
-#define DP_DPRX_ESI_LEN 14
+#define DP_DPRX_ESI_LEN 16
 
 /* DP DSC throughput values used for slice count calculations KPixels/s */
 #define DP_DSC_PEAK_PIXEL_RATE			2720000
-- 
2.29.2

Re: [PATCH 11/11] [RFC] drm/i915/dp: fix array overflow warning

[Jani Nikula]

On Mon, 22 Mar 2021, Arnd Bergmann <arnd@kernel.org> wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> gcc-11 warns that intel_dp_check_mst_status() has a local array of
> fourteen bytes and passes the last four bytes into a function that
> expects a six-byte array:
>
> drivers/gpu/drm/i915/display/intel_dp.c: In function ‘intel_dp_check_mst_status’:
> drivers/gpu/drm/i915/display/intel_dp.c:4556:22: error: ‘drm_dp_channel_eq_ok’ reading 6 bytes from a region of size 4 [-Werror=stringop-overread]
>  4556 |                     !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
>       |                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/i915/display/intel_dp.c:4556:22: note: referencing argument 1 of type ‘const u8 *’ {aka ‘const unsigned char *’}
> In file included from drivers/gpu/drm/i915/display/intel_dp.c:38:
> include/drm/drm_dp_helper.h:1459:6: note: in a call to function ‘drm_dp_channel_eq_ok’
>  1459 | bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE],
>       |      ^~~~~~~~~~~~~~~~~~~~
>
> 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?

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().


BR,
Jani.


>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 8c12d5375607..830e2515f119 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -65,7 +65,7 @@
>  #include "intel_vdsc.h"
>  #include "intel_vrr.h"
>  
> -#define DP_DPRX_ESI_LEN 14
> +#define DP_DPRX_ESI_LEN 16
>  
>  /* DP DSC throughput values used for slice count calculations KPixels/s */
>  #define DP_DSC_PEAK_PIXEL_RATE			2720000

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/11] [RFC] drm/i915/dp: fix array overflow warning

[Arnd Bergmann]

On Thu, Mar 25, 2021 at 9:05 AM Jani Nikula <jani.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> 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.

> 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-r2nGTMty4D4@public.gmane.org>.

       Arnd

Re: [PATCH 11/11] [RFC] drm/i915/dp: fix array overflow warning

[Martin Sebor]

On 3/25/21 3:53 AM, Arnd Bergmann wrote:
> On Thu, Mar 25, 2021 at 9:05 AM Jani Nikula <jani.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> 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-r2nGTMty4D4@public.gmane.org>.
> 
>         Arnd
> 

Re: [PATCH 11/11] [RFC] drm/i915/dp: fix array overflow warning

[Hans de Goede]

Hi,

On 3/22/21 5:02 PM, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> gcc-11 warns that intel_dp_check_mst_status() has a local array of
> fourteen bytes and passes the last four bytes into a function that
> expects a six-byte array:
> 
> drivers/gpu/drm/i915/display/intel_dp.c: In function ‘intel_dp_check_mst_status’:
> drivers/gpu/drm/i915/display/intel_dp.c:4556:22: error: ‘drm_dp_channel_eq_ok’ reading 6 bytes from a region of size 4 [-Werror=stringop-overread]
>  4556 |                     !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
>       |                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/i915/display/intel_dp.c:4556:22: note: referencing argument 1 of type ‘const u8 *’ {aka ‘const unsigned char *’}
> In file included from drivers/gpu/drm/i915/display/intel_dp.c:38:
> include/drm/drm_dp_helper.h:1459:6: note: in a call to function ‘drm_dp_channel_eq_ok’
>  1459 | bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE],
>       |      ^~~~~~~~~~~~~~~~~~~~
> 
> 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.

The drm displayport-helpers indeed expect a 6 bytes buffer, but they
usually only consume 4 bytes.

I don't think that changing the DP_DPRX_ESI_LEN is a good fix here,
since it is used in multiple places, but the esi array already gets
zero-ed out by its initializer, so we can just pass 2 extra 0 bytes
to give drm_dp_channel_eq_ok() call the 6 byte buffer its prototype
specifies by doing this:

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 897711d9d7d3..147962d4ad06 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4538,7 +4538,11 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
 	drm_WARN_ON_ONCE(&i915->drm, intel_dp->active_mst_links < 0);
 
 	for (;;) {
-		u8 esi[DP_DPRX_ESI_LEN] = {};
+		/*
+		 * drm_dp_channel_eq_ok() expects a 6 byte large buffer, but
+		 * the ESI info only contains 4 bytes, pass 2 extra 0 bytes.
+		 */
+		u8 esi[DP_DPRX_ESI_LEN + 2] = {};
 		bool handled;
 		int retry;
 

So i915 devs, would such a fix be acceptable ?

Regards,

Hans






> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 8c12d5375607..830e2515f119 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -65,7 +65,7 @@
>  #include "intel_vdsc.h"
>  #include "intel_vrr.h"
>  
> -#define DP_DPRX_ESI_LEN 14
> +#define DP_DPRX_ESI_LEN 16
>  
>  /* DP DSC throughput values used for slice count calculations KPixels/s */
>  #define DP_DSC_PEAK_PIXEL_RATE			2720000
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 02/11] x86: tboot: avoid Wstringop-overread-warning

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

gcc-11 warns about using string operations on pointers that are
defined at compile time as offsets from a NULL pointer. Unfortunately
that also happens on the result of fix_to_virt(), which is a
compile-time constant for a constantn input:

arch/x86/kernel/tboot.c: In function 'tboot_probe':
arch/x86/kernel/tboot.c:70:13: error: '__builtin_memcmp_eq' specified bound 16 exceeds source size 0 [-Werror=stringop-overread]
   70 |         if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) {
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I hope this can get addressed in gcc-11 before the release.

As a workaround, split up the tboot_probe() function in two halves
to separate the pointer generation from the usage. This is a bit
ugly, and hopefully gcc understands that the code is actually correct
before it learns to peek into the noinline function.

Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/x86/kernel/tboot.c | 44 ++++++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c
index 4c09ba110204..f9af561c3cd4 100644
--- a/arch/x86/kernel/tboot.c
+++ b/arch/x86/kernel/tboot.c
@@ -49,6 +49,30 @@ bool tboot_enabled(void)
 	return tboot != NULL;
 }
 
+/* noinline to prevent gcc from warning about dereferencing constant fixaddr */
+static noinline __init bool check_tboot_version(void)
+{
+	if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) {
+		pr_warn("tboot at 0x%llx is invalid\n", boot_params.tboot_addr);
+		return false;
+	}
+
+	if (tboot->version < 5) {
+		pr_warn("tboot version is invalid: %u\n", tboot->version);
+		return false;
+	}
+
+	pr_info("found shared page at phys addr 0x%llx:\n",
+		boot_params.tboot_addr);
+	pr_debug("version: %d\n", tboot->version);
+	pr_debug("log_addr: 0x%08x\n", tboot->log_addr);
+	pr_debug("shutdown_entry: 0x%x\n", tboot->shutdown_entry);
+	pr_debug("tboot_base: 0x%08x\n", tboot->tboot_base);
+	pr_debug("tboot_size: 0x%x\n", tboot->tboot_size);
+
+	return true;
+}
+
 void __init tboot_probe(void)
 {
 	/* Look for valid page-aligned address for shared page. */
@@ -66,25 +90,9 @@ void __init tboot_probe(void)
 
 	/* Map and check for tboot UUID. */
 	set_fixmap(FIX_TBOOT_BASE, boot_params.tboot_addr);
-	tboot = (struct tboot *)fix_to_virt(FIX_TBOOT_BASE);
-	if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) {
-		pr_warn("tboot at 0x%llx is invalid\n", boot_params.tboot_addr);
+	tboot = (void *)fix_to_virt(FIX_TBOOT_BASE);
+	if (!check_tboot_version())
 		tboot = NULL;
-		return;
-	}
-	if (tboot->version < 5) {
-		pr_warn("tboot version is invalid: %u\n", tboot->version);
-		tboot = NULL;
-		return;
-	}
-
-	pr_info("found shared page at phys addr 0x%llx:\n",
-		boot_params.tboot_addr);
-	pr_debug("version: %d\n", tboot->version);
-	pr_debug("log_addr: 0x%08x\n", tboot->log_addr);
-	pr_debug("shutdown_entry: 0x%x\n", tboot->shutdown_entry);
-	pr_debug("tboot_base: 0x%08x\n", tboot->tboot_base);
-	pr_debug("tboot_size: 0x%x\n", tboot->tboot_size);
 }
 
 static pgd_t *tboot_pg_dir;
-- 
2.29.2

Re: [PATCH 02/11] x86: tboot: avoid Wstringop-overread-warning

[Ingo Molnar]

* Arnd Bergmann <arnd@kernel.org> wrote:

> From: Arnd Bergmann <arnd@arndb.de>
> 
> gcc-11 warns about using string operations on pointers that are
> defined at compile time as offsets from a NULL pointer. Unfortunately
> that also happens on the result of fix_to_virt(), which is a
> compile-time constant for a constantn input:
> 
> arch/x86/kernel/tboot.c: In function 'tboot_probe':
> arch/x86/kernel/tboot.c:70:13: error: '__builtin_memcmp_eq' specified bound 16 exceeds source size 0 [-Werror=stringop-overread]
>    70 |         if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) {
>       |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> I hope this can get addressed in gcc-11 before the release.
> 
> As a workaround, split up the tboot_probe() function in two halves
> to separate the pointer generation from the usage. This is a bit
> ugly, and hopefully gcc understands that the code is actually correct
> before it learns to peek into the noinline function.
> 
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  arch/x86/kernel/tboot.c | 44 ++++++++++++++++++++++++-----------------
>  1 file changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c
> index 4c09ba110204..f9af561c3cd4 100644
> --- a/arch/x86/kernel/tboot.c
> +++ b/arch/x86/kernel/tboot.c
> @@ -49,6 +49,30 @@ bool tboot_enabled(void)
>  	return tboot != NULL;
>  }
>  
> +/* noinline to prevent gcc from warning about dereferencing constant fixaddr */
> +static noinline __init bool check_tboot_version(void)
> +{
> +	if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) {
> +		pr_warn("tboot at 0x%llx is invalid\n", boot_params.tboot_addr);
> +		return false;
> +	}
> +
> +	if (tboot->version < 5) {
> +		pr_warn("tboot version is invalid: %u\n", tboot->version);
> +		return false;
> +	}
> +
> +	pr_info("found shared page at phys addr 0x%llx:\n",
> +		boot_params.tboot_addr);
> +	pr_debug("version: %d\n", tboot->version);
> +	pr_debug("log_addr: 0x%08x\n", tboot->log_addr);
> +	pr_debug("shutdown_entry: 0x%x\n", tboot->shutdown_entry);
> +	pr_debug("tboot_base: 0x%08x\n", tboot->tboot_base);
> +	pr_debug("tboot_size: 0x%x\n", tboot->tboot_size);
> +
> +	return true;
> +}
> +
>  void __init tboot_probe(void)
>  {
>  	/* Look for valid page-aligned address for shared page. */
> @@ -66,25 +90,9 @@ void __init tboot_probe(void)
>  
>  	/* Map and check for tboot UUID. */
>  	set_fixmap(FIX_TBOOT_BASE, boot_params.tboot_addr);
> -	tboot = (struct tboot *)fix_to_virt(FIX_TBOOT_BASE);
> -	if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) {
> -		pr_warn("tboot at 0x%llx is invalid\n", boot_params.tboot_addr);
> +	tboot = (void *)fix_to_virt(FIX_TBOOT_BASE);
> +	if (!check_tboot_version())
>  		tboot = NULL;
> -		return;
> -	}
> -	if (tboot->version < 5) {
> -		pr_warn("tboot version is invalid: %u\n", tboot->version);
> -		tboot = NULL;
> -		return;
> -	}
> -
> -	pr_info("found shared page at phys addr 0x%llx:\n",
> -		boot_params.tboot_addr);
> -	pr_debug("version: %d\n", tboot->version);
> -	pr_debug("log_addr: 0x%08x\n", tboot->log_addr);
> -	pr_debug("shutdown_entry: 0x%x\n", tboot->shutdown_entry);
> -	pr_debug("tboot_base: 0x%08x\n", tboot->tboot_base);
> -	pr_debug("tboot_size: 0x%x\n", tboot->tboot_size);

This is indeed rather ugly - and the other patch that removes a debug 
check seems counterproductive as well.

Do we know how many genuine bugs -Wstringop-overread-warning has 
caught or is about to catch?

I.e. the real workaround might be to turn off the -Wstringop-overread-warning,
until GCC-11 gets fixed?

Thanks,

	Ingo

Re: [PATCH 02/11] x86: tboot: avoid Wstringop-overread-warning

[Arnd Bergmann]

On Mon, Mar 22, 2021 at 9:29 PM Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> * Arnd Bergmann <arnd-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>

> This is indeed rather ugly - and the other patch that removes a debug
> check seems counterproductive as well.
>
> Do we know how many genuine bugs -Wstringop-overread-warning has
> caught or is about to catch?
>
> I.e. the real workaround might be to turn off the -Wstringop-overread-warning,
> until GCC-11 gets fixed?

See the [PATCH 0/11] message. The last two patches in the series are for
code that I suspect may be broken, the others are basically all false positives.

As gcc-11 is not released yet, I don't think we have to apply any of the
patches or disable the warning at the moment, but I posted all the patches
to get a better understanding on which of them should be addressed in
the kernel vs gcc.

       Arnd

Re: [PATCH 02/11] x86: tboot: avoid Wstringop-overread-warning

[Martin Sebor]

On 3/22/21 2:29 PM, Ingo Molnar wrote:
> 
> * Arnd Bergmann <arnd-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
>> From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
>>
>> gcc-11 warns about using string operations on pointers that are
>> defined at compile time as offsets from a NULL pointer. Unfortunately
>> that also happens on the result of fix_to_virt(), which is a
>> compile-time constant for a constantn input:
>>
>> arch/x86/kernel/tboot.c: In function 'tboot_probe':
>> arch/x86/kernel/tboot.c:70:13: error: '__builtin_memcmp_eq' specified bound 16 exceeds source size 0 [-Werror=stringop-overread]
>>     70 |         if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) {
>>        |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> I hope this can get addressed in gcc-11 before the release.
>>
>> As a workaround, split up the tboot_probe() function in two halves
>> to separate the pointer generation from the usage. This is a bit
>> ugly, and hopefully gcc understands that the code is actually correct
>> before it learns to peek into the noinline function.
>>
>> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
>> Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
>> ---
>>   arch/x86/kernel/tboot.c | 44 ++++++++++++++++++++++++-----------------
>>   1 file changed, 26 insertions(+), 18 deletions(-)
>>
>> diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c
>> index 4c09ba110204..f9af561c3cd4 100644
>> --- a/arch/x86/kernel/tboot.c
>> +++ b/arch/x86/kernel/tboot.c
>> @@ -49,6 +49,30 @@ bool tboot_enabled(void)
>>   	return tboot != NULL;
>>   }
>>   
>> +/* noinline to prevent gcc from warning about dereferencing constant fixaddr */
>> +static noinline __init bool check_tboot_version(void)
>> +{
>> +	if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) {
>> +		pr_warn("tboot at 0x%llx is invalid\n", boot_params.tboot_addr);
>> +		return false;
>> +	}
>> +
>> +	if (tboot->version < 5) {
>> +		pr_warn("tboot version is invalid: %u\n", tboot->version);
>> +		return false;
>> +	}
>> +
>> +	pr_info("found shared page at phys addr 0x%llx:\n",
>> +		boot_params.tboot_addr);
>> +	pr_debug("version: %d\n", tboot->version);
>> +	pr_debug("log_addr: 0x%08x\n", tboot->log_addr);
>> +	pr_debug("shutdown_entry: 0x%x\n", tboot->shutdown_entry);
>> +	pr_debug("tboot_base: 0x%08x\n", tboot->tboot_base);
>> +	pr_debug("tboot_size: 0x%x\n", tboot->tboot_size);
>> +
>> +	return true;
>> +}
>> +
>>   void __init tboot_probe(void)
>>   {
>>   	/* Look for valid page-aligned address for shared page. */
>> @@ -66,25 +90,9 @@ void __init tboot_probe(void)
>>   
>>   	/* Map and check for tboot UUID. */
>>   	set_fixmap(FIX_TBOOT_BASE, boot_params.tboot_addr);
>> -	tboot = (struct tboot *)fix_to_virt(FIX_TBOOT_BASE);
>> -	if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) {
>> -		pr_warn("tboot at 0x%llx is invalid\n", boot_params.tboot_addr);
>> +	tboot = (void *)fix_to_virt(FIX_TBOOT_BASE);
>> +	if (!check_tboot_version())
>>   		tboot = NULL;
>> -		return;
>> -	}
>> -	if (tboot->version < 5) {
>> -		pr_warn("tboot version is invalid: %u\n", tboot->version);
>> -		tboot = NULL;
>> -		return;
>> -	}
>> -
>> -	pr_info("found shared page at phys addr 0x%llx:\n",
>> -		boot_params.tboot_addr);
>> -	pr_debug("version: %d\n", tboot->version);
>> -	pr_debug("log_addr: 0x%08x\n", tboot->log_addr);
>> -	pr_debug("shutdown_entry: 0x%x\n", tboot->shutdown_entry);
>> -	pr_debug("tboot_base: 0x%08x\n", tboot->tboot_base);
>> -	pr_debug("tboot_size: 0x%x\n", tboot->tboot_size);
> 
> This is indeed rather ugly - and the other patch that removes a debug
> check seems counterproductive as well.
> 
> Do we know how many genuine bugs -Wstringop-overread-warning has
> caught or is about to catch?
> 
> I.e. the real workaround might be to turn off the -Wstringop-overread-warning,
> until GCC-11 gets fixed?

In GCC 10 -Wstringop-overread is a subset of -Wstringop-overflow.
GCC 11 breaks it out as a separate warning to make it easier to
control.  Both warnings have caught some real bugs but they both
have a nonzero rate of false positives.  Other than bug reports
we don't have enough data to say what their S/N ratio might be
but my sense is that it's fairly high in general.

   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=wstringop-overread
   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=wstringop-overflow

In GCC 11, all access warnings expect objects to be either declared
or allocated.  Pointers with constant values are taken to point to
nothing valid (as Arnd mentioned above, this is to detect invalid
accesses to members of structs at address zero).

One possible solution to the known address problem is to extend GCC
attributes address and io that pin an object to a hardwired address
to all targets (at the moment they're supported on just one or two
targets).  I'm not sure this can still happen before GCC 11 releases
sometime in April or May.

Until then, another workaround is to convert the fixed address to
a volatile pointer before using it for the access, along the lines
below.  It should have only a negligible effect on efficiency.

diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c
index 4c09ba110204..76326b906010 100644
--- a/arch/x86/kernel/tboot.c
+++ b/arch/x86/kernel/tboot.c
@@ -67,7 +67,9 @@ void __init tboot_probe(void)
         /* Map and check for tboot UUID. */
         set_fixmap(FIX_TBOOT_BASE, boot_params.tboot_addr);
         tboot = (struct tboot *)fix_to_virt(FIX_TBOOT_BASE);
-       if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) {
+       if (memcmp(&tboot_uuid,
+                  (*(struct tboot* volatile *)(&tboot))->uuid,
+                  sizeof(tboot->uuid))) {
                 pr_warn("tboot at 0x%llx is invalid\n", 
boot_params.tboot_addr);
                 tboot = NULL;
                 return;

Martin

> 
> Thanks,
> 
> 	Ingo
> 

Re: [PATCH 02/11] x86: tboot: avoid Wstringop-overread-warning

[Arnd Bergmann]

On Mon, Mar 22, 2021 at 11:09 PM Martin Sebor <msebor-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 3/22/21 2:29 PM, Ingo Molnar wrote:
> > * Arnd Bergmann <arnd-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >
> > I.e. the real workaround might be to turn off the -Wstringop-overread-warning,
> > until GCC-11 gets fixed?
>
> In GCC 10 -Wstringop-overread is a subset of -Wstringop-overflow.
> GCC 11 breaks it out as a separate warning to make it easier to
> control.  Both warnings have caught some real bugs but they both
> have a nonzero rate of false positives.  Other than bug reports
> we don't have enough data to say what their S/N ratio might be
> but my sense is that it's fairly high in general.
>
>    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=wstringop-overread
>    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=wstringop-overflow

Unfortunately, stringop-overflow is one of the warnings that is
completely disabled in the kernel at the moment, rather than
enabled at one of the user-selectable higher warning levels.

I have a patch series to change that and to pull some of these
into the lower W=1 or W=2 levels or even enable them by default.

To do this though, I first need to ensure that the actual output
is empty for the normal builds. I added -Wstringop-overflow to
the list of warnings I wanted to address because it is a new
warning and only has a dozen or so occurrences throughout the
kernel.

> In GCC 11, all access warnings expect objects to be either declared
> or allocated.  Pointers with constant values are taken to point to
> nothing valid (as Arnd mentioned above, this is to detect invalid
> accesses to members of structs at address zero).
>
> One possible solution to the known address problem is to extend GCC
> attributes address and io that pin an object to a hardwired address
> to all targets (at the moment they're supported on just one or two
> targets).  I'm not sure this can still happen before GCC 11 releases
> sometime in April or May.
>
> Until then, another workaround is to convert the fixed address to
> a volatile pointer before using it for the access, along the lines
> below.  It should have only a negligible effect on efficiency.
>
> diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c
> index 4c09ba110204..76326b906010 100644
> --- a/arch/x86/kernel/tboot.c
> +++ b/arch/x86/kernel/tboot.c
> @@ -67,7 +67,9 @@ void __init tboot_probe(void)
>          /* Map and check for tboot UUID. */
>          set_fixmap(FIX_TBOOT_BASE, boot_params.tboot_addr);
>          tboot = (struct tboot *)fix_to_virt(FIX_TBOOT_BASE);
> -       if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) {
> +       if (memcmp(&tboot_uuid,
> +                  (*(struct tboot* volatile *)(&tboot))->uuid,
> +                  sizeof(tboot->uuid))) {
>                  pr_warn("tboot at 0x%llx is invalid\n",

I think a stray 'volatile' would raise too many red flags here, but
I checked that the RELOC_HIDE() macro has the same effect, e.g.

@@ -66,7 +67,8 @@ void __init tboot_probe(void)

        /* Map and check for tboot UUID. */
        set_fixmap(FIX_TBOOT_BASE, boot_params.tboot_addr);
-       tboot = (struct tboot *)fix_to_virt(FIX_TBOOT_BASE);
+       /* RELOC_HIDE to prevent gcc warnings about NULL pointer */
+       tboot = RELOC_HIDE(NULL, fix_to_virt(FIX_TBOOT_BASE));
        if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) {
                pr_warn("tboot at 0x%llx is invalid\n", boot_params.tboot_addr);
                tboot = NULL;

     Arnd

Re: [PATCH 02/11] x86: tboot: avoid Wstringop-overread-warning

[Ingo Molnar]

* Martin Sebor <msebor@gmail.com> wrote:

> > I.e. the real workaround might be to turn off the -Wstringop-overread-warning,
> > until GCC-11 gets fixed?
> 
> In GCC 10 -Wstringop-overread is a subset of -Wstringop-overflow.
> GCC 11 breaks it out as a separate warning to make it easier to
> control.  Both warnings have caught some real bugs but they both
> have a nonzero rate of false positives.  Other than bug reports
> we don't have enough data to say what their S/N ratio might be
> but my sense is that it's fairly high in general.
> 
>   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=wstringop-overread
>   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=wstringop-overflow
> 
> In GCC 11, all access warnings expect objects to be either declared
> or allocated.  Pointers with constant values are taken to point to
> nothing valid (as Arnd mentioned above, this is to detect invalid
> accesses to members of structs at address zero).
> 
> One possible solution to the known address problem is to extend GCC
> attributes address and io that pin an object to a hardwired address
> to all targets (at the moment they're supported on just one or two
> targets).  I'm not sure this can still happen before GCC 11 releases
> sometime in April or May.
> 
> Until then, another workaround is to convert the fixed address to
> a volatile pointer before using it for the access, along the lines
> below.  It should have only a negligible effect on efficiency.

Thank you for the detailed answer!

I think I'll go with Arnd's original patch - which makes the code a 
slightly bit cleaner by separating out the check_tboot_version() check 
into a standalone function.

The only ugly aspect is the global nature of the 'tboot' pointer - but 
that's a self-inflicted wound.

I'd also guess that the S/N ratio somewhat unfairly penalizes this 
warning right now, because the kernel had a decade of growing real 
fixes via other efforts such as static and dynamic instrumentation as 
well.

So the probability of false positive remaining is in fact higher, and 
going forward we should see a better S/N ratio of this warning. Most 
of which will never be seen by upstream maintainers, as the mishaps 
will stay at the individual developer level. :-)

Thanks,

	Ingo

RE: [PATCH 02/11] x86: tboot: avoid Wstringop-overread-warning

[David Laight]

From: Martin Sebor
> Sent: 22 March 2021 22:08
...
> In GCC 11, all access warnings expect objects to be either declared
> or allocated.  Pointers with constant values are taken to point to
> nothing valid (as Arnd mentioned above, this is to detect invalid
> accesses to members of structs at address zero).
> 
> One possible solution to the known address problem is to extend GCC
> attributes address and io that pin an object to a hardwired address
> to all targets (at the moment they're supported on just one or two
> targets).  I'm not sure this can still happen before GCC 11 releases
> sometime in April or May.

A different solution is to define a normal C external data item
and then assign a fixed address with an asm statement or in
the linker script.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 02/11] x86: tboot: avoid Wstringop-overread-warning

[David Laight]

From: David Laight
> Sent: 24 March 2021 09:12
> 
> From: Martin Sebor
> > Sent: 22 March 2021 22:08
> ...
> > In GCC 11, all access warnings expect objects to be either declared
> > or allocated.  Pointers with constant values are taken to point to
> > nothing valid (as Arnd mentioned above, this is to detect invalid
> > accesses to members of structs at address zero).
> >
> > One possible solution to the known address problem is to extend GCC
> > attributes address and io that pin an object to a hardwired address
> > to all targets (at the moment they're supported on just one or two
> > targets).  I'm not sure this can still happen before GCC 11 releases
> > sometime in April or May.
> 
> A different solution is to define a normal C external data item
> and then assign a fixed address with an asm statement or in
> the linker script.

Or stop gcc tracking the value by using:
	struct foo *foo = (void *)xxxxx;
	asm ("", "+r" (foo));

If the address is used more than once forcing it into
a register is also likely to generate better code.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

[PATCH 03/11] security: commoncap: fix -Wstringop-overread warning

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

gcc-11 introdces a harmless warning for cap_inode_getsecurity:

security/commoncap.c: In function ‘cap_inode_getsecurity’:
security/commoncap.c:440:33: error: ‘memcpy’ reading 16 bytes from a region of size 0 [-Werror=stringop-overread]
  440 |                                 memcpy(&nscap->data, &cap->data, sizeof(__le32) * 2 * VFS_CAP_U32);
      |                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The problem here is that tmpbuf is initialized to NULL, so gcc assumes
it is not accessible unless it gets set by vfs_getxattr_alloc().  This is
a legitimate warning as far as I can tell, but the code is correct since
it correctly handles the error when that function fails.

Add a separate NULL check to tell gcc about it as well.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 security/commoncap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index 28f4d25480df..9a36ed6dd737 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -400,7 +400,7 @@ int cap_inode_getsecurity(struct user_namespace *mnt_userns,
 				      &tmpbuf, size, GFP_NOFS);
 	dput(dentry);
 
-	if (ret < 0)
+	if (ret < 0 || !tmpbuf)
 		return ret;
 
 	fs_ns = inode->i_sb->s_user_ns;
-- 
2.29.2

Re: [PATCH 03/11] security: commoncap: fix -Wstringop-overread warning

[Christian Brauner]

On Mon, Mar 22, 2021 at 05:02:41PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> 
> gcc-11 introdces a harmless warning for cap_inode_getsecurity:
> 
> security/commoncap.c: In function ‘cap_inode_getsecurity’:
> security/commoncap.c:440:33: error: ‘memcpy’ reading 16 bytes from a region of size 0 [-Werror=stringop-overread]
>   440 |                                 memcpy(&nscap->data, &cap->data, sizeof(__le32) * 2 * VFS_CAP_U32);
>       |                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> The problem here is that tmpbuf is initialized to NULL, so gcc assumes
> it is not accessible unless it gets set by vfs_getxattr_alloc().  This is
> a legitimate warning as far as I can tell, but the code is correct since
> it correctly handles the error when that function fails.
> 
> Add a separate NULL check to tell gcc about it as well.
> 
> Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> ---

Seems reasonable,
Acked-by: Christian Brauner <christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 03/11] security: commoncap: fix -Wstringop-overread warning

[James Morris]

[-- Attachment #1: Type: text/plain, Size: 1158 bytes --]

On Mon, 22 Mar 2021, Arnd Bergmann wrote:

> From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> 
> gcc-11 introdces a harmless warning for cap_inode_getsecurity:
> 
> security/commoncap.c: In function ‘cap_inode_getsecurity’:
> security/commoncap.c:440:33: error: ‘memcpy’ reading 16 bytes from a region of size 0 [-Werror=stringop-overread]
>   440 |                                 memcpy(&nscap->data, &cap->data, sizeof(__le32) * 2 * VFS_CAP_U32);
>       |                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> The problem here is that tmpbuf is initialized to NULL, so gcc assumes
> it is not accessible unless it gets set by vfs_getxattr_alloc().  This is
> a legitimate warning as far as I can tell, but the code is correct since
> it correctly handles the error when that function fails.
> 
> Add a separate NULL check to tell gcc about it as well.
> 
> Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>

Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git fixes-v5.12

-- 
James Morris
<jmorris-gx6/JNMH7DfYtjvyW6yDsg@public.gmane.org>

[PATCH 08/11] atmel: avoid gcc -Wstringop-overflow warning

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

gcc-11 notices that the fields as defined in the ass_req_format
structure do not match the actual use of that structure:

cc1: error: writing 4 bytes into a region of size between 18446744073709551613 and 2 [-Werror=stringop-overflow=]
drivers/net/wireless/atmel/atmel.c:2884:20: note: at offset [4, 6] into destination object ‘ap’ of size 6
 2884 |                 u8 ap[ETH_ALEN]; /* nothing after here directly accessible */
      |                    ^~
drivers/net/wireless/atmel/atmel.c:2885:20: note: at offset [4, 6] into destination object ‘ssid_el_id’ of size 1
 2885 |                 u8 ssid_el_id;
      |                    ^~~~~~~~~~

This is expected here because the actual structure layout is variable.
As the code does not actually access the individual fields, replace
them with a comment and fixed-length array so prevent gcc from
complaining about it.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/net/wireless/atmel/atmel.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/atmel/atmel.c b/drivers/net/wireless/atmel/atmel.c
index 707fe66727f8..ff9152d600e1 100644
--- a/drivers/net/wireless/atmel/atmel.c
+++ b/drivers/net/wireless/atmel/atmel.c
@@ -2881,13 +2881,18 @@ static void send_association_request(struct atmel_private *priv, int is_reassoc)
 	struct ass_req_format {
 		__le16 capability;
 		__le16 listen_interval;
-		u8 ap[ETH_ALEN]; /* nothing after here directly accessible */
-		u8 ssid_el_id;
-		u8 ssid_len;
-		u8 ssid[MAX_SSID_LENGTH];
-		u8 sup_rates_el_id;
-		u8 sup_rates_len;
-		u8 rates[4];
+		u8 ssid_el[ETH_ALEN + 2 + MAX_SSID_LENGTH + 2 + 4];
+		/*
+		 * nothing after here directly accessible:
+		 *
+		 * u8 ap[ETH_ALEN];
+		 * u8 ssid_el_id;
+		 * u8 ssid_len;
+		 * u8 ssid[MAX_SSID_LENGTH];
+		 * u8 sup_rates_el_id;
+		 * u8 sup_rates_len;
+		 * u8 rates[4];
+		 */
 	} body;
 
 	header.frame_control = cpu_to_le16(IEEE80211_FTYPE_MGMT |
@@ -2907,13 +2912,13 @@ static void send_association_request(struct atmel_private *priv, int is_reassoc)
 
 	body.listen_interval = cpu_to_le16(priv->listen_interval * priv->beacon_period);
 
+	ssid_el_p = body.ssid_el;
 	/* current AP address - only in reassoc frame */
 	if (is_reassoc) {
-		memcpy(body.ap, priv->CurrentBSSID, ETH_ALEN);
-		ssid_el_p = &body.ssid_el_id;
+		memcpy(ssid_el_p, priv->CurrentBSSID, ETH_ALEN);
+		ssid_el_p += ETH_ALEN;
 		bodysize = 18 + priv->SSID_size;
 	} else {
-		ssid_el_p = &body.ap[0];
 		bodysize = 12 + priv->SSID_size;
 	}
 
-- 
2.29.2

[PATCH 09/11] scsi: lpfc: fix gcc -Wstringop-overread warning

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

gcc-11 warns about an strnlen with a length larger than the size of the
passed buffer:

drivers/scsi/lpfc/lpfc_attr.c: In function 'lpfc_nvme_info_show':
drivers/scsi/lpfc/lpfc_attr.c:518:25: error: 'strnlen' specified bound 4095 exceeds source size 24 [-Werror=stringop-overread]
  518 |                         strnlen(LPFC_NVME_INFO_MORE_STR, PAGE_SIZE - 1)
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

In this case, the code is entirely valid, as the string is properly
terminated, and the size argument is only there out of extra caution
in case it exceeds a page.

This cannot really happen here, so just simplify it to a sizeof().

Fixes: afff0d2321ea ("scsi: lpfc: Add Buffer overflow check, when nvme_info larger than PAGE_SIZE")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/scsi/lpfc/lpfc_attr.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_attr.c b/drivers/scsi/lpfc/lpfc_attr.c
index bdd9a29f4201..f6d886f9dfb3 100644
--- a/drivers/scsi/lpfc/lpfc_attr.c
+++ b/drivers/scsi/lpfc/lpfc_attr.c
@@ -512,11 +512,9 @@ lpfc_nvme_info_show(struct device *dev, struct device_attribute *attr,
 				"6314 Catching potential buffer "
 				"overflow > PAGE_SIZE = %lu bytes\n",
 				PAGE_SIZE);
-		strlcpy(buf + PAGE_SIZE - 1 -
-			strnlen(LPFC_NVME_INFO_MORE_STR, PAGE_SIZE - 1),
+		strlcpy(buf + PAGE_SIZE - 1 - sizeof(LPFC_NVME_INFO_MORE_STR),
 			LPFC_NVME_INFO_MORE_STR,
-			strnlen(LPFC_NVME_INFO_MORE_STR, PAGE_SIZE - 1)
-			+ 1);
+			sizeof(LPFC_NVME_INFO_MORE_STR) + 1);
 	}
 
 	return len;
-- 
2.29.2

[PATCH 10/11] drm/i915: avoid stringop-overread warning on pri_latency

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

gcc-11 warns about what appears to be an out-of-range array access:

In function ‘snb_wm_latency_quirk’,
    inlined from ‘ilk_setup_wm_latency’ at drivers/gpu/drm/i915/intel_pm.c:3108:3:
drivers/gpu/drm/i915/intel_pm.c:3057:9: error: ‘intel_print_wm_latency’ reading 16 bytes from a region of size 10 [-Werror=stringop-overread]
 3057 |         intel_print_wm_latency(dev_priv, "Primary", dev_priv->wm.pri_latency);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/i915/intel_pm.c: In function ‘ilk_setup_wm_latency’:
drivers/gpu/drm/i915/intel_pm.c:3057:9: note: referencing argument 3 of type ‘const u16 *’ {aka ‘const short unsigned int *’}
drivers/gpu/drm/i915/intel_pm.c:2994:13: note: in a call to function ‘intel_print_wm_latency’
 2994 | static void intel_print_wm_latency(struct drm_i915_private *dev_priv,
      |             ^~~~~~~~~~~~~~~~~~~~~~

My guess is that this code is actually safe because the size of the
array depends on the hardware generation, and the function checks for
that, but at the same time I would not expect the compiler to work it
out correctly, and the code seems a little fragile with regards to
future changes. Simply increasing the size of the array should help.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/i915/i915_drv.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 26d69d06aa6d..3567602e0a35 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1095,11 +1095,11 @@ struct drm_i915_private {
 		 * in 0.5us units for WM1+.
 		 */
 		/* primary */
-		u16 pri_latency[5];
+		u16 pri_latency[8];
 		/* sprite */
-		u16 spr_latency[5];
+		u16 spr_latency[8];
 		/* cursor */
-		u16 cur_latency[5];
+		u16 cur_latency[8];
 		/*
 		 * Raw watermark memory latency values
 		 * for SKL for all 8 levels
-- 
2.29.2

Re: [PATCH 10/11] drm/i915: avoid stringop-overread warning on pri_latency

[Jani Nikula]

On Mon, 22 Mar 2021, Arnd Bergmann <arnd@kernel.org> wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> gcc-11 warns about what appears to be an out-of-range array access:
>
> In function ‘snb_wm_latency_quirk’,
>     inlined from ‘ilk_setup_wm_latency’ at drivers/gpu/drm/i915/intel_pm.c:3108:3:
> drivers/gpu/drm/i915/intel_pm.c:3057:9: error: ‘intel_print_wm_latency’ reading 16 bytes from a region of size 10 [-Werror=stringop-overread]
>  3057 |         intel_print_wm_latency(dev_priv, "Primary", dev_priv->wm.pri_latency);
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/i915/intel_pm.c: In function ‘ilk_setup_wm_latency’:
> drivers/gpu/drm/i915/intel_pm.c:3057:9: note: referencing argument 3 of type ‘const u16 *’ {aka ‘const short unsigned int *’}
> drivers/gpu/drm/i915/intel_pm.c:2994:13: note: in a call to function ‘intel_print_wm_latency’
>  2994 | static void intel_print_wm_latency(struct drm_i915_private *dev_priv,
>       |             ^~~~~~~~~~~~~~~~~~~~~~
>
> My guess is that this code is actually safe because the size of the
> array depends on the hardware generation, and the function checks for
> that, but at the same time I would not expect the compiler to work it
> out correctly, and the code seems a little fragile with regards to
> future changes. Simply increasing the size of the array should help.

Agreed, I don't think there's an issue, but the code could use a bunch
of improvements.

Like, we have intel_print_wm_latency() for debug logging and
wm_latency_show() for debugfs, and there's a bunch of duplication and
ugh.

But this seems like the easiest fix for the warning.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/gpu/drm/i915/i915_drv.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 26d69d06aa6d..3567602e0a35 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1095,11 +1095,11 @@ struct drm_i915_private {
>  		 * in 0.5us units for WM1+.
>  		 */
>  		/* primary */
> -		u16 pri_latency[5];
> +		u16 pri_latency[8];
>  		/* sprite */
> -		u16 spr_latency[5];
> +		u16 spr_latency[8];
>  		/* cursor */
> -		u16 cur_latency[5];
> +		u16 cur_latency[8];
>  		/*
>  		 * Raw watermark memory latency values
>  		 * for SKL for all 8 levels

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/11] drm/i915: avoid stringop-overread warning on pri_latency

[Ville Syrjälä]

On Wed, Mar 24, 2021 at 05:30:24PM +0200, Jani Nikula wrote:
> On Mon, 22 Mar 2021, Arnd Bergmann <arnd@kernel.org> wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > gcc-11 warns about what appears to be an out-of-range array access:
> >
> > In function ‘snb_wm_latency_quirk’,
> >     inlined from ‘ilk_setup_wm_latency’ at drivers/gpu/drm/i915/intel_pm.c:3108:3:
> > drivers/gpu/drm/i915/intel_pm.c:3057:9: error: ‘intel_print_wm_latency’ reading 16 bytes from a region of size 10 [-Werror=stringop-overread]
> >  3057 |         intel_print_wm_latency(dev_priv, "Primary", dev_priv->wm.pri_latency);
> >       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > drivers/gpu/drm/i915/intel_pm.c: In function ‘ilk_setup_wm_latency’:
> > drivers/gpu/drm/i915/intel_pm.c:3057:9: note: referencing argument 3 of type ‘const u16 *’ {aka ‘const short unsigned int *’}
> > drivers/gpu/drm/i915/intel_pm.c:2994:13: note: in a call to function ‘intel_print_wm_latency’
> >  2994 | static void intel_print_wm_latency(struct drm_i915_private *dev_priv,
> >       |             ^~~~~~~~~~~~~~~~~~~~~~
> >
> > My guess is that this code is actually safe because the size of the
> > array depends on the hardware generation, and the function checks for
> > that, but at the same time I would not expect the compiler to work it
> > out correctly, and the code seems a little fragile with regards to
> > future changes. Simply increasing the size of the array should help.
> 
> Agreed, I don't think there's an issue, but the code could use a bunch
> of improvements.
> 
> Like, we have intel_print_wm_latency() for debug logging and
> wm_latency_show() for debugfs, and there's a bunch of duplication and
> ugh.

There is all this ancient stuff in review limbo...
https://patchwork.freedesktop.org/series/50802/

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 00/11] treewide: address gcc-11 -Wstringop-overread warnings

[Martin K. Petersen]

On Mon, 22 Mar 2021 17:02:38 +0100, Arnd Bergmann wrote:

> The coming gcc release introduces a new warning for string operations
> reading beyond the end of a fixed-length object. After testing
> randconfig kernels for a while, think I have patches for any such
> warnings that came up on x86, arm and arm64.
> 
> Most of these warnings are false-positive ones, either gcc warning
> about something that is entirely correct, or about something that
> looks suspicious but turns out to be correct after all.
> 
> [...]

Applied to 5.13/scsi-queue, thanks!

[09/11] scsi: lpfc: fix gcc -Wstringop-overread warning
        https://git.kernel.org/mkp/scsi/c/ada48ba70f6b

-- 
Martin K. Petersen	Oracle Linux Engineering