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
From: Arnd Bergmann arnd@arndb.de
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@arndb.de --- 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
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;
* 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
On Mon, Mar 22, 2021 at 9:29 PM Ingo Molnar mingo@kernel.org wrote:
- Arnd Bergmann arnd@kernel.org wrote:
From: Arnd Bergmann arnd@arndb.de
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
On 3/22/21 2:29 PM, Ingo Molnar wrote:
- 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?
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
On Mon, Mar 22, 2021 at 11:09 PM Martin Sebor msebor@gmail.com wrote:
On 3/22/21 2:29 PM, Ingo Molnar wrote:
- Arnd Bergmann arnd@kernel.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
* 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
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)
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)
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;
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 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
From: Arnd Bergmann arnd@arndb.de
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 --- 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;
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
From: Arnd Bergmann arnd@arndb.de
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@arndb.de --- 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;
From: Arnd Bergmann arnd@arndb.de
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@arndb.de --- 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;
On Mon, Mar 22, 2021 at 05:02:44PM +0100, Arnd Bergmann arnd@kernel.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@suse.com
On Tue, Mar 30, 2021 at 10:41 AM Michal Koutný mkoutny@suse.com wrote:
On Mon, Mar 22, 2021 at 05:02:44PM +0100, Arnd Bergmann arnd@kernel.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@suse.com
Thanks
Arnd
On Tue, Mar 30, 2021 at 11:00:36AM +0200, Arnd Bergmann arnd@kernel.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
From: Arnd Bergmann arnd@arndb.de
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@arndb.de --- 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;
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; }
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;
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
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];
u16 spr_latency[5];
u16 spr_latency[8];
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/
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.
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
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
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.
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 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
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
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
dri-devel@lists.freedesktop.org
-
Arnd Bergmann -
Christian Brauner -
David Laight -
Hans de Goede -
Ingo Molnar -
James Morris -
Jani Nikula -
Kalle Valo -
Martin K. Petersen -
Martin Sebor -
Michal Koutný -
Ville Syrjälä