| Message ID | 20210322160253.4032422-1-arnd@kernel.org |
|---|---|
| Headers | show |
| Series | treewide: address gcc-11 -Wstringop-overread warnings | expand |
On Mon, Mar 22, 2021 at 05:02:41PM +0100, Arnd Bergmann wrote: > 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> > --- Seems reasonable, Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
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
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/
On Mon, 22 Mar 2021, Arnd Bergmann wrote: > 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> Applied to git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git fixes-v5.12
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 >
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
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