[PATCH] nouveau/gsp: Avoid addressing beyond end of rpc->entries
Using the end of rpc->entries[] for addressing runs into both compile-time
and run-time detection of accessing beyond the end of the array. Use the
base pointer instead, since was allocated with the additional bytes for
storing the strings. Avoids the following warning in future GCC releases
with support for __counted_by:
In function 'fortify_memcpy_chk',
inlined from 'r535_gsp_rpc_set_registry' at
../drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:1123:3:
../include/linux/fortify-string.h:553:25: error: call to
'__write_overflow_field' declared with attribute warning: detected write beyond
size of field (1st parameter); maybe use struct_group()?
[-Werror=attribute-warning]
553 | __write_overflow_field(p_size_field, size);
| ^~
for this code:
strings = (char *)>entries[NV_GSP_REG_NUM_ENTRIES];
...
memcpy(strings, r535_registry_entries[i].name, name_len);
Signed-off-by: Kees Cook
---
Cc: Karol Herbst
Cc: Lyude Paul
Cc: Danilo Krummrich
Cc: David Airlie
Cc: Daniel Vetter
Cc: Dave Airlie
Cc: Ben Skeggs
Cc: Timur Tabi
Cc: dri-de...@lists.freedesktop.org
Cc: nouveau@lists.freedesktop.org
---
drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
index 9994cbd6f1c4..9858c1438aa7 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
@@ -1112,7 +1112,7 @@ r535_gsp_rpc_set_registry(struct nvkm_gsp *gsp)
rpc->numEntries = NV_GSP_REG_NUM_ENTRIES;
str_offset = offsetof(typeof(*rpc), entries[NV_GSP_REG_NUM_ENTRIES]);
- strings = (char *)>entries[NV_GSP_REG_NUM_ENTRIES];
+ strings = (char *)rpc + str_offset;
for (i = 0; i < NV_GSP_REG_NUM_ENTRIES; i++) {
int name_len = strlen(r535_registry_entries[i].name) + 1;
--
2.34.1
Re: [linux-next:master] BUILD REGRESSION e31185ce00a96232308300008db193416ceb9769
On February 22, 2024 8:29:28 PM PST, kernel test robot wrote: >tree/branch: >https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master >branch HEAD: e31185ce00a9623230838db193416ceb9769 Add linux-next specific >files for 20240222 > >Error/Warning reports: > >https://lore.kernel.org/oe-kbuild-all/20240223.h9rfmyj4-...@intel.com >https://lore.kernel.org/oe-kbuild-all/20240314.j6a7eb4b-...@intel.com >https://lore.kernel.org/oe-kbuild-all/202402230537.2s6nhfsn-...@intel.com > >Error/Warning: (recently discovered and may have been fixed) > >arch/arm/boot/compressed/misc.c:157:6: warning: no previous prototype for >function '__fortify_panic' [-Wmissing-prototypes] >arch/arm/boot/compressed/misc.h:13:36: error: macro "fortify_panic" requires 2 >arguments, but only 1 given This is fixed for the subsequent -next tree. >arch/sh/boot/compressed/../../../../lib/decompress_unxz.c:350:(.text+0x20b4): >undefined reference to `__ubsan_handle_out_of_bounds' >sh4-linux-ld: >arch/sh/boot/compressed/../../../../lib/xz/xz_dec_lzma2.c:751:(.text+0x904): >undefined reference to `__ubsan_handle_out_of_bounds' This is fixed here and is waiting to land: https://lore.kernel.org/linux-hardening/20240130232717.work.088-k...@kernel.org/ -Kees -- Kees Cook
[PATCH 16/82] drm/nouveau/mmu: Refactor intentional wrap-around calculation
In an effort to separate intentional arithmetic wrap-around from
unexpected wrap-around, we need to refactor places that depend on this
kind of math. One of the most common code patterns of this is:
VAR + value < VAR
Notably, this is considered "undefined behavior" for signed and pointer
types, which the kernel works around by using the -fno-strict-overflow
option in the build[1] (which used to just be -fwrapv). Regardless, we
want to get the kernel source to the position where we can meaningfully
instrument arithmetic wrap-around conditions and catch them when they
are unexpected, regardless of whether they are signed[2], unsigned[3],
or pointer[4] types.
Refactor open-coded unsigned wrap-around addition test to use
check_add_overflow(), retaining the result for later usage (which removes
the redundant open-coded addition). This paves the way to enabling the
wrap-around sanitizers in the future.
Link: https://git.kernel.org/linus/68df3755e383e6fecf2354a67b08f92f18536594 [1]
Link: https://github.com/KSPP/linux/issues/26 [2]
Link: https://github.com/KSPP/linux/issues/27 [3]
Link: https://github.com/KSPP/linux/issues/344 [4]
Cc: Karol Herbst
Cc: Lyude Paul
Cc: Danilo Krummrich
Cc: David Airlie
Cc: Daniel Vetter
Cc: Ben Skeggs
Cc: Dave Airlie
Cc: Julia Lawall
Cc: Jiang Jian
Cc: dri-de...@lists.freedesktop.org
Cc: nouveau@lists.freedesktop.org
Signed-off-by: Kees Cook
---
drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c
index 9c97800fe037..6ca1a82ccbc1 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c
@@ -1149,13 +1149,15 @@ nvkm_vmm_ctor(const struct nvkm_vmm_func *func, struct
nvkm_mmu *mmu,
vmm->root = RB_ROOT;
if (managed) {
+ u64 sum;
+
/* Address-space will be managed by the client for the most
* part, except for a specified area where NVKM allocations
* are allowed to be placed.
*/
vmm->start = 0;
vmm->limit = 1ULL << bits;
- if (addr + size < addr || addr + size > vmm->limit)
+ if (check_add_overflow(addr, size, ) || sum > vmm->limit)
return -EINVAL;
/* Client-managed area before the NVKM-managed area. */
@@ -1174,7 +1176,7 @@ nvkm_vmm_ctor(const struct nvkm_vmm_func *func, struct
nvkm_mmu *mmu,
}
/* Client-managed area after the NVKM-managed area. */
- addr = addr + size;
+ addr = sum;
size = vmm->limit - addr;
if (size && (ret = nvkm_vmm_ctor_managed(vmm, addr, size)))
return ret;
--
2.34.1
[PATCH 48/82] drm/nouveau/mmu: Refactor intentional wrap-around test
In an effort to separate intentional arithmetic wrap-around from
unexpected wrap-around, we need to refactor places that depend on this
kind of math. One of the most common code patterns of this is:
VAR + value < VAR
Notably, this is considered "undefined behavior" for signed and pointer
types, which the kernel works around by using the -fno-strict-overflow
option in the build[1] (which used to just be -fwrapv). Regardless, we
want to get the kernel source to the position where we can meaningfully
instrument arithmetic wrap-around conditions and catch them when they
are unexpected, regardless of whether they are signed[2], unsigned[3],
or pointer[4] types.
Refactor open-coded wrap-around addition test to use add_would_overflow().
This paves the way to enabling the wrap-around sanitizers in the future.
Link: https://git.kernel.org/linus/68df3755e383e6fecf2354a67b08f92f18536594 [1]
Link: https://github.com/KSPP/linux/issues/26 [2]
Link: https://github.com/KSPP/linux/issues/27 [3]
Link: https://github.com/KSPP/linux/issues/344 [4]
Cc: Karol Herbst
Cc: Lyude Paul
Cc: Danilo Krummrich
Cc: David Airlie
Cc: Daniel Vetter
Cc: Dave Airlie
Cc: Ben Skeggs
Cc: dri-de...@lists.freedesktop.org
Cc: nouveau@lists.freedesktop.org
Signed-off-by: Kees Cook
---
drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c
index 6ca1a82ccbc1..87c0903be9a7 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c
@@ -1291,7 +1291,7 @@ nvkm_vmm_pfn_map(struct nvkm_vmm *vmm, u8 shift, u64
addr, u64 size, u64 *pfn)
if (!page->shift || !IS_ALIGNED(addr, 1ULL << shift) ||
!IS_ALIGNED(size, 1ULL << shift) ||
- addr + size < addr || addr + size > vmm->limit) {
+ add_would_overflow(addr, size) || addr + size > vmm->limit) {
VMM_DEBUG(vmm, "paged map %d %d %016llx %016llx\n",
shift, page->shift, addr, size);
return -EINVAL;
--
2.34.1
Re: [PATCH linux-next] drm/nouveau/disp: switch to use kmemdup() helper
On Thu, Dec 14, 2023 at 08:03:22PM +0800, yang.gua...@zte.com.cn wrote: > From: Yang Guang > > Use kmemdup() helper instead of open-coding to > simplify the code. > > Signed-off-by: Chen Haonan Sure, good cleanup. Reviewed-by: Kees Cook -- Kees Cook
Re: [Nouveau] [PATCH][next] nouveau/gsp: replace zero-length array with flex-array member and use __counted_by
On Thu, Nov 16, 2023 at 12:11:43PM -0600, Gustavo A. R. Silva wrote: > Fake flexible arrays (zero-length and one-element arrays) are deprecated, > and should be replaced by flexible-array members. So, replace > zero-length array with a flexible-array member in `struct > PACKED_REGISTRY_TABLE`. > > Also annotate array `entries` with `__counted_by()` to prepare for the > coming implementation by GCC and Clang of the `__counted_by` attribute. > Flexible array members annotated with `__counted_by` can have their > accesses bounds-checked at run-time via `CONFIG_UBSAN_BOUNDS` (for array > indexing) and `CONFIG_FORTIFY_SOURCE` (for strcpy/memcpy-family functions). > > This fixes multiple -Warray-bounds warnings: > drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:1069:29: warning: array > subscript 0 is outside array bounds of 'PACKED_REGISTRY_ENTRY[0]' > [-Warray-bounds=] > drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:1070:29: warning: array > subscript 0 is outside array bounds of 'PACKED_REGISTRY_ENTRY[0]' > [-Warray-bounds=] > drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:1071:29: warning: array > subscript 0 is outside array bounds of 'PACKED_REGISTRY_ENTRY[0]' > [-Warray-bounds=] > drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:1072:29: warning: array > subscript 0 is outside array bounds of 'PACKED_REGISTRY_ENTRY[0]' > [-Warray-bounds=] > > While there, also make use of the struct_size() helper, and address > checkpatch.pl warning: > WARNING: please, no spaces at the start of a line > > This results in no differences in binary output. > > Signed-off-by: Gustavo A. R. Silva Looks nice to me. Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH 0/9] drm: Annotate structs with __counted_by
On Thu, Oct 05, 2023 at 11:42:38AM +0200, Christian König wrote: > Am 02.10.23 um 20:22 schrieb Kees Cook: > > On Mon, Oct 02, 2023 at 08:11:41PM +0200, Christian König wrote: > > > Am 02.10.23 um 20:08 schrieb Kees Cook: > > > > On Mon, Oct 02, 2023 at 08:01:57PM +0200, Christian König wrote: > > > > > Am 02.10.23 um 18:53 schrieb Kees Cook: > > > > > > On Mon, Oct 02, 2023 at 11:06:19AM -0400, Alex Deucher wrote: > > > > > > > On Mon, Oct 2, 2023 at 5:20 AM Christian König > > > > > > > wrote: > > > > > > > > Am 29.09.23 um 21:33 schrieb Kees Cook: > > > > > > > > > On Fri, 22 Sep 2023 10:32:05 -0700, Kees Cook wrote: > > > > > > > > > > This is a batch of patches touching drm for preparing for > > > > > > > > > > the coming > > > > > > > > > > implementation by GCC and Clang of the __counted_by > > > > > > > > > > attribute. Flexible > > > > > > > > > > array members annotated with __counted_by can have their > > > > > > > > > > accesses > > > > > > > > > > bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS > > > > > > > > > > (for array > > > > > > > > > > indexing) and CONFIG_FORTIFY_SOURCE (for > > > > > > > > > > strcpy/memcpy-family functions). > > > > > > > > > > > > > > > > > > > > As found with Coccinelle[1], add __counted_by to structs > > > > > > > > > > that would > > > > > > > > > > benefit from the annotation. > > > > > > > > > > > > > > > > > > > > [...] > > > > > > > > > Since this got Acks, I figure I should carry it in my tree. > > > > > > > > > Let me know > > > > > > > > > if this should go via drm instead. > > > > > > > > > > > > > > > > > > Applied to for-next/hardening, thanks! > > > > > > > > > > > > > > > > > > [1/9] drm/amd/pm: Annotate struct > > > > > > > > > smu10_voltage_dependency_table with __counted_by > > > > > > > > > https://git.kernel.org/kees/c/a6046ac659d6 > > > > > > > > STOP! In a follow up discussion Alex and I figured out that > > > > > > > > this won't work. > > > > > > I'm so confused; from the discussion I saw that Alex said both > > > > > > instances > > > > > > were false positives? > > > > > > > > > > > > > > The value in the structure is byte swapped based on some > > > > > > > > firmware > > > > > > > > endianness which not necessary matches the CPU endianness. > > > > > > > SMU10 is APU only so the endianess of the SMU firmware and the CPU > > > > > > > will always match. > > > > > > Which I think is what is being said here? > > > > > > > > > > > > > > Please revert that one from going upstream if it's already on > > > > > > > > it's way. > > > > > > > > > > > > > > > > And because of those reasons I strongly think that patches like > > > > > > > > this > > > > > > > > should go through the DRM tree :) > > > > > > Sure, that's fine -- please let me know. It was others Acked/etc. > > > > > > Who > > > > > > should carry these patches? > > > > > Probably best if the relevant maintainer pick them up individually. > > > > > > > > > > Some of those structures are filled in by firmware/hardware and only > > > > > the > > > > > maintainers can judge if that value actually matches what the compiler > > > > > needs. > > > > > > > > > > We have cases where individual bits are used as flags or when the > > > > > size is > > > > > byte swapped etc... > > > > > > > > > > Even Alex and I didn't immediately say how and where that field is > > > > > actually > > > > > used and had to dig that up. That's where the confusion came from. > > > > Okay, I've dropped them all from my tree. Several had Acks/Reviews, so > > > > hopefully those can get picked up for the DRM tree? > > > I will pick those up to go through drm-misc-next. > > > > > > Going to ping maintainers once more when I'm not sure if stuff is correct > > > or > > > not. > > Sounds great; thanks! > > I wasn't 100% sure for the VC4 patch, but pushed the whole set to > drm-misc-next anyway. > > This also means that the patches are now auto merged into the drm-tip > integration branch and should any build or unit test go boom we should > notice immediately and can revert it pretty easily. Thanks very much; I'll keep an eye out for any reports. -- Kees Cook
Re: [PATCH 0/9] drm: Annotate structs with __counted_by
On Mon, Oct 02, 2023 at 08:11:41PM +0200, Christian König wrote: > Am 02.10.23 um 20:08 schrieb Kees Cook: > > On Mon, Oct 02, 2023 at 08:01:57PM +0200, Christian König wrote: > > > Am 02.10.23 um 18:53 schrieb Kees Cook: > > > > On Mon, Oct 02, 2023 at 11:06:19AM -0400, Alex Deucher wrote: > > > > > On Mon, Oct 2, 2023 at 5:20 AM Christian König > > > > > wrote: > > > > > > Am 29.09.23 um 21:33 schrieb Kees Cook: > > > > > > > On Fri, 22 Sep 2023 10:32:05 -0700, Kees Cook wrote: > > > > > > > > This is a batch of patches touching drm for preparing for the > > > > > > > > coming > > > > > > > > implementation by GCC and Clang of the __counted_by attribute. > > > > > > > > Flexible > > > > > > > > array members annotated with __counted_by can have their > > > > > > > > accesses > > > > > > > > bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS > > > > > > > > (for array > > > > > > > > indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > > > > > > > > functions). > > > > > > > > > > > > > > > > As found with Coccinelle[1], add __counted_by to structs that > > > > > > > > would > > > > > > > > benefit from the annotation. > > > > > > > > > > > > > > > > [...] > > > > > > > Since this got Acks, I figure I should carry it in my tree. Let > > > > > > > me know > > > > > > > if this should go via drm instead. > > > > > > > > > > > > > > Applied to for-next/hardening, thanks! > > > > > > > > > > > > > > [1/9] drm/amd/pm: Annotate struct smu10_voltage_dependency_table > > > > > > > with __counted_by > > > > > > > https://git.kernel.org/kees/c/a6046ac659d6 > > > > > > STOP! In a follow up discussion Alex and I figured out that this > > > > > > won't work. > > > > I'm so confused; from the discussion I saw that Alex said both instances > > > > were false positives? > > > > > > > > > > The value in the structure is byte swapped based on some firmware > > > > > > endianness which not necessary matches the CPU endianness. > > > > > SMU10 is APU only so the endianess of the SMU firmware and the CPU > > > > > will always match. > > > > Which I think is what is being said here? > > > > > > > > > > Please revert that one from going upstream if it's already on it's > > > > > > way. > > > > > > > > > > > > And because of those reasons I strongly think that patches like this > > > > > > should go through the DRM tree :) > > > > Sure, that's fine -- please let me know. It was others Acked/etc. Who > > > > should carry these patches? > > > Probably best if the relevant maintainer pick them up individually. > > > > > > Some of those structures are filled in by firmware/hardware and only the > > > maintainers can judge if that value actually matches what the compiler > > > needs. > > > > > > We have cases where individual bits are used as flags or when the size is > > > byte swapped etc... > > > > > > Even Alex and I didn't immediately say how and where that field is > > > actually > > > used and had to dig that up. That's where the confusion came from. > > Okay, I've dropped them all from my tree. Several had Acks/Reviews, so > > hopefully those can get picked up for the DRM tree? > > I will pick those up to go through drm-misc-next. > > Going to ping maintainers once more when I'm not sure if stuff is correct or > not. Sounds great; thanks! -Kees -- Kees Cook
Re: [Nouveau] [PATCH 0/9] drm: Annotate structs with __counted_by
On Mon, Oct 02, 2023 at 08:01:57PM +0200, Christian König wrote: > Am 02.10.23 um 18:53 schrieb Kees Cook: > > On Mon, Oct 02, 2023 at 11:06:19AM -0400, Alex Deucher wrote: > > > On Mon, Oct 2, 2023 at 5:20 AM Christian König > > > wrote: > > > > Am 29.09.23 um 21:33 schrieb Kees Cook: > > > > > On Fri, 22 Sep 2023 10:32:05 -0700, Kees Cook wrote: > > > > > > This is a batch of patches touching drm for preparing for the coming > > > > > > implementation by GCC and Clang of the __counted_by attribute. > > > > > > Flexible > > > > > > array members annotated with __counted_by can have their accesses > > > > > > bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS (for > > > > > > array > > > > > > indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > > > > > > functions). > > > > > > > > > > > > As found with Coccinelle[1], add __counted_by to structs that would > > > > > > benefit from the annotation. > > > > > > > > > > > > [...] > > > > > Since this got Acks, I figure I should carry it in my tree. Let me > > > > > know > > > > > if this should go via drm instead. > > > > > > > > > > Applied to for-next/hardening, thanks! > > > > > > > > > > [1/9] drm/amd/pm: Annotate struct smu10_voltage_dependency_table with > > > > > __counted_by > > > > > https://git.kernel.org/kees/c/a6046ac659d6 > > > > STOP! In a follow up discussion Alex and I figured out that this won't > > > > work. > > I'm so confused; from the discussion I saw that Alex said both instances > > were false positives? > > > > > > The value in the structure is byte swapped based on some firmware > > > > endianness which not necessary matches the CPU endianness. > > > SMU10 is APU only so the endianess of the SMU firmware and the CPU > > > will always match. > > Which I think is what is being said here? > > > > > > Please revert that one from going upstream if it's already on it's way. > > > > > > > > And because of those reasons I strongly think that patches like this > > > > should go through the DRM tree :) > > Sure, that's fine -- please let me know. It was others Acked/etc. Who > > should carry these patches? > > Probably best if the relevant maintainer pick them up individually. > > Some of those structures are filled in by firmware/hardware and only the > maintainers can judge if that value actually matches what the compiler > needs. > > We have cases where individual bits are used as flags or when the size is > byte swapped etc... > > Even Alex and I didn't immediately say how and where that field is actually > used and had to dig that up. That's where the confusion came from. Okay, I've dropped them all from my tree. Several had Acks/Reviews, so hopefully those can get picked up for the DRM tree? Thanks! -Kees -- Kees Cook
Re: [PATCH 0/9] drm: Annotate structs with __counted_by
On Mon, Oct 02, 2023 at 11:06:19AM -0400, Alex Deucher wrote: > On Mon, Oct 2, 2023 at 5:20 AM Christian König > wrote: > > > > Am 29.09.23 um 21:33 schrieb Kees Cook: > > > On Fri, 22 Sep 2023 10:32:05 -0700, Kees Cook wrote: > > >> This is a batch of patches touching drm for preparing for the coming > > >> implementation by GCC and Clang of the __counted_by attribute. Flexible > > >> array members annotated with __counted_by can have their accesses > > >> bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS (for array > > >> indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions). > > >> > > >> As found with Coccinelle[1], add __counted_by to structs that would > > >> benefit from the annotation. > > >> > > >> [...] > > > Since this got Acks, I figure I should carry it in my tree. Let me know > > > if this should go via drm instead. > > > > > > Applied to for-next/hardening, thanks! > > > > > > [1/9] drm/amd/pm: Annotate struct smu10_voltage_dependency_table with > > > __counted_by > > >https://git.kernel.org/kees/c/a6046ac659d6 > > > > STOP! In a follow up discussion Alex and I figured out that this won't work. I'm so confused; from the discussion I saw that Alex said both instances were false positives? > > > > The value in the structure is byte swapped based on some firmware > > endianness which not necessary matches the CPU endianness. > > SMU10 is APU only so the endianess of the SMU firmware and the CPU > will always match. Which I think is what is being said here? > > Please revert that one from going upstream if it's already on it's way. > > > > And because of those reasons I strongly think that patches like this > > should go through the DRM tree :) Sure, that's fine -- please let me know. It was others Acked/etc. Who should carry these patches? Thanks! -Kees > > > > Regards, > > Christian. > > > > > [2/9] drm/amdgpu/discovery: Annotate struct ip_hw_instance with > > > __counted_by > > >https://git.kernel.org/kees/c/4df33089b46f > > > [3/9] drm/i915/selftests: Annotate struct perf_series with __counted_by > > >https://git.kernel.org/kees/c/ffd3f823bdf6 > > > [4/9] drm/msm/dpu: Annotate struct dpu_hw_intr with __counted_by > > >https://git.kernel.org/kees/c/2de35a989b76 > > > [5/9] drm/nouveau/pm: Annotate struct nvkm_perfdom with __counted_by > > >https://git.kernel.org/kees/c/188aeb08bfaa > > > [6/9] drm/vc4: Annotate struct vc4_perfmon with __counted_by > > >https://git.kernel.org/kees/c/59a54dc896c3 > > > [7/9] drm/virtio: Annotate struct virtio_gpu_object_array with > > > __counted_by > > >https://git.kernel.org/kees/c/5cd476de33af > > > [8/9] drm/vmwgfx: Annotate struct vmw_surface_dirty with __counted_by > > >https://git.kernel.org/kees/c/b426f2e5356a > > > [9/9] drm/v3d: Annotate struct v3d_perfmon with __counted_by > > >https://git.kernel.org/kees/c/dc662fa1b0e4 > > > > > > Take care, > > > > > -- Kees Cook
Re: [PATCH 0/9] drm: Annotate structs with __counted_by
On Fri, 22 Sep 2023 10:32:05 -0700, Kees Cook wrote: > This is a batch of patches touching drm for preparing for the coming > implementation by GCC and Clang of the __counted_by attribute. Flexible > array members annotated with __counted_by can have their accesses > bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS (for array > indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions). > > As found with Coccinelle[1], add __counted_by to structs that would > benefit from the annotation. > > [...] Since this got Acks, I figure I should carry it in my tree. Let me know if this should go via drm instead. Applied to for-next/hardening, thanks! [1/9] drm/amd/pm: Annotate struct smu10_voltage_dependency_table with __counted_by https://git.kernel.org/kees/c/a6046ac659d6 [2/9] drm/amdgpu/discovery: Annotate struct ip_hw_instance with __counted_by https://git.kernel.org/kees/c/4df33089b46f [3/9] drm/i915/selftests: Annotate struct perf_series with __counted_by https://git.kernel.org/kees/c/ffd3f823bdf6 [4/9] drm/msm/dpu: Annotate struct dpu_hw_intr with __counted_by https://git.kernel.org/kees/c/2de35a989b76 [5/9] drm/nouveau/pm: Annotate struct nvkm_perfdom with __counted_by https://git.kernel.org/kees/c/188aeb08bfaa [6/9] drm/vc4: Annotate struct vc4_perfmon with __counted_by https://git.kernel.org/kees/c/59a54dc896c3 [7/9] drm/virtio: Annotate struct virtio_gpu_object_array with __counted_by https://git.kernel.org/kees/c/5cd476de33af [8/9] drm/vmwgfx: Annotate struct vmw_surface_dirty with __counted_by https://git.kernel.org/kees/c/b426f2e5356a [9/9] drm/v3d: Annotate struct v3d_perfmon with __counted_by https://git.kernel.org/kees/c/dc662fa1b0e4 Take care, -- Kees Cook
Re: [Nouveau] [PATCH 0/2][next] nouveau/svm: Replace one-element array with flexible-array member
On Wed, 16 Aug 2023 12:03:06 -0600, Gustavo A. R. Silva wrote: > This small series aims to replace a one-element array with a > flexible-array member in struct nouveau_svm. And, while at it, > fix a checkpatch.pl error. > > Gustavo A. R. Silva (2): > nouveau/svm: Replace one-element array with flexible-array member in > struct nouveau_svm > nouveau/svm: Split assignment from if conditional > > [...] These look trivially correct and haven't had further feedback for over a month. Applied to for-next/hardening, thanks! [1/2] nouveau/svm: Replace one-element array with flexible-array member in struct nouveau_svm https://git.kernel.org/kees/c/6ad33b53c9b8 [2/2] nouveau/svm: Split assignment from if conditional https://git.kernel.org/kees/c/4cb2e89fea5f Take care, -- Kees Cook
Re: [Nouveau] [PATCH 1/9] drm/amd/pm: Annotate struct smu10_voltage_dependency_table with __counted_by
On Mon, Sep 25, 2023 at 08:30:30AM +0200, Christian König wrote:
> Am 22.09.23 um 19:41 schrieb Alex Deucher:
> > On Fri, Sep 22, 2023 at 1:32 PM Kees Cook wrote:
> > > Prepare for the coming implementation by GCC and Clang of the __counted_by
> > > attribute. Flexible array members annotated with __counted_by can have
> > > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
> > > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
> > > functions).
> > >
> > > As found with Coccinelle[1], add __counted_by for struct
> > > smu10_voltage_dependency_table.
> > >
> > > [1]
> > > https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
> > >
> > > Cc: Evan Quan
> > > Cc: Alex Deucher
> > > Cc: "Christian König"
> > > Cc: "Pan, Xinhui"
> > > Cc: David Airlie
> > > Cc: Daniel Vetter
> > > Cc: Xiaojian Du
> > > Cc: Huang Rui
> > > Cc: Kevin Wang
> > > Cc: amd-...@lists.freedesktop.org
> > > Cc: dri-de...@lists.freedesktop.org
> > > Signed-off-by: Kees Cook
> > Acked-by: Alex Deucher
>
> Mhm, I'm not sure if this is a good idea. That is a structure filled in by
> the firmware, isn't it?
>
> That would imply that we might need to byte swap count before it is
> checkable.
The script found this instance because of this:
static int smu10_get_clock_voltage_dependency_table(struct pp_hwmgr *hwmgr,
struct smu10_voltage_dependency_table **pptable,
uint32_t num_entry, const DpmClock_t
*pclk_dependency_table)
{
uint32_t i;
struct smu10_voltage_dependency_table *ptable;
ptable = kzalloc(struct_size(ptable, entries, num_entry), GFP_KERNEL);
if (NULL == ptable)
return -ENOMEM;
ptable->count = num_entry;
So the implication is that it's native byte order... but you tell me! I
certainly don't want this annotation if it's going to break stuff. :)
--
Kees Cook
Re: [Nouveau] [PATCH 3/9] drm/i915/selftests: Annotate struct perf_series with __counted_by
On Mon, Sep 25, 2023 at 12:08:36PM +0200, Andrzej Hajda wrote: > > > On 22.09.2023 19:32, Kees Cook wrote: > > Prepare for the coming implementation by GCC and Clang of the __counted_by > > attribute. Flexible array members annotated with __counted_by can have > > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS > > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > > functions). > > > > As found with Coccinelle[1], add __counted_by for struct perf_series. > > > > [1] > > https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci > > > > Cc: Jani Nikula > > Cc: Joonas Lahtinen > > Cc: Rodrigo Vivi > > Cc: Tvrtko Ursulin > > Cc: David Airlie > > Cc: Daniel Vetter > > Cc: Chris Wilson > > Cc: John Harrison > > Cc: Andi Shyti > > Cc: Matthew Brost > > Cc: intel-...@lists.freedesktop.org > > Cc: dri-de...@lists.freedesktop.org > > Signed-off-by: Kees Cook > > I am surprised this is the only finding in i915, I would expected more. I'm sure there are more, but it's likely my Coccinelle pattern didn't catch it. There are many many flexible arrays in drm. :) $ grep -nRH '\[\];$' drivers/gpu/drm include/uapi/drm | grep -v :extern | wc -l 122 If anyone has some patterns I can add to the Coccinelle script, I can take another pass at it. > Anyway: > > Reviewed-by: Andrzej Hajda Thank you! -Kees -- Kees Cook
[Nouveau] [PATCH 8/9] drm/vmwgfx: Annotate struct vmw_surface_dirty with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct vmw_surface_dirty.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Zack Rusin
Cc: VMware Graphics Reviewers
Cc: David Airlie
Cc: Daniel Vetter
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: Kees Cook
---
drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
index 5db403ee8261..2d1d857f99ae 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
@@ -77,7 +77,7 @@ struct vmw_surface_offset {
struct vmw_surface_dirty {
struct vmw_surface_cache cache;
u32 num_subres;
- SVGA3dBox boxes[];
+ SVGA3dBox boxes[] __counted_by(num_subres);
};
static void vmw_user_surface_free(struct vmw_resource *res);
--
2.34.1
[PATCH 9/9] drm/v3d: Annotate struct v3d_perfmon with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct v3d_perfmon.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Emma Anholt
Cc: Melissa Wen
Cc: David Airlie
Cc: Daniel Vetter
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: Kees Cook
---
drivers/gpu/drm/v3d/v3d_drv.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index 7f664a4b2a75..106454f28956 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -59,7 +59,7 @@ struct v3d_perfmon {
* values can't be reset, but you can fake a reset by
* destroying the perfmon and creating a new one.
*/
- u64 values[];
+ u64 values[] __counted_by(ncounters);
};
struct v3d_dev {
--
2.34.1
[PATCH 7/9] drm/virtio: Annotate struct virtio_gpu_object_array with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct
virtio_gpu_object_array.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: David Airlie
Cc: Gerd Hoffmann
Cc: Gurchetan Singh
Cc: Chia-I Wu
Cc: Daniel Vetter
Cc: dri-de...@lists.freedesktop.org
Cc: virtualizat...@lists.linux-foundation.org
Signed-off-by: Kees Cook
---
drivers/gpu/drm/virtio/virtgpu_drv.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 8513b671f871..96365a772f77 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -119,7 +119,7 @@ struct virtio_gpu_object_array {
struct ww_acquire_ctx ticket;
struct list_head next;
u32 nents, total;
- struct drm_gem_object *objs[];
+ struct drm_gem_object *objs[] __counted_by(total);
};
struct virtio_gpu_vbuffer;
--
2.34.1
[PATCH 5/9] drm/nouveau/pm: Annotate struct nvkm_perfdom with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct nvkm_perfdom.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Ben Skeggs
Cc: Karol Herbst
Cc: Lyude Paul
Cc: David Airlie
Cc: Daniel Vetter
Cc: dri-de...@lists.freedesktop.org
Cc: nouveau@lists.freedesktop.org
Signed-off-by: Kees Cook
---
drivers/gpu/drm/nouveau/nvkm/engine/pm/priv.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/pm/priv.h
b/drivers/gpu/drm/nouveau/nvkm/engine/pm/priv.h
index 6ae25d3e7f45..c011227f7052 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/pm/priv.h
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/pm/priv.h
@@ -82,7 +82,7 @@ struct nvkm_perfdom {
u8 mode;
u32 clk;
u16 signal_nr;
- struct nvkm_perfsig signal[];
+ struct nvkm_perfsig signal[] __counted_by(signal_nr);
};
struct nvkm_funcdom {
--
2.34.1
[PATCH 6/9] drm/vc4: Annotate struct vc4_perfmon with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct vc4_perfmon.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Emma Anholt
Cc: Maxime Ripard
Cc: David Airlie
Cc: Daniel Vetter
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: Kees Cook
---
drivers/gpu/drm/vc4/vc4_drv.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index bf66499765fb..ab61e96e7e14 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -76,7 +76,7 @@ struct vc4_perfmon {
* Note that counter values can't be reset, but you can fake a reset by
* destroying the perfmon and creating a new one.
*/
- u64 counters[];
+ u64 counters[] __counted_by(ncounters);
};
struct vc4_dev {
--
2.34.1
[PATCH 4/9] drm/msm/dpu: Annotate struct dpu_hw_intr with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct dpu_hw_intr.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Rob Clark
Cc: Abhinav Kumar
Cc: Dmitry Baryshkov
Cc: Sean Paul
Cc: Marijn Suijten
Cc: David Airlie
Cc: Daniel Vetter
Cc: Bjorn Andersson
Cc: linux-arm-...@vger.kernel.org
Cc: dri-de...@lists.freedesktop.org
Cc: freedr...@lists.freedesktop.org
Signed-off-by: Kees Cook
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
index dab761e54863..50cf9523d367 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
@@ -61,7 +61,7 @@ struct dpu_hw_intr {
void (*cb)(void *arg, int irq_idx);
void *arg;
atomic_t count;
- } irq_tbl[];
+ } irq_tbl[] __counted_by(total_irqs);
};
/**
--
2.34.1
[PATCH 3/9] drm/i915/selftests: Annotate struct perf_series with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct perf_series.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Jani Nikula
Cc: Joonas Lahtinen
Cc: Rodrigo Vivi
Cc: Tvrtko Ursulin
Cc: David Airlie
Cc: Daniel Vetter
Cc: Chris Wilson
Cc: John Harrison
Cc: Andi Shyti
Cc: Matthew Brost
Cc: intel-...@lists.freedesktop.org
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: Kees Cook
---
drivers/gpu/drm/i915/selftests/i915_request.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/selftests/i915_request.c
b/drivers/gpu/drm/i915/selftests/i915_request.c
index a9b79888c193..acae30a04a94 100644
--- a/drivers/gpu/drm/i915/selftests/i915_request.c
+++ b/drivers/gpu/drm/i915/selftests/i915_request.c
@@ -1924,7 +1924,7 @@ struct perf_stats {
struct perf_series {
struct drm_i915_private *i915;
unsigned int nengines;
- struct intel_context *ce[];
+ struct intel_context *ce[] __counted_by(nengines);
};
static int cmp_u32(const void *A, const void *B)
--
2.34.1
[PATCH 2/9] drm/amdgpu/discovery: Annotate struct ip_hw_instance with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct ip_hw_instance.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Alex Deucher
Cc: "Christian König"
Cc: "Pan, Xinhui"
Cc: David Airlie
Cc: Daniel Vetter
Cc: Hawking Zhang
Cc: amd-...@lists.freedesktop.org
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: Kees Cook
---
drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
index d1bc7b212520..be4c97a3d7bf 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
@@ -662,7 +662,7 @@ struct ip_hw_instance {
u8 harvest;
int num_base_addresses;
- u32 base_addr[];
+ u32 base_addr[] __counted_by(num_base_addresses);
};
struct ip_hw_id {
--
2.34.1
[PATCH 1/9] drm/amd/pm: Annotate struct smu10_voltage_dependency_table with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct
smu10_voltage_dependency_table.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Evan Quan
Cc: Alex Deucher
Cc: "Christian König"
Cc: "Pan, Xinhui"
Cc: David Airlie
Cc: Daniel Vetter
Cc: Xiaojian Du
Cc: Huang Rui
Cc: Kevin Wang
Cc: amd-...@lists.freedesktop.org
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: Kees Cook
---
drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.h
b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.h
index 808e0ecbe1f0..42adc2a3dcbc 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.h
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.h
@@ -192,7 +192,7 @@ struct smu10_clock_voltage_dependency_record {
struct smu10_voltage_dependency_table {
uint32_t count;
- struct smu10_clock_voltage_dependency_record entries[];
+ struct smu10_clock_voltage_dependency_record entries[]
__counted_by(count);
};
struct smu10_clock_voltage_information {
--
2.34.1
[PATCH 0/9] drm: Annotate structs with __counted_by
Hi, This is a batch of patches touching drm for preparing for the coming implementation by GCC and Clang of the __counted_by attribute. Flexible array members annotated with __counted_by can have their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions). As found with Coccinelle[1], add __counted_by to structs that would benefit from the annotation. Since the element count member must be set before accessing the annotated flexible array member, some patches also move the member's initialization earlier. (These are noted in the individual patches.) -Kees [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Kees Cook (9): drm/amd/pm: Annotate struct smu10_voltage_dependency_table with __counted_by drm/amdgpu/discovery: Annotate struct ip_hw_instance with __counted_by drm/i915/selftests: Annotate struct perf_series with __counted_by drm/msm/dpu: Annotate struct dpu_hw_intr with __counted_by drm/nouveau/pm: Annotate struct nvkm_perfdom with __counted_by drm/vc4: Annotate struct vc4_perfmon with __counted_by drm/virtio: Annotate struct virtio_gpu_object_array with __counted_by drm/vmwgfx: Annotate struct vmw_surface_dirty with __counted_by drm/v3d: Annotate struct v3d_perfmon with __counted_by drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c| 2 +- drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.h | 2 +- drivers/gpu/drm/i915/selftests/i915_request.c| 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h| 2 +- drivers/gpu/drm/nouveau/nvkm/engine/pm/priv.h| 2 +- drivers/gpu/drm/v3d/v3d_drv.h| 2 +- drivers/gpu/drm/vc4/vc4_drv.h| 2 +- drivers/gpu/drm/virtio/virtgpu_drv.h | 2 +- drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 2 +- 9 files changed, 9 insertions(+), 9 deletions(-) -- 2.34.1
Re: [Nouveau] [PATCH] drm/nouveau/pm: refactor deprecated strncpy
On Thu, Sep 14, 2023 at 10:17:08PM +, Justin Stitt wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > We should prefer more robust and less ambiguous string interfaces. > > A suitable replacement is `strscpy` [2] due to the fact that it guarantees > NUL-termination on the destination buffer without unnecessarily NUL-padding. > > Link: > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-harden...@vger.kernel.org > Signed-off-by: Justin Stitt The "- 1" use in the original code is strong evidence for this being a sane conversion. :) Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH] drm/nouveau/core: refactor deprecated strncpy
On Thu, Sep 14, 2023 at 09:40:49PM +, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
>
> We should prefer more robust and less ambiguous string interfaces.
>
> A suitable replacement is `strscpy` [2] due to the fact that it guarantees
> NUL-termination on the destination buffer without unnecessarily NUL-padding.
>
> There is likely no bug in the current implementation due to the safeguard:
> | cname[sizeof(cname) - 1] = '\0';
> ... however we can provide simpler and easier to understand code using
> the newer (and recommended) `strscpy` api.
>
> Link:
> https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> [1]
> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html
> [2]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-harden...@vger.kernel.org
> Signed-off-by: Justin Stitt
> ---
> drivers/gpu/drm/nouveau/nvkm/core/firmware.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nvkm/core/firmware.c
> b/drivers/gpu/drm/nouveau/nvkm/core/firmware.c
> index 91fb494d4009..374212da9e95 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/core/firmware.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/core/firmware.c
> @@ -79,8 +79,7 @@ nvkm_firmware_get(const struct nvkm_subdev *subdev, const
> char *fwname, int ver,
> int i;
>
> /* Convert device name to lowercase */
> - strncpy(cname, device->chip->name, sizeof(cname));
> - cname[sizeof(cname) - 1] = '\0';
> + strscpy(cname, device->chip->name, sizeof(cname));
> i = strlen(cname);
> while (i) {
> --i;
Yup, consumed by strlen() and snprintf(). Looks like a standard
conversion. :)
Reviewed-by: Kees Cook
-Kees
>
> ---
> base-commit: 3669558bdf354cd352be955ef2764cde6a9bf5ec
> change-id:
> 20230914-strncpy-drivers-gpu-drm-nouveau-nvkm-core-firmware-c-791223838b72
>
> Best regards,
> --
> Justin Stitt
>
--
Kees Cook
Re: [PATCH] drm/nouveau/nvif: refactor deprecated strncpy
On Thu, Sep 14, 2023 at 09:30:37PM +, Justin Stitt wrote:
> `strncpy` is deprecated and as such we should prefer more robust and
> less ambiguous string interfaces.
>
> A suitable replacement is `strscpy_pad` due to the fact that it
> guarantees NUL-termination on the destination buffer whilst also
> maintaining the NUL-padding behavior that `strncpy` provides. I am not
> sure whether NUL-padding is strictly needed but I see in
> `nvif_object_ctor()` args is memcpy'd elsewhere so I figured we'd keep
> the same functionality.
>
> Link:
> https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> [1]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-harden...@vger.kernel.org
> Signed-off-by: Justin Stitt
> ---
> Note: build-tested only.
> ---
> drivers/gpu/drm/nouveau/nvif/client.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nvif/client.c
> b/drivers/gpu/drm/nouveau/nvif/client.c
> index a3264a0e933a..3a27245f467f 100644
> --- a/drivers/gpu/drm/nouveau/nvif/client.c
> +++ b/drivers/gpu/drm/nouveau/nvif/client.c
> @@ -69,7 +69,7 @@ nvif_client_ctor(struct nvif_client *parent, const char
> *name, u64 device,
> } nop = {};
> int ret;
>
> - strncpy(args.name, name, sizeof(args.name));
> + strscpy_pad(args.name, name, sizeof(args.name));
> ret = nvif_object_ctor(parent != client ? >object : NULL,
> name ? name : "nvifClient", 0,
> NVIF_CLASS_CLIENT, , sizeof(args),
Right, I see the memcpy too:
nvif_object_ctor(struct nvif_object *parent, const char *name, u32 handle,
s32 oclass, void *data, u32 size, struct nvif_object *object)
{
...
memcpy(args->new.data, data, size);
However, "args" is already zeroed:
struct nvif_client_v0 args = { .device = device };
So there's no need for _pad(). However, I couldn't immediatley see
why args.name must be %NUL terminated. It's ultimately passed through
an ioctl:
return client->driver->ioctl(client->object.priv, data, size, hack);
But I did find it...
args is:
struct nvif_client_v0 {
...
char name[32];
};
And "name" only ever comes through nvif_client_ctor(), which all end up
via here, using "cli":
struct nouveau_cli {
...
char name[32];
...
snprintf(cli->name, sizeof(cli->name), "%s", sname);
...
ret = nvif_client_ctor(>master.base, cli->name, device,
>base);
So we'll always be %NUL terminated.
Therefore, yes, conversion looks good:
Reviewed-by: Kees Cook
Thanks!
-Kees
>
> ---
> base-commit: 3669558bdf354cd352be955ef2764cde6a9bf5ec
> change-id: 20230914-strncpy-drivers-gpu-drm-nouveau-nvif-client-c-82b023c36953
>
> Best regards,
> --
> Justin Stitt
>
--
Kees Cook
Re: [Nouveau] [PATCH 2/2][next] nouveau/svm: Split assignment from if conditional
On Wed, Aug 16, 2023 at 12:05:06PM -0600, Gustavo A. R. Silva wrote: > Fix checkpatch.pl ERROR: do not use assignment in if condition. > > Signed-off-by: Gustavo A. R. Silva Reviewed-by: Kees Cook -- Kees Cook
Re: [Nouveau] [PATCH 1/2][next] nouveau/svm: Replace one-element array with flexible-array member in struct nouveau_svm
On Wed, Aug 16, 2023 at 12:04:06PM -0600, Gustavo A. R. Silva wrote: > One-element and zero-length arrays are deprecated. So, replace > one-element array in struct nouveau_svm with flexible-array member. > > This results in no differences in binary output. > > Link: https://github.com/KSPP/linux/issues/338 > Signed-off-by: Gustavo A. R. Silva Reviewed-by: Kees Cook -- Kees Cook
Re: [Nouveau] Disabling -Warray-bounds for gcc-13 too
On April 27, 2023 3:50:06 PM PDT, Karol Herbst wrote:
>On Fri, Apr 28, 2023 at 12:46 AM Lyude Paul wrote:
>>
>> Hey Linus, Kees. Responses below
>>
>> On Sun, 2023-04-23 at 13:23 -0700, Kees Cook wrote:
>> > On April 23, 2023 10:36:24 AM PDT, Linus Torvalds
>> > wrote:
>> > > Kees,
>> > > I made the mistake of upgrading my M2 Macbook Air to Fedora-38, and
>> > > in the process I got gcc-13 which is not WERROR-clean because we only
>> > > limited the 'array-bounds' warning to gcc-11 and gcc-12. But gcc-13
>> > > has all the same issues.
>> > >
>> > > And I want to be able to do my arm64 builds with WERROR on still...
>> > >
>> > > I guess it never made much sense to hope it was going to go away
>> > > without having a confirmation, so I just changed it to be gcc-11+.
>> >
>> > Yeah, that's fine. GCC 13 released without having a fix for at least one
>> > (hopefully last) known array-bounds vs jump threading bug:
>> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109071
>> >
>> > > And one of them is from you.
>> > >
>> > > In particular, commit 4076ea2419cf ("drm/nouveau/disp: Fix
>> > > nvif_outp_acquire_dp() argument size") cannot possibly be right, It
>> > > changes
>> > >
>> > > nvif_outp_acquire_dp(struct nvif_outp *outp, u8 dpcd[16],
>> > >
>> > > to
>> > >
>> > > nvif_outp_acquire_dp(struct nvif_outp *outp, u8
>> > > dpcd[DP_RECEIVER_CAP_SIZE],
>> > >
>> > > and then does
>> > >
>> > >memcpy(args.dp.dpcd, dpcd, sizeof(args.dp.dpcd));
>> > >
>> > > where that 'args.dp.dpcd' is a 16-byte array, and DP_RECEIVER_CAP_SIZE
>> > > is 15.
>> >
>> > Yeah, it was an incomplete fix. I sent the other half here, but it fell
>> > through the cracks:
>> > https://lore.kernel.org/lkml/20230204184307.never.825-k...@kernel.org/
>>
>> Thanks for bringing this to our attention, yeah this definitely just looks
>> like it got missed somewhere down the line. It looks like Karol responded
>> already so I assume the patch is in the pipeline now, but let me know if
>> there's anything else you need.
>>
>
>uhm, I didn't push anything, but I can push it through drm-misct asap,
>just wanted to ask if somebody wants to pick a quicker route. But I
>guess not?
If you can pick it up, that would be great. There's no rush. :)
--
Kees Cook
Re: [Nouveau] Disabling -Warray-bounds for gcc-13 too
On April 23, 2023 10:36:24 AM PDT, Linus Torvalds
wrote:
>Kees,
> I made the mistake of upgrading my M2 Macbook Air to Fedora-38, and
>in the process I got gcc-13 which is not WERROR-clean because we only
>limited the 'array-bounds' warning to gcc-11 and gcc-12. But gcc-13
>has all the same issues.
>
>And I want to be able to do my arm64 builds with WERROR on still...
>
>I guess it never made much sense to hope it was going to go away
>without having a confirmation, so I just changed it to be gcc-11+.
Yeah, that's fine. GCC 13 released without having a fix for at least one
(hopefully last) known array-bounds vs jump threading bug:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109071
>And one of them is from you.
>
>In particular, commit 4076ea2419cf ("drm/nouveau/disp: Fix
>nvif_outp_acquire_dp() argument size") cannot possibly be right, It
>changes
>
> nvif_outp_acquire_dp(struct nvif_outp *outp, u8 dpcd[16],
>
>to
>
> nvif_outp_acquire_dp(struct nvif_outp *outp, u8 dpcd[DP_RECEIVER_CAP_SIZE],
>
>and then does
>
>memcpy(args.dp.dpcd, dpcd, sizeof(args.dp.dpcd));
>
>where that 'args.dp.dpcd' is a 16-byte array, and DP_RECEIVER_CAP_SIZE is 15.
Yeah, it was an incomplete fix. I sent the other half here, but it fell through
the cracks:
https://lore.kernel.org/lkml/20230204184307.never.825-k...@kernel.org/
>
>I think it's all entirely harmless from a code generation standpoint,
>because the 15-byte field will be padded out to 16 bytes in the
>structure that contains it, but it's most definitely buggy.
Right; between this, that GCC 13 wasn't released yet, and I had no feedback
from NV folks, I didn't chase down landing that fix.
>
>So that warning does find real cases of wrong code. But when those
>real cases are hidden by hundreds of lines of unfixable false
>positives, we don't have much choice.
Yup, totally agreed. The false positives I've looked at all seem to be similar
to the outstanding jump threading bug, so I'm hoping once that gets fixed we'll
finally have a good signal with that warning enabled. :)
-Kees
--
Kees Cook
[Nouveau] [PATCH] drm/nouveau/disp: More DP_RECEIVER_CAP_SIZE array fixes
More arrays (and arguments) for dcpd were set to 16, when it looks like
DP_RECEIVER_CAP_SIZE (15) should be used. Fix the remaining cases, seen
with GCC 13:
../drivers/gpu/drm/nouveau/nvif/outp.c: In function 'nvif_outp_acquire_dp':
../include/linux/fortify-string.h:57:33: warning: array subscript 'unsigned
char[16][0]' is partly outside array bounds of 'u8[15]' {aka 'unsigned
char[15]'} [-Warray-bounds=]
57 | #define __underlying_memcpy __builtin_memcpy
| ^
...
../drivers/gpu/drm/nouveau/nvif/outp.c:140:9: note: in expansion of macro
'memcpy'
140 | memcpy(args.dp.dpcd, dpcd, sizeof(args.dp.dpcd));
| ^~
../drivers/gpu/drm/nouveau/nvif/outp.c:130:49: note: object 'dpcd' of size [0,
15]
130 | nvif_outp_acquire_dp(struct nvif_outp *outp, u8
dpcd[DP_RECEIVER_CAP_SIZE],
|
~~~^~
Fixes: 813443721331 ("drm/nouveau/disp: move DP link config into acquire")
Cc: Ben Skeggs
Cc: Lyude Paul
Cc: Karol Herbst
Cc: David Airlie
Cc: Daniel Vetter
Cc: Dave Airlie
Cc: "Gustavo A. R. Silva"
Cc: dri-de...@lists.freedesktop.org
Cc: nouveau@lists.freedesktop.org
Signed-off-by: Kees Cook
---
drivers/gpu/drm/nouveau/include/nvif/if0012.h| 4 +++-
drivers/gpu/drm/nouveau/nvkm/engine/disp/outp.h | 3 ++-
drivers/gpu/drm/nouveau/nvkm/engine/disp/uoutp.c | 2 +-
3 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/include/nvif/if0012.h
b/drivers/gpu/drm/nouveau/include/nvif/if0012.h
index eb99d84eb844..16d4ad5023a3 100644
--- a/drivers/gpu/drm/nouveau/include/nvif/if0012.h
+++ b/drivers/gpu/drm/nouveau/include/nvif/if0012.h
@@ -2,6 +2,8 @@
#ifndef __NVIF_IF0012_H__
#define __NVIF_IF0012_H__
+#include
+
union nvif_outp_args {
struct nvif_outp_v0 {
__u8 version;
@@ -63,7 +65,7 @@ union nvif_outp_acquire_args {
__u8 hda;
__u8 mst;
__u8 pad04[4];
- __u8 dpcd[16];
+ __u8 dpcd[DP_RECEIVER_CAP_SIZE];
} dp;
};
} v0;
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/disp/outp.h
b/drivers/gpu/drm/nouveau/nvkm/engine/disp/outp.h
index b7631c1ab242..4e7f873f66e2 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/disp/outp.h
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/disp/outp.h
@@ -3,6 +3,7 @@
#define __NVKM_DISP_OUTP_H__
#include "priv.h"
+#include
#include
#include
#include
@@ -42,7 +43,7 @@ struct nvkm_outp {
bool aux_pwr_pu;
u8 lttpr[6];
u8 lttprs;
- u8 dpcd[16];
+ u8 dpcd[DP_RECEIVER_CAP_SIZE];
struct {
int dpcd; /* -1, or index into
SUPPORTED_LINK_RATES table */
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/disp/uoutp.c
b/drivers/gpu/drm/nouveau/nvkm/engine/disp/uoutp.c
index 4f0ca709c85a..fc283a4a1522 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/disp/uoutp.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/disp/uoutp.c
@@ -146,7 +146,7 @@ nvkm_uoutp_mthd_release(struct nvkm_outp *outp, void *argv,
u32 argc)
}
static int
-nvkm_uoutp_mthd_acquire_dp(struct nvkm_outp *outp, u8 dpcd[16],
+nvkm_uoutp_mthd_acquire_dp(struct nvkm_outp *outp, u8
dpcd[DP_RECEIVER_CAP_SIZE],
u8 link_nr, u8 link_bw, bool hda, bool mst)
{
int ret;
--
2.34.1
Re: [Nouveau] [PATCH] drm/nouveau/disp: Fix nvif_outp_acquire_dp() argument size
On Wed, Jan 25, 2023 at 04:24:19PM -0500, Lyude Paul wrote:
> Sorry! I've been pretty busy until now, this is:
>
> Reviewed-by: Lyude Paul
>
> Let me know if you've pushed it already or if you want me to push it to drm-
> misc
Either way is fine. I'm currently carrying it, but I can easily drop it
if you prefer it go via drm-misc.
Thanks!
-Kees
>
> On Wed, 2023-01-25 at 12:15 -0800, Kees Cook wrote:
> > Ping. I'll take this via my tree unless someone else wants to take it...
> >
> > On Sun, Nov 27, 2022 at 10:30:41AM -0800, Kees Cook wrote:
> > > Both Coverity and GCC with -Wstringop-overflow noticed that
> > > nvif_outp_acquire_dp() accidentally defined its second argument with 1
> > > additional element:
> > >
> > > drivers/gpu/drm/nouveau/dispnv50/disp.c: In function
> > > 'nv50_pior_atomic_enable':
> > > drivers/gpu/drm/nouveau/dispnv50/disp.c:1813:17: error:
> > > 'nvif_outp_acquire_dp' accessing 16 bytes in a region of size 15
> > > [-Werror=stringop-overflow=]
> > > 1813 | nvif_outp_acquire_dp(_encoder->outp,
> > > nv_encoder->dp.dpcd, 0, 0, false, false);
> > > |
> > > ^~~
> > > drivers/gpu/drm/nouveau/dispnv50/disp.c:1813:17: note: referencing
> > > argument 2 of type 'u8[16]' {aka 'unsigned char[16]'}
> > > drivers/gpu/drm/nouveau/include/nvif/outp.h:24:5: note: in a call to
> > > function 'nvif_outp_acquire_dp'
> > >24 | int nvif_outp_acquire_dp(struct nvif_outp *, u8 dpcd[16],
> > > | ^~~~
> > >
> > > Avoid these warnings by defining the argument size using the matching
> > > define (DP_RECEIVER_CAP_SIZE, 15) instead of having it be a literal
> > > (and incorrect) value (16).
> > >
> > > Reported-by: coverity-bot
> > > Addresses-Coverity-ID: 1527269 ("Memory - corruptions")
> > > Addresses-Coverity-ID: 1527268 ("Memory - corruptions")
> > > Link: https://lore.kernel.org/lkml/202211100848.FFBA2432@keescook/
> > > Link: https://lore.kernel.org/lkml/202211100848.F4C2819BB@keescook/
> > > Fixes: 813443721331 ("drm/nouveau/disp: move DP link config into acquire")
> > > Cc: Ben Skeggs
> > > Cc: Karol Herbst
> > > Cc: Lyude Paul
> > > Cc: David Airlie
> > > Cc: Daniel Vetter
> > > Cc: Dave Airlie
> > > Cc: "Gustavo A. R. Silva"
> > > Cc: dri-de...@lists.freedesktop.org
> > > Cc: nouveau@lists.freedesktop.org
> > > Signed-off-by: Kees Cook
> > > ---
> > > drivers/gpu/drm/nouveau/include/nvif/outp.h | 3 ++-
> > > drivers/gpu/drm/nouveau/nvif/outp.c | 2 +-
> > > 2 files changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/nouveau/include/nvif/outp.h
> > > b/drivers/gpu/drm/nouveau/include/nvif/outp.h
> > > index 45daadec3c0c..fa76a7b5e4b3 100644
> > > --- a/drivers/gpu/drm/nouveau/include/nvif/outp.h
> > > +++ b/drivers/gpu/drm/nouveau/include/nvif/outp.h
> > > @@ -3,6 +3,7 @@
> > > #define __NVIF_OUTP_H__
> > > #include
> > > #include
> > > +#include
> > > struct nvif_disp;
> > >
> > > struct nvif_outp {
> > > @@ -21,7 +22,7 @@ int nvif_outp_acquire_rgb_crt(struct nvif_outp *);
> > > int nvif_outp_acquire_tmds(struct nvif_outp *, int head,
> > > bool hdmi, u8 max_ac_packet, u8 rekey, u8 scdc, bool
> > > hda);
> > > int nvif_outp_acquire_lvds(struct nvif_outp *, bool dual, bool bpc8);
> > > -int nvif_outp_acquire_dp(struct nvif_outp *, u8 dpcd[16],
> > > +int nvif_outp_acquire_dp(struct nvif_outp *outp, u8
> > > dpcd[DP_RECEIVER_CAP_SIZE],
> > >int link_nr, int link_bw, bool hda, bool mst);
> > > void nvif_outp_release(struct nvif_outp *);
> > > int nvif_outp_infoframe(struct nvif_outp *, u8 type, struct
> > > nvif_outp_infoframe_v0 *, u32 size);
> > > diff --git a/drivers/gpu/drm/nouveau/nvif/outp.c
> > > b/drivers/gpu/drm/nouveau/nvif/outp.c
> > > index 7da39f1eae9f..c24bc5eae3ec 100644
> > > --- a/drivers/gpu/drm/nouveau/nvif/outp.c
> > > +++ b/drivers/gpu/drm/nouveau/nvif/outp.c
> > > @@ -127,7 +127,7 @@ nvif_outp_acquire(struct nvif_outp *outp, u8 proto,
> > > struct nvif_outp_acquire_v0
> > > }
> > >
> > > int
> > > -nvif_outp_acquire_dp(struct nvif_outp *outp, u8 dpcd[16],
> > > +nvif_outp_acquire_dp(struct nvif_outp *outp, u8
> > > dpcd[DP_RECEIVER_CAP_SIZE],
> > >int link_nr, int link_bw, bool hda, bool mst)
> > > {
> > > struct nvif_outp_acquire_v0 args;
> > > --
> > > 2.34.1
> > >
> >
>
> --
> Cheers,
> Lyude Paul (she/her)
> Software Engineer at Red Hat
>
--
Kees Cook
Re: [Nouveau] [PATCH] drm/nouveau/disp: Fix nvif_outp_acquire_dp() argument size
Ping. I'll take this via my tree unless someone else wants to take it...
On Sun, Nov 27, 2022 at 10:30:41AM -0800, Kees Cook wrote:
> Both Coverity and GCC with -Wstringop-overflow noticed that
> nvif_outp_acquire_dp() accidentally defined its second argument with 1
> additional element:
>
> drivers/gpu/drm/nouveau/dispnv50/disp.c: In function
> 'nv50_pior_atomic_enable':
> drivers/gpu/drm/nouveau/dispnv50/disp.c:1813:17: error:
> 'nvif_outp_acquire_dp' accessing 16 bytes in a region of size 15
> [-Werror=stringop-overflow=]
> 1813 | nvif_outp_acquire_dp(_encoder->outp,
> nv_encoder->dp.dpcd, 0, 0, false, false);
> |
> ^~~
> drivers/gpu/drm/nouveau/dispnv50/disp.c:1813:17: note: referencing argument 2
> of type 'u8[16]' {aka 'unsigned char[16]'}
> drivers/gpu/drm/nouveau/include/nvif/outp.h:24:5: note: in a call to function
> 'nvif_outp_acquire_dp'
>24 | int nvif_outp_acquire_dp(struct nvif_outp *, u8 dpcd[16],
> | ^~~~
>
> Avoid these warnings by defining the argument size using the matching
> define (DP_RECEIVER_CAP_SIZE, 15) instead of having it be a literal
> (and incorrect) value (16).
>
> Reported-by: coverity-bot
> Addresses-Coverity-ID: 1527269 ("Memory - corruptions")
> Addresses-Coverity-ID: 1527268 ("Memory - corruptions")
> Link: https://lore.kernel.org/lkml/202211100848.FFBA2432@keescook/
> Link: https://lore.kernel.org/lkml/202211100848.F4C2819BB@keescook/
> Fixes: 813443721331 ("drm/nouveau/disp: move DP link config into acquire")
> Cc: Ben Skeggs
> Cc: Karol Herbst
> Cc: Lyude Paul
> Cc: David Airlie
> Cc: Daniel Vetter
> Cc: Dave Airlie
> Cc: "Gustavo A. R. Silva"
> Cc: dri-de...@lists.freedesktop.org
> Cc: nouveau@lists.freedesktop.org
> Signed-off-by: Kees Cook
> ---
> drivers/gpu/drm/nouveau/include/nvif/outp.h | 3 ++-
> drivers/gpu/drm/nouveau/nvif/outp.c | 2 +-
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/include/nvif/outp.h
> b/drivers/gpu/drm/nouveau/include/nvif/outp.h
> index 45daadec3c0c..fa76a7b5e4b3 100644
> --- a/drivers/gpu/drm/nouveau/include/nvif/outp.h
> +++ b/drivers/gpu/drm/nouveau/include/nvif/outp.h
> @@ -3,6 +3,7 @@
> #define __NVIF_OUTP_H__
> #include
> #include
> +#include
> struct nvif_disp;
>
> struct nvif_outp {
> @@ -21,7 +22,7 @@ int nvif_outp_acquire_rgb_crt(struct nvif_outp *);
> int nvif_outp_acquire_tmds(struct nvif_outp *, int head,
> bool hdmi, u8 max_ac_packet, u8 rekey, u8 scdc, bool
> hda);
> int nvif_outp_acquire_lvds(struct nvif_outp *, bool dual, bool bpc8);
> -int nvif_outp_acquire_dp(struct nvif_outp *, u8 dpcd[16],
> +int nvif_outp_acquire_dp(struct nvif_outp *outp, u8
> dpcd[DP_RECEIVER_CAP_SIZE],
>int link_nr, int link_bw, bool hda, bool mst);
> void nvif_outp_release(struct nvif_outp *);
> int nvif_outp_infoframe(struct nvif_outp *, u8 type, struct
> nvif_outp_infoframe_v0 *, u32 size);
> diff --git a/drivers/gpu/drm/nouveau/nvif/outp.c
> b/drivers/gpu/drm/nouveau/nvif/outp.c
> index 7da39f1eae9f..c24bc5eae3ec 100644
> --- a/drivers/gpu/drm/nouveau/nvif/outp.c
> +++ b/drivers/gpu/drm/nouveau/nvif/outp.c
> @@ -127,7 +127,7 @@ nvif_outp_acquire(struct nvif_outp *outp, u8 proto,
> struct nvif_outp_acquire_v0
> }
>
> int
> -nvif_outp_acquire_dp(struct nvif_outp *outp, u8 dpcd[16],
> +nvif_outp_acquire_dp(struct nvif_outp *outp, u8 dpcd[DP_RECEIVER_CAP_SIZE],
>int link_nr, int link_bw, bool hda, bool mst)
> {
> struct nvif_outp_acquire_v0 args;
> --
> 2.34.1
>
--
Kees Cook
Re: [Nouveau] [PATCH][next] drm/nouveau/nvkm: Replace zero-length array with flexible-array member
On Mon, Jan 09, 2023 at 07:39:30PM -0600, Gustavo A. R. Silva wrote: > Zero-length arrays are deprecated[1] and we are moving towards > adopting C99 flexible-array members instead. So, replace zero-length > array declaration in struct nvfw_hs_load_header_v2 with flex-array > member. > > This helps with the ongoing efforts to tighten the FORTIFY_SOURCE > routines on memcpy() and help us make progress towards globally > enabling -fstrict-flex-arrays=3 [2]. > > Link: > https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays > [1] > Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [2] > Link: https://github.com/KSPP/linux/issues/78 > Signed-off-by: Gustavo A. R. Silva Reviewed-by: Kees Cook -- Kees Cook
[Nouveau] [RESEND][PATCH] drm/nouveau/fb/ga102: Replace zero-length array of trailing structs with flex-array
Zero-length arrays are deprecated[1] and are being replaced with
flexible array members in support of the ongoing efforts to tighten the
FORTIFY_SOURCE routines on memcpy(), correctly instrument array indexing
with UBSAN_BOUNDS, and to globally enable -fstrict-flex-arrays=3.
Replace zero-length array with flexible-array member.
This results in no differences in binary output.
[1] https://github.com/KSPP/linux/issues/78
Cc: Ben Skeggs
Cc: Karol Herbst
Cc: Lyude Paul
Cc: David Airlie
Cc: Daniel Vetter
Cc: Gourav Samaiya
Cc: "Gustavo A. R. Silva"
Cc: dri-de...@lists.freedesktop.org
Cc: nouveau@lists.freedesktop.org
Signed-off-by: Kees Cook
---
Sent before as:
https://lore.kernel.org/all/20221118211207.never.039-k...@kernel.org/
---
drivers/gpu/drm/nouveau/include/nvfw/hs.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/include/nvfw/hs.h
b/drivers/gpu/drm/nouveau/include/nvfw/hs.h
index 8c4cd08a7b5f..8b58b668fc0c 100644
--- a/drivers/gpu/drm/nouveau/include/nvfw/hs.h
+++ b/drivers/gpu/drm/nouveau/include/nvfw/hs.h
@@ -52,7 +52,7 @@ struct nvfw_hs_load_header_v2 {
struct {
u32 offset;
u32 size;
- } app[0];
+ } app[];
};
const struct nvfw_hs_load_header_v2 *nvfw_hs_load_header_v2(struct nvkm_subdev
*, const void *);
--
2.34.1
[Nouveau] [PATCH] drm/nouveau/disp: Fix nvif_outp_acquire_dp() argument size
Both Coverity and GCC with -Wstringop-overflow noticed that
nvif_outp_acquire_dp() accidentally defined its second argument with 1
additional element:
drivers/gpu/drm/nouveau/dispnv50/disp.c: In function 'nv50_pior_atomic_enable':
drivers/gpu/drm/nouveau/dispnv50/disp.c:1813:17: error: 'nvif_outp_acquire_dp'
accessing 16 bytes in a region of size 15 [-Werror=stringop-overflow=]
1813 | nvif_outp_acquire_dp(_encoder->outp,
nv_encoder->dp.dpcd, 0, 0, false, false);
|
^~~
drivers/gpu/drm/nouveau/dispnv50/disp.c:1813:17: note: referencing argument 2
of type 'u8[16]' {aka 'unsigned char[16]'}
drivers/gpu/drm/nouveau/include/nvif/outp.h:24:5: note: in a call to function
'nvif_outp_acquire_dp'
24 | int nvif_outp_acquire_dp(struct nvif_outp *, u8 dpcd[16],
| ^~~~
Avoid these warnings by defining the argument size using the matching
define (DP_RECEIVER_CAP_SIZE, 15) instead of having it be a literal
(and incorrect) value (16).
Reported-by: coverity-bot
Addresses-Coverity-ID: 1527269 ("Memory - corruptions")
Addresses-Coverity-ID: 1527268 ("Memory - corruptions")
Link: https://lore.kernel.org/lkml/202211100848.FFBA2432@keescook/
Link: https://lore.kernel.org/lkml/202211100848.F4C2819BB@keescook/
Fixes: 813443721331 ("drm/nouveau/disp: move DP link config into acquire")
Cc: Ben Skeggs
Cc: Karol Herbst
Cc: Lyude Paul
Cc: David Airlie
Cc: Daniel Vetter
Cc: Dave Airlie
Cc: "Gustavo A. R. Silva"
Cc: dri-de...@lists.freedesktop.org
Cc: nouveau@lists.freedesktop.org
Signed-off-by: Kees Cook
---
drivers/gpu/drm/nouveau/include/nvif/outp.h | 3 ++-
drivers/gpu/drm/nouveau/nvif/outp.c | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/include/nvif/outp.h
b/drivers/gpu/drm/nouveau/include/nvif/outp.h
index 45daadec3c0c..fa76a7b5e4b3 100644
--- a/drivers/gpu/drm/nouveau/include/nvif/outp.h
+++ b/drivers/gpu/drm/nouveau/include/nvif/outp.h
@@ -3,6 +3,7 @@
#define __NVIF_OUTP_H__
#include
#include
+#include
struct nvif_disp;
struct nvif_outp {
@@ -21,7 +22,7 @@ int nvif_outp_acquire_rgb_crt(struct nvif_outp *);
int nvif_outp_acquire_tmds(struct nvif_outp *, int head,
bool hdmi, u8 max_ac_packet, u8 rekey, u8 scdc, bool
hda);
int nvif_outp_acquire_lvds(struct nvif_outp *, bool dual, bool bpc8);
-int nvif_outp_acquire_dp(struct nvif_outp *, u8 dpcd[16],
+int nvif_outp_acquire_dp(struct nvif_outp *outp, u8 dpcd[DP_RECEIVER_CAP_SIZE],
int link_nr, int link_bw, bool hda, bool mst);
void nvif_outp_release(struct nvif_outp *);
int nvif_outp_infoframe(struct nvif_outp *, u8 type, struct
nvif_outp_infoframe_v0 *, u32 size);
diff --git a/drivers/gpu/drm/nouveau/nvif/outp.c
b/drivers/gpu/drm/nouveau/nvif/outp.c
index 7da39f1eae9f..c24bc5eae3ec 100644
--- a/drivers/gpu/drm/nouveau/nvif/outp.c
+++ b/drivers/gpu/drm/nouveau/nvif/outp.c
@@ -127,7 +127,7 @@ nvif_outp_acquire(struct nvif_outp *outp, u8 proto, struct
nvif_outp_acquire_v0
}
int
-nvif_outp_acquire_dp(struct nvif_outp *outp, u8 dpcd[16],
+nvif_outp_acquire_dp(struct nvif_outp *outp, u8 dpcd[DP_RECEIVER_CAP_SIZE],
int link_nr, int link_bw, bool hda, bool mst)
{
struct nvif_outp_acquire_v0 args;
--
2.34.1
[Nouveau] [PATCH] drm/nouveau/fb/ga102: Replace zero-length array of trailing structs with flex-array
Zero-length arrays are deprecated[1] and are being replaced with
flexible array members in support of the ongoing efforts to tighten the
FORTIFY_SOURCE routines on memcpy(), correctly instrument array indexing
with UBSAN_BOUNDS, and to globally enable -fstrict-flex-arrays=3.
Replace zero-length array with flexible-array member.
This results in no differences in binary output.
[1] https://github.com/KSPP/linux/issues/78
Cc: Ben Skeggs
Cc: Karol Herbst
Cc: Lyude Paul
Cc: David Airlie
Cc: Daniel Vetter
Cc: Gourav Samaiya
Cc: "Gustavo A. R. Silva"
Cc: dri-de...@lists.freedesktop.org
Cc: nouveau@lists.freedesktop.org
Signed-off-by: Kees Cook
---
drivers/gpu/drm/nouveau/include/nvfw/hs.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/include/nvfw/hs.h
b/drivers/gpu/drm/nouveau/include/nvfw/hs.h
index 8c4cd08a7b5f..8b58b668fc0c 100644
--- a/drivers/gpu/drm/nouveau/include/nvfw/hs.h
+++ b/drivers/gpu/drm/nouveau/include/nvfw/hs.h
@@ -52,7 +52,7 @@ struct nvfw_hs_load_header_v2 {
struct {
u32 offset;
u32 size;
- } app[0];
+ } app[];
};
const struct nvfw_hs_load_header_v2 *nvfw_hs_load_header_v2(struct nvkm_subdev
*, const void *);
--
2.34.1
Re: [Nouveau] Coverity: nouveau_dp_irq(): Null pointer dereferences
On Fri, Nov 11, 2022 at 09:06:54PM +0100, Karol Herbst wrote:
> On Fri, Nov 11, 2022 at 8:21 PM Kees Cook wrote:
> >
> > On Fri, Nov 11, 2022 at 11:13:17AM +0200, Jani Nikula wrote:
> > > On Thu, 10 Nov 2022, coverity-bot wrote:
> > > > Hello!
> > > >
> > > > This is an experimental semi-automated report about issues detected by
> > > > Coverity from a scan of next-20221110 as part of the linux-next scan
> > > > project:
> > > > https://scan.coverity.com/projects/linux-next-weekly-scan
> > > >
> > > > You're getting this email because you were associated with the
> > > > identified
> > > > lines of code (noted below) that were touched by commits:
> > > >
> > > > Mon Aug 31 19:10:08 2020 -0400
> > > > a0922278f83e ("drm/nouveau/kms/nv50-: Refactor and cleanup DP HPD
> > > > handling")
> > >
> > > Hi Kees, this looks like a good idea, but maybe double check the Cc list
> > > generation? I was Cc'd on four mails today that I thought were
> > > irrelevant to me.
> >
> > Hi!
> >
> > Heh, I was recently asked to _expand_ the CC list. :)
> >
> > For these last pass of reports, I added a get_maintainers.pl run to the
> > identified commit. In this instance, the commit touched:
> >
> > drivers/gpu/drm/nouveau/dispnv04/disp.c |6 +
> > drivers/gpu/drm/nouveau/dispnv50/disp.c | 192
> > ++--
> > drivers/gpu/drm/nouveau/nouveau_connector.c | 14 ---
> > drivers/gpu/drm/nouveau/nouveau_display.c |2
> > drivers/gpu/drm/nouveau/nouveau_display.h |2
> > drivers/gpu/drm/nouveau/nouveau_dp.c| 132
> > -
> > drivers/gpu/drm/nouveau/nouveau_encoder.h | 33 +++-
> > 7 files changed, 244 insertions(+), 137 deletions(-)
> >
> > And the get_maintainers.pl rationale was:
> >
> > Ben Skeggs (supporter:DRM DRIVER FOR NVIDIA
> > GEFORCE/QUADRO
> > GPUS,commit_signer:1/1=100%,commit_signer:6/16=38%,authored:4/16=25%,added_lines:23/124=19%,removed_lines:36/152=24%)
> > Karol Herbst (supporter:DRM DRIVER FOR NVIDIA
> > GEFORCE/QUADRO GPUS,commit_signer:2/1=100%)
> > Lyude Paul (supporter:DRM DRIVER FOR NVIDIA
> > GEFORCE/QUADRO
> > GPUS,commit_signer:9/16=56%,authored:6/16=38%,added_lines:92/124=74%,removed_lines:107/152=70%)
> > David Airlie (maintainer:DRM DRIVERS)
> > Daniel Vetter (maintainer:DRM DRIVERS)
> > Ilia Mirkin
> > (commit_signer:1/1=100%,authored:1/1=100%,added_lines:2/2=100%,removed_lines:2/2=100%)
> > "Nathan E. Egge" (commit_signer:1/1=100%)
> > Jani Nikula (commit_signer:6/16=38%)
> > Dave Airlie (commit_signer:5/16=31%)
> > Thomas Zimmermann
> > (commit_signer:4/16=25%,authored:4/16=25%)
> > dri-de...@lists.freedesktop.org (open list:DRM DRIVER FOR NVIDIA
> > GEFORCE/QUADRO GPUS)
> > nouveau@lists.freedesktop.org (open list:DRM DRIVER FOR NVIDIA
> > GEFORCE/QUADRO GPUS)
> >
>
> I'd say it's good enough to message supporters and the mailing lists
> for at least Nouveau code, maybe even all drm drivers.
i.e. leave out the commit_signer hits?
> Not sure what to do about actual maintainers, but I doubt Dave and
> Daniel want to be CCed on every Coverity report here either.
I updated the CC logic based on this feedback:
https://lore.kernel.org/linux-hardening/87h6zgfub4@kernel.org/
So maybe just mailing lists?
--
Kees Cook
Re: [Nouveau] Coverity: nouveau_dp_irq(): Null pointer dereferences
On Fri, Nov 11, 2022 at 11:13:17AM +0200, Jani Nikula wrote:
> On Thu, 10 Nov 2022, coverity-bot wrote:
> > Hello!
> >
> > This is an experimental semi-automated report about issues detected by
> > Coverity from a scan of next-20221110 as part of the linux-next scan
> > project:
> > https://scan.coverity.com/projects/linux-next-weekly-scan
> >
> > You're getting this email because you were associated with the identified
> > lines of code (noted below) that were touched by commits:
> >
> > Mon Aug 31 19:10:08 2020 -0400
> > a0922278f83e ("drm/nouveau/kms/nv50-: Refactor and cleanup DP HPD
> > handling")
>
> Hi Kees, this looks like a good idea, but maybe double check the Cc list
> generation? I was Cc'd on four mails today that I thought were
> irrelevant to me.
Hi!
Heh, I was recently asked to _expand_ the CC list. :)
For these last pass of reports, I added a get_maintainers.pl run to the
identified commit. In this instance, the commit touched:
drivers/gpu/drm/nouveau/dispnv04/disp.c |6 +
drivers/gpu/drm/nouveau/dispnv50/disp.c | 192
++--
drivers/gpu/drm/nouveau/nouveau_connector.c | 14 ---
drivers/gpu/drm/nouveau/nouveau_display.c |2
drivers/gpu/drm/nouveau/nouveau_display.h |2
drivers/gpu/drm/nouveau/nouveau_dp.c| 132
-
drivers/gpu/drm/nouveau/nouveau_encoder.h | 33 +++-
7 files changed, 244 insertions(+), 137 deletions(-)
And the get_maintainers.pl rationale was:
Ben Skeggs (supporter:DRM DRIVER FOR NVIDIA GEFORCE/QUADRO
GPUS,commit_signer:1/1=100%,commit_signer:6/16=38%,authored:4/16=25%,added_lines:23/124=19%,removed_lines:36/152=24%)
Karol Herbst (supporter:DRM DRIVER FOR NVIDIA
GEFORCE/QUADRO GPUS,commit_signer:2/1=100%)
Lyude Paul (supporter:DRM DRIVER FOR NVIDIA GEFORCE/QUADRO
GPUS,commit_signer:9/16=56%,authored:6/16=38%,added_lines:92/124=74%,removed_lines:107/152=70%)
David Airlie (maintainer:DRM DRIVERS)
Daniel Vetter (maintainer:DRM DRIVERS)
Ilia Mirkin
(commit_signer:1/1=100%,authored:1/1=100%,added_lines:2/2=100%,removed_lines:2/2=100%)
"Nathan E. Egge" (commit_signer:1/1=100%)
Jani Nikula (commit_signer:6/16=38%)
Dave Airlie (commit_signer:5/16=31%)
Thomas Zimmermann
(commit_signer:4/16=25%,authored:4/16=25%)
dri-de...@lists.freedesktop.org (open list:DRM DRIVER FOR NVIDIA GEFORCE/QUADRO
GPUS)
nouveau@lists.freedesktop.org (open list:DRM DRIVER FOR NVIDIA GEFORCE/QUADRO
GPUS)
--
Kees Cook
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Wed, Mar 02, 2022 at 10:29:31AM +0100, Rasmus Villemoes wrote:
> This won't help the current issue (because it doesn't exist and might
> never), but just in case some compiler people are listening, I'd like to
> have some sort of way to tell the compiler "treat this variable as
> uninitialized from here on". So one could do
>
> #define kfree(p) do { __kfree(p); __magic_uninit(p); } while (0)
>
> with __magic_uninit being a magic no-op that doesn't affect the
> semantics of the code, but could be used by the compiler's "[is/may be]
> used uninitialized" machinery to flag e.g. double frees on some odd
> error path etc. It would probably only work for local automatic
> variables, but it should be possible to just ignore the hint if p is
> some expression like foo->bar or has side effects. If we had that, the
> end-of-loop test could include that to "uninitialize" the iterator.
I've long wanted to change kfree() to explicitly set pointers to NULL on
free. https://github.com/KSPP/linux/issues/87
The thing stopping a trivial transformation of kfree() is:
kfree(get_some_pointer());
I would argue, though, that the above is poor form: the thing holding
the pointer should be the thing freeing it, so these cases should be
refactored and kfree() could do the NULLing by default.
Quoting myself in the above issue:
Without doing massive tree-wide changes, I think we need compiler
support. If we had something like __builtin_is_lvalue(), we could
distinguish function returns from lvalues. For example, right now a
common case are things like:
kfree(get_some_ptr());
But if we could at least gain coverage of the lvalue cases, and detect
them statically at compile-time, we could do:
#define __kfree_and_null(x) do { __kfree(*x); *x = NULL; } while (0)
#define kfree(x) __builtin_choose_expr(__builtin_is_lvalue(x),
__kfree_and_null(&(x)), __kfree(x))
Alternatively, we could do a tree-wide change of the former case (findable
with Coccinelle) and change them into something like kfree_no_null()
and redefine kfree() itself:
#define kfree_no_null(x) do { void *__ptr = (x); __kfree(__ptr); } while (0)
#define kfree(x) do { __kfree(x); x = NULL; } while (0)
--
Kees Cook
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Wed, Mar 02, 2022 at 12:18:45PM -0800, Linus Torvalds wrote: > On Wed, Mar 2, 2022 at 12:07 PM Kees Cook wrote: > > > > I've long wanted to change kfree() to explicitly set pointers to NULL on > > free. https://github.com/KSPP/linux/issues/87 > > We've had this discussion with the gcc people in the past, and gcc > actually has some support for it, but it's sadly tied to the actual > function name (ie gcc has some special-casing for "free()") > > See > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94527 > > for some of that discussion. > > Oh, and I see some patch actually got merged since I looked there last > so that you can mark "deallocator" functions, but I think it's only > for the context matching, not for actually killing accesses to the > pointer afterwards. Ah! I missed that getting added in GCC 11. But yes, there it is: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-malloc-function-attribute Hah, now we may need to split __malloc from __alloc_size. ;) I'd still like the NULL assignment behavior, though, since some things can easily avoid static analysis. -- Kees Cook
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 04:45:11PM -0800, Linus Torvalds wrote: > Really. The "-Wshadow doesn't work on the kernel" is not some new > issue, because you have to do completely insane things to the source > code to enable it. The first big glitch with -Wshadow was with shadowed global variables. GCC 4.8 fixed that, but it still yells about shadowed functions. What _almost_ works is -Wshadow=local. At first glace, all the warnings look solvable, but then one will eventually discover __wait_event() and associated macros that mix when and how deeply it intentionally shadows variables. :) Another way to try to catch misused shadow variables is -Wunused-but-set-varible, but it, too, has tons of false positives. I tried to capture some of the rationale and research here: https://github.com/KSPP/linux/issues/152 -- Kees Cook
Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Tue, Mar 01, 2022 at 12:28:15PM +0100, Jakob Koschel wrote: > Based on the coccinelle script there are ~480 cases that need fixing > in total. I'll now finish all of them and then split them by > submodules as Greg suggested and repost a patch set per submodule. > Sounds good? To help with this splitting, see: https://github.com/kees/kernel-tools/blob/trunk/split-on-maintainer It's not perfect, but it'll get you really close. For example, if you had a single big tree-wide patch applied to your tree: $ rm 0*.patch $ git format-patch -1 HEAD $ mv 0*.patch treewide.patch $ split-on-maintainer treewide.patch $ ls 0*.patch If you have a build log before the patch that spits out warnings, the --build-log argument can extract those warnings on a per-file basis, too (though this can be fragile). -- Kees Cook
Re: [Nouveau] [PATCH][next] treewide: Replace zero-length arrays with flexible-array members
On Tue, Feb 15, 2022 at 11:47:43AM -0600, Gustavo A. R. Silva wrote:
> There is a regular need in the kernel to provide a way to declare
> having a dynamically sized set of trailing elements in a structure.
> Kernel code should always use “flexible array members”[1] for these
> cases. The older style of one-element or zero-length arrays should
> no longer be used[2].
>
> This code was transformed with the help of Coccinelle:
> (next-20220214$ spatch --jobs $(getconf _NPROCESSORS_ONLN) --sp-file
> script.cocci --include-headers --dir . > output.patch)
>
> @@
> identifier S, member, array;
> type T1, T2;
> @@
>
> struct S {
> ...
> T1 member;
> T2 array[
> - 0
> ];
> };
These all look trivially correct to me. Only two didn't have the end of
the struct visible in the patch, and checking those showed them to be
trailing members as well, so:
Reviewed-by: Kees Cook
--
Kees Cook
Re: [Nouveau] [PATCH][next] nouveau/svm: Use kvcalloc() instead of kvzalloc()
On Wed, Sep 29, 2021 at 05:28:47AM +0200, Karol Herbst wrote: > Lack of documentation inside Linux here is a bit annoying, but do I > understand it correctly, that the main (and probably only) difference > is that kvcalloc checks whether the multiplication overflows and > returns NULL in this case? That's correct. :) > On Wed, Sep 29, 2021 at 12:21 AM Gustavo A. R. Silva > wrote: > > > > Use 2-factor argument form kvcalloc() instead of kvzalloc(). > > > > Link: https://github.com/KSPP/linux/issues/162 > > Signed-off-by: Gustavo A. R. Silva Reviewed-by: Kees Cook > > --- > > drivers/gpu/drm/nouveau/nouveau_svm.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c > > b/drivers/gpu/drm/nouveau/nouveau_svm.c > > index b0c3422cb01f..1a896a24288a 100644 > > --- a/drivers/gpu/drm/nouveau/nouveau_svm.c > > +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c > > @@ -992,7 +992,7 @@ nouveau_svm_fault_buffer_ctor(struct nouveau_svm *svm, > > s32 oclass, int id) > > if (ret) > > return ret; > > > > - buffer->fault = kvzalloc(sizeof(*buffer->fault) * buffer->entries, > > GFP_KERNEL); > > + buffer->fault = kvcalloc(sizeof(*buffer->fault), buffer->entries, > > GFP_KERNEL); > > if (!buffer->fault) > > return -ENOMEM; > > > > -- > > 2.27.0 > > > -- Kees Cook
Re: [Nouveau] [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, Nov 23, 2020 at 05:32:51PM -0800, Nick Desaulniers wrote:
> On Sun, Nov 22, 2020 at 8:17 AM Kees Cook wrote:
> >
> > On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote:
> > > If none of the 140 patches here fix a real bug, and there is no change
> > > to machine code then it sounds to me like a W=2 kind of a warning.
> >
> > FWIW, this series has found at least one bug so far:
> > https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=kr...@mail.gmail.com/
>
> So looks like the bulk of these are:
> switch (x) {
> case 0:
> ++x;
> default:
> break;
> }
>
> I have a patch that fixes those up for clang:
> https://reviews.llvm.org/D91895
I still think this isn't right -- it's a case statement that runs off
the end without an explicit flow control determination. I think Clang is
right to warn for these, and GCC should also warn.
--
Kees Cook
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, Nov 23, 2020 at 08:31:30AM -0800, James Bottomley wrote: > Really, no ... something which produces no improvement has no value at > all ... we really shouldn't be wasting maintainer time with it because > it has a cost to merge. I'm not sure we understand where the balance > lies in value vs cost to merge but I am confident in the zero value > case. What? We can't measure how many future bugs aren't introduced because the kernel requires explicit case flow-control statements for all new code. We already enable -Wimplicit-fallthrough globally, so that's not the discussion. The issue is that Clang is (correctly) even more strict than GCC for this, so these are the remaining ones to fix for full Clang coverage too. People have spent more time debating this already than it would have taken to apply the patches. :) This is about robustness and language wrangling. It's a big code-base, and this is the price of our managing technical debt for permanent robustness improvements. (The numbers I ran from Gustavo's earlier patches were that about 10% of the places adjusted were identified as legitimate bugs being fixed. This final series may be lower, but there are still bugs being found from it -- we need to finish this and shut the door on it for good.) -- Kees Cook ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Tue, Nov 24, 2020 at 11:05:35PM -0800, James Bottomley wrote: > Now, what we have seems to be about 6 cases (at least what's been shown > in this thread) where a missing break would cause potentially user > visible issues. That means the value of this isn't zero, but it's not > a no-brainer massive win either. That's why I think asking what we've > invested vs the return isn't a useless exercise. The number is much higher[1]. If it were 6 in the entire history of the kernel, I would agree with you. :) Some were fixed _before_ Gustavo's effort too, which I also count towards the idea of "this is a dangerous weakness in C, and now we have stopped it forever." > But the broader point I'm making is just because the compiler people > come up with a shiny new warning doesn't necessarily mean the problem > it's detecting is one that causes us actual problems in the code base. > I'd really be happier if we had a theory about what classes of CVE or > bug we could eliminate before we embrace the next new warning. But we did! It was long ago justified and documented[2], and even links to the CWE[3] for it. This wasn't random joy over discovering a new warning we could turn on, this was turning on a warning that the compiler folks finally gave us to handle an entire class of flaws. If we need to update the code-base to address it not a useful debate -- that was settled already, even if you're only discovering it now. :P. This last patch set is about finishing that work for Clang, which is correctly even more strict than GCC. -Kees [1] https://outflux.net/slides/2019/lss/kspp.pdf calls out specific numbers (about 6.5% of the patches fixed missing breaks): v4.19: 3 of 129 v4.20: 2 of 59 v5.0: 3 of 56 v5.1: 10 of 100 v5.2: 6 of 71 v5.3: 7 of 69 And in the history of the kernel, it's been an ongoing source of flaws: $ l --no-merges | grep -i 'missing break' | wc -l 185 The frequency of such errors being "naturally" found was pretty steady until the static checkers started warning, and then it was on the rise, but the full effort flushed the rest out, and now it's dropped to almost zero: 1 v2.6.12 3 v2.6.16.28 1 v2.6.17 1 v2.6.19 2 v2.6.21 1 v2.6.22 3 v2.6.24 3 v2.6.29 1 v2.6.32 1 v2.6.33 1 v2.6.35 4 v2.6.36 3 v2.6.38 2 v2.6.39 7 v3.0 2 v3.1 2 v3.2 2 v3.3 3 v3.4 1 v3.5 8 v3.6 7 v3.7 3 v3.8 6 v3.9 3 v3.10 2 v3.11 5 v3.12 5 v3.13 2 v3.14 4 v3.15 2 v3.16 3 v3.17 2 v3.18 2 v3.19 1 v4.0 2 v4.1 5 v4.2 4 v4.5 5 v4.7 6 v4.8 1 v4.9 3 v4.10 2 v4.11 6 v4.12 3 v4.13 2 v4.14 5 v4.15 2 v4.16 7 v4.18 2 v4.19 6 v4.20 3 v5.0 12 v5.1 3 v5.2 4 v5.3 2 v5.4 1 v5.8 And the reason it's fully zero, is because we still have the cases we're cleaning up right now. Even this last one from v5.8 is specifically of the same type this series addresses: case 4: color_index = TrueCModeIndex; + break; default: return; } [2] https://www.kernel.org/doc/html/latest/process/deprecated.html#implicit-switch-case-fall-through All switch/case blocks must end in one of: break; fallthrough; continue; goto ; return [expression]; [3] https://cwe.mitre.org/data/definitions/484.html -- Kees Cook ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH 000/141] Fix fall-through warnings for Clang
On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote: > On Fri, 20 Nov 2020 11:30:40 -0800 Kees Cook wrote: > > On Fri, Nov 20, 2020 at 10:53:44AM -0800, Jakub Kicinski wrote: > > > On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote: > > > > This series aims to fix almost all remaining fall-through warnings in > > > > order to enable -Wimplicit-fallthrough for Clang. > > > > > > > > In preparation to enable -Wimplicit-fallthrough for Clang, explicitly > > > > add multiple break/goto/return/fallthrough statements instead of just > > > > letting the code fall through to the next case. > > > > > > > > Notice that in order to enable -Wimplicit-fallthrough for Clang, this > > > > change[1] is meant to be reverted at some point. So, this patch helps > > > > to move in that direction. > > > > > > > > Something important to mention is that there is currently a discrepancy > > > > between GCC and Clang when dealing with switch fall-through to empty > > > > case > > > > statements or to cases that only contain a break/continue/return > > > > statement[2][3][4]. > > > > > > Are we sure we want to make this change? Was it discussed before? > > > > > > Are there any bugs Clangs puritanical definition of fallthrough helped > > > find? > > > > > > IMVHO compiler warnings are supposed to warn about issues that could > > > be bugs. Falling through to default: break; can hardly be a bug?! > > > > It's certainly a place where the intent is not always clear. I think > > this makes all the cases unambiguous, and doesn't impact the machine > > code, since the compiler will happily optimize away any behavioral > > redundancy. > > If none of the 140 patches here fix a real bug, and there is no change > to machine code then it sounds to me like a W=2 kind of a warning. FWIW, this series has found at least one bug so far: https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=kr...@mail.gmail.com/ -- Kees Cook ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH 000/141] Fix fall-through warnings for Clang
On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote: > On Fri, 20 Nov 2020 11:30:40 -0800 Kees Cook wrote: > > On Fri, Nov 20, 2020 at 10:53:44AM -0800, Jakub Kicinski wrote: > > > On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote: > > > > This series aims to fix almost all remaining fall-through warnings in > > > > order to enable -Wimplicit-fallthrough for Clang. > > > > > > > > In preparation to enable -Wimplicit-fallthrough for Clang, explicitly > > > > add multiple break/goto/return/fallthrough statements instead of just > > > > letting the code fall through to the next case. > > > > > > > > Notice that in order to enable -Wimplicit-fallthrough for Clang, this > > > > change[1] is meant to be reverted at some point. So, this patch helps > > > > to move in that direction. > > > > > > > > Something important to mention is that there is currently a discrepancy > > > > between GCC and Clang when dealing with switch fall-through to empty > > > > case > > > > statements or to cases that only contain a break/continue/return > > > > statement[2][3][4]. > > > > > > Are we sure we want to make this change? Was it discussed before? > > > > > > Are there any bugs Clangs puritanical definition of fallthrough helped > > > find? > > > > > > IMVHO compiler warnings are supposed to warn about issues that could > > > be bugs. Falling through to default: break; can hardly be a bug?! > > > > It's certainly a place where the intent is not always clear. I think > > this makes all the cases unambiguous, and doesn't impact the machine > > code, since the compiler will happily optimize away any behavioral > > redundancy. > > If none of the 140 patches here fix a real bug, and there is no change > to machine code then it sounds to me like a W=2 kind of a warning. I'd like to avoid splitting common -W options between default and W=2 just based on the compiler. Getting -Wimplicit-fallthrough enabled found plenty of bugs, so making sure it works correctly for both compilers feels justified to me. (This is just a subset of the same C language short-coming.) > I think clang is just being annoying here, but if I'm the only one who > feels this way chances are I'm wrong :) It's being pretty pedantic, but I don't think it's unreasonable to explicitly state how every case ends. GCC's silence for the case of "fall through to a break" doesn't really seem justified. -- Kees Cook ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH 000/141] Fix fall-through warnings for Clang
On Fri, Nov 20, 2020 at 10:53:44AM -0800, Jakub Kicinski wrote: > On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote: > > This series aims to fix almost all remaining fall-through warnings in > > order to enable -Wimplicit-fallthrough for Clang. > > > > In preparation to enable -Wimplicit-fallthrough for Clang, explicitly > > add multiple break/goto/return/fallthrough statements instead of just > > letting the code fall through to the next case. > > > > Notice that in order to enable -Wimplicit-fallthrough for Clang, this > > change[1] is meant to be reverted at some point. So, this patch helps > > to move in that direction. > > > > Something important to mention is that there is currently a discrepancy > > between GCC and Clang when dealing with switch fall-through to empty case > > statements or to cases that only contain a break/continue/return > > statement[2][3][4]. > > Are we sure we want to make this change? Was it discussed before? > > Are there any bugs Clangs puritanical definition of fallthrough helped > find? > > IMVHO compiler warnings are supposed to warn about issues that could > be bugs. Falling through to default: break; can hardly be a bug?! It's certainly a place where the intent is not always clear. I think this makes all the cases unambiguous, and doesn't impact the machine code, since the compiler will happily optimize away any behavioral redundancy. -- Kees Cook ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH RFC 00/15] Zero ****s, hugload of hugs <3
On Fri, Nov 30, 2018 at 11:27 AM Jarkko Sakkinen wrote: > > In order to comply with the CoC, replace with a hug. Heh. I support the replacement of the stronger language, but I find "hug", "hugged", and "hugging" to be very weird replacements. Can we bikeshed this to "heck", "hecked", and "hecking" (or "heckin" to follow true Doggo meme style). "This API is hugged" doesn't make any sense to me. "This API is hecked" is better, or at least funnier (to me). "Hug this interface" similarly makes no sense, but "Heck this interface" seems better. "Don't touch my hecking code", "What the heck were they thinking?" etc... "hug" is odd. Better yet, since it's only 17 files, how about doing context-specific changes? "This API is terrible", "Hateful interface", "Don't touch my freakin' code", "What in the world were they thinking?" etc? -Kees > > Jarkko Sakkinen (15): > MIPS: replace with a hug > Documentation: replace with a hug > drm/nouveau: replace with a hug > m68k: replace with a hug > parisc: replace with a hug > cpufreq: replace with a hug > ide: replace with a hug > media: replace with a hug > mtd: replace with a hug > net/sunhme: replace with a hug > scsi: replace with a hug > inotify: replace with a hug > irq: replace with a hug > lib: replace with a hug > net: replace with a hug > > Documentation/kernel-hacking/locking.rst | 2 +- > arch/m68k/include/asm/sun3ints.h | 2 +- > arch/mips/pci/ops-bridge.c| 24 +-- > arch/mips/sgi-ip22/ip22-setup.c | 2 +- > arch/parisc/kernel/sys_parisc.c | 2 +- > drivers/cpufreq/powernow-k7.c | 2 +- > .../gpu/drm/nouveau/nvkm/subdev/bios/init.c | 2 +- > .../nouveau/nvkm/subdev/pmu/fuc/macros.fuc| 2 +- > drivers/ide/cmd640.c | 2 +- > drivers/media/i2c/bt819.c | 8 --- > drivers/mtd/mtd_blkdevs.c | 2 +- > drivers/net/ethernet/sun/sunhme.c | 4 ++-- > drivers/scsi/qlogicpti.h | 2 +- > fs/notify/inotify/inotify_user.c | 2 +- > kernel/irq/timings.c | 2 +- > lib/vsprintf.c| 2 +- > net/core/skbuff.c | 2 +- > 17 files changed, 33 insertions(+), 31 deletions(-) > > -- > 2.19.1 > -- Kees Cook ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH] drm/nouveau/secboot/acr: fix memory leak
On Fri, Sep 7, 2018 at 8:02 PM, John Hubbard wrote:
> On 8/2/18 12:51 PM, Gustavo A. R. Silva wrote:
>> Hi all,
>>
>> Friendly ping! Who can take this?
>>
>> Thanks
>> --
>> Gustavo
>>
>> On 07/24/2018 08:27 AM, Gustavo A. R. Silva wrote:
>>> In case memory resources for *bl_desc* were allocated, release
>>> them before return.
>>>
>>> Addresses-Coverity-ID: 1472021 ("Resource leak")
>>> Fixes: 0d466901552a ("drm/nouveau/secboot/acr: Remove VLA usage")
>>> Signed-off-by: Gustavo A. R. Silva
>>> ---
>>> drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
>>> b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
>>> index d02e183..5c14d6a 100644
>>> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
>>> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
>>> @@ -801,6 +801,7 @@ acr_r352_load(struct nvkm_acr *_acr, struct nvkm_falcon
>>> *falcon,
>>> bl = acr->hsbl_unload_blob;
>>> } else {
>>> nvkm_error(_acr->subdev, "invalid secure boot blob!\n");
>>> +kfree(bl_desc);
>>> return -EINVAL;
>>> }
>>>
>>>
>
> Hi Gustavo,
>
> Seeing as how I've been working on this corner of Nouveau lately (don't ask,
> haha),
> I reviewed and also tested this. It looks good, you can add:
>
> Reviewed-by: John Hubbard
Ben can you take this?
Reviewed-by: Kees Cook
-Kees
--
Kees Cook
Pixel Security
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH] drm/nouveau/secboot/acr: Remove VLA usage
On Fri, Jun 22, 2018 at 2:40 PM, Karol Herbst wrote: > Your patch is fine as it is though, because it doesn't add a new > issue, it just showed us there is a potential one. We should keep that > in mind and see how we want to fix that up. I can imagine that this > might cause some issues in some places, maybe it is totally fine. Okay, thanks! Who can take the patch into their tree? -Kees -- Kees Cook Pixel Security ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH] drm/nouveau/secboot/acr: Remove VLA usage
On Fri, Jun 22, 2018 at 10:50 AM, Karol Herbst wrote:
> On Thu, May 24, 2018 at 7:24 PM, Kees Cook wrote:
>> In the quest to remove all stack VLA usage from the kernel[1], this
>> allocates the working buffers before starting the writing so it won't
>> abort in the middle. This needs an initial walk of the lists to figure
>> out how large the buffer should be.
>>
>> [1]
>> https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com
>>
>> Signed-off-by: Kees Cook
>> ---
>> .../nouveau/nvkm/subdev/secboot/acr_r352.c| 25 ---
>> .../nouveau/nvkm/subdev/secboot/acr_r367.c| 16 +++-
>> 2 files changed, 37 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
>> b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
>> index a721354249ce..d02e183717dc 100644
>> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
>> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
>> @@ -414,6 +414,20 @@ acr_r352_ls_write_wpr(struct acr_r352 *acr, struct
>> list_head *imgs,
>> {
>> struct ls_ucode_img *_img;
>> u32 pos = 0;
>> + u32 max_desc_size = 0;
>> + u8 *gdesc;
>> +
>> + /* Figure out how large we need gdesc to be. */
>> + list_for_each_entry(_img, imgs, node) {
>> + const struct acr_r352_ls_func *ls_func =
>> +
>> acr->func->ls_func[_img->falcon_id];
>> +
>> + max_desc_size = max(max_desc_size, ls_func->bl_desc_size);
>> + }
>> +
>> + gdesc = kmalloc(max_desc_size, GFP_KERNEL);
>> + if (!gdesc)
>> + return -ENOMEM;
>>
>> nvkm_kmap(wpr_blob);
>>
>> @@ -421,7 +435,6 @@ acr_r352_ls_write_wpr(struct acr_r352 *acr, struct
>> list_head *imgs,
>> struct ls_ucode_img_r352 *img = ls_ucode_img_r352(_img);
>> const struct acr_r352_ls_func *ls_func =
>>
>> acr->func->ls_func[_img->falcon_id];
>> - u8 gdesc[ls_func->bl_desc_size];
>>
>
> if there are no guarantees that (ls_func->bl_desc_size & 0x4 == 0),
> then we need to memset a bit more, because 4 bytes at the time are
> actually copied inside nvkm_gpuobj_memcpy_to later in that code, but
> the last 4 bytes are only partly memset to 0.
I think this is unchanged from the original code, yes? The memset() is
always against bl_desc_size; I haven't changed that.
> If ls_func->bl_desc_size is always a multiple of 0x4, then it isn't as
> important, but still better to be fixed. Or maybe
> nvkm_gpuobj_memcpy_to should do that handling and check if the size is
> a multiple of 0x4 and otherwise handle that case?
>
> Same is valid for the changes in the r367 file.
Should I resend with both the allocation and the memset getting
rounded up to the next multiple of 4?
-Kees
--
Kees Cook
Pixel Security
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH] drm/nouveau/secboot/acr: Remove VLA usage
On Thu, May 24, 2018 at 10:24 AM, Kees Cook wrote:
> In the quest to remove all stack VLA usage from the kernel[1], this
> allocates the working buffers before starting the writing so it won't
> abort in the middle. This needs an initial walk of the lists to figure
> out how large the buffer should be.
>
> [1]
> https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com
>
> Signed-off-by: Kees Cook
Friendly ping. Who is best to take this patch?
Thanks!
-Kees
> ---
> .../nouveau/nvkm/subdev/secboot/acr_r352.c| 25 ---
> .../nouveau/nvkm/subdev/secboot/acr_r367.c| 16 +++-
> 2 files changed, 37 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
> b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
> index a721354249ce..d02e183717dc 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
> @@ -414,6 +414,20 @@ acr_r352_ls_write_wpr(struct acr_r352 *acr, struct
> list_head *imgs,
> {
> struct ls_ucode_img *_img;
> u32 pos = 0;
> + u32 max_desc_size = 0;
> + u8 *gdesc;
> +
> + /* Figure out how large we need gdesc to be. */
> + list_for_each_entry(_img, imgs, node) {
> + const struct acr_r352_ls_func *ls_func =
> +
> acr->func->ls_func[_img->falcon_id];
> +
> + max_desc_size = max(max_desc_size, ls_func->bl_desc_size);
> + }
> +
> + gdesc = kmalloc(max_desc_size, GFP_KERNEL);
> + if (!gdesc)
> + return -ENOMEM;
>
> nvkm_kmap(wpr_blob);
>
> @@ -421,7 +435,6 @@ acr_r352_ls_write_wpr(struct acr_r352 *acr, struct
> list_head *imgs,
> struct ls_ucode_img_r352 *img = ls_ucode_img_r352(_img);
> const struct acr_r352_ls_func *ls_func =
>
> acr->func->ls_func[_img->falcon_id];
> - u8 gdesc[ls_func->bl_desc_size];
>
> nvkm_gpuobj_memcpy_to(wpr_blob, pos, >wpr_header,
> sizeof(img->wpr_header));
> @@ -447,6 +460,8 @@ acr_r352_ls_write_wpr(struct acr_r352 *acr, struct
> list_head *imgs,
>
> nvkm_done(wpr_blob);
>
> + kfree(gdesc);
> +
> return 0;
> }
>
> @@ -771,7 +786,11 @@ acr_r352_load(struct nvkm_acr *_acr, struct nvkm_falcon
> *falcon,
> struct fw_bl_desc *hsbl_desc;
> void *bl, *blob_data, *hsbl_code, *hsbl_data;
> u32 code_size;
> - u8 bl_desc[bl_desc_size];
> + u8 *bl_desc;
> +
> + bl_desc = kzalloc(bl_desc_size, GFP_KERNEL);
> + if (!bl_desc)
> + return -ENOMEM;
>
> /* Find the bootloader descriptor for our blob and copy it */
> if (blob == acr->load_blob) {
> @@ -802,7 +821,6 @@ acr_r352_load(struct nvkm_acr *_acr, struct nvkm_falcon
> *falcon,
> code_size, hsbl_desc->start_tag, 0, false);
>
> /* Generate the BL header */
> - memset(bl_desc, 0, bl_desc_size);
> acr->func->generate_hs_bl_desc(load_hdr, bl_desc, offset);
>
> /*
> @@ -811,6 +829,7 @@ acr_r352_load(struct nvkm_acr *_acr, struct nvkm_falcon
> *falcon,
> nvkm_falcon_load_dmem(falcon, bl_desc, hsbl_desc->dmem_load_off,
> bl_desc_size, 0);
>
> + kfree(bl_desc);
> return hsbl_desc->start_tag << 8;
> }
>
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r367.c
> b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r367.c
> index 866877b88797..978ad0790367 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r367.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r367.c
> @@ -265,6 +265,19 @@ acr_r367_ls_write_wpr(struct acr_r352 *acr, struct
> list_head *imgs,
> {
> struct ls_ucode_img *_img;
> u32 pos = 0;
> + u32 max_desc_size = 0;
> + u8 *gdesc;
> +
> + list_for_each_entry(_img, imgs, node) {
> + const struct acr_r352_ls_func *ls_func =
> +
> acr->func->ls_func[_img->falcon_id];
> +
> + max_desc_size = max(max_desc_size, ls_func->bl_desc_size);
> + }
> +
> + gdesc = kmalloc(max_desc_size, GFP_KERNEL);
> + if (!gdesc)
> + return -ENOMEM;
>
> nvkm_kmap(wpr_blob);
>
> @@ -272,7 +285,6 @@ acr_r367_ls_write_wpr(struct acr_r352 *acr, struct
>
[Nouveau] [PATCH] drm/nouveau/secboot/acr: Remove VLA usage
In the quest to remove all stack VLA usage from the kernel[1], this
allocates the working buffers before starting the writing so it won't
abort in the middle. This needs an initial walk of the lists to figure
out how large the buffer should be.
[1]
https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
.../nouveau/nvkm/subdev/secboot/acr_r352.c| 25 ---
.../nouveau/nvkm/subdev/secboot/acr_r367.c| 16 +++-
2 files changed, 37 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
index a721354249ce..d02e183717dc 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
@@ -414,6 +414,20 @@ acr_r352_ls_write_wpr(struct acr_r352 *acr, struct
list_head *imgs,
{
struct ls_ucode_img *_img;
u32 pos = 0;
+ u32 max_desc_size = 0;
+ u8 *gdesc;
+
+ /* Figure out how large we need gdesc to be. */
+ list_for_each_entry(_img, imgs, node) {
+ const struct acr_r352_ls_func *ls_func =
+ acr->func->ls_func[_img->falcon_id];
+
+ max_desc_size = max(max_desc_size, ls_func->bl_desc_size);
+ }
+
+ gdesc = kmalloc(max_desc_size, GFP_KERNEL);
+ if (!gdesc)
+ return -ENOMEM;
nvkm_kmap(wpr_blob);
@@ -421,7 +435,6 @@ acr_r352_ls_write_wpr(struct acr_r352 *acr, struct
list_head *imgs,
struct ls_ucode_img_r352 *img = ls_ucode_img_r352(_img);
const struct acr_r352_ls_func *ls_func =
acr->func->ls_func[_img->falcon_id];
- u8 gdesc[ls_func->bl_desc_size];
nvkm_gpuobj_memcpy_to(wpr_blob, pos, >wpr_header,
sizeof(img->wpr_header));
@@ -447,6 +460,8 @@ acr_r352_ls_write_wpr(struct acr_r352 *acr, struct
list_head *imgs,
nvkm_done(wpr_blob);
+ kfree(gdesc);
+
return 0;
}
@@ -771,7 +786,11 @@ acr_r352_load(struct nvkm_acr *_acr, struct nvkm_falcon
*falcon,
struct fw_bl_desc *hsbl_desc;
void *bl, *blob_data, *hsbl_code, *hsbl_data;
u32 code_size;
- u8 bl_desc[bl_desc_size];
+ u8 *bl_desc;
+
+ bl_desc = kzalloc(bl_desc_size, GFP_KERNEL);
+ if (!bl_desc)
+ return -ENOMEM;
/* Find the bootloader descriptor for our blob and copy it */
if (blob == acr->load_blob) {
@@ -802,7 +821,6 @@ acr_r352_load(struct nvkm_acr *_acr, struct nvkm_falcon
*falcon,
code_size, hsbl_desc->start_tag, 0, false);
/* Generate the BL header */
- memset(bl_desc, 0, bl_desc_size);
acr->func->generate_hs_bl_desc(load_hdr, bl_desc, offset);
/*
@@ -811,6 +829,7 @@ acr_r352_load(struct nvkm_acr *_acr, struct nvkm_falcon
*falcon,
nvkm_falcon_load_dmem(falcon, bl_desc, hsbl_desc->dmem_load_off,
bl_desc_size, 0);
+ kfree(bl_desc);
return hsbl_desc->start_tag << 8;
}
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r367.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r367.c
index 866877b88797..978ad0790367 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r367.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r367.c
@@ -265,6 +265,19 @@ acr_r367_ls_write_wpr(struct acr_r352 *acr, struct
list_head *imgs,
{
struct ls_ucode_img *_img;
u32 pos = 0;
+ u32 max_desc_size = 0;
+ u8 *gdesc;
+
+ list_for_each_entry(_img, imgs, node) {
+ const struct acr_r352_ls_func *ls_func =
+ acr->func->ls_func[_img->falcon_id];
+
+ max_desc_size = max(max_desc_size, ls_func->bl_desc_size);
+ }
+
+ gdesc = kmalloc(max_desc_size, GFP_KERNEL);
+ if (!gdesc)
+ return -ENOMEM;
nvkm_kmap(wpr_blob);
@@ -272,7 +285,6 @@ acr_r367_ls_write_wpr(struct acr_r352 *acr, struct
list_head *imgs,
struct ls_ucode_img_r367 *img = ls_ucode_img_r367(_img);
const struct acr_r352_ls_func *ls_func =
acr->func->ls_func[_img->falcon_id];
- u8 gdesc[ls_func->bl_desc_size];
nvkm_gpuobj_memcpy_to(wpr_blob, pos, >wpr_header,
sizeof(img->wpr_header));
@@ -298,6 +310,8 @@ acr_r367_ls_write_wpr(struct acr_r352 *acr, struct
list_head *imgs,
nvkm_done(wpr_blob);
+ kfree(gdesc);
+
return 0;
}
--
2.17.0
--
Kees Cook
Pixel Security
__
Re: [Nouveau] [PATCH v2] drm/nouveau/secboot: remove VLA usage
On Wed, May 23, 2018 at 5:36 PM, Ben Skeggs <bske...@redhat.com> wrote: > On Thu, May 24, 2018 at 8:48 AM, Kees Cook <keesc...@chromium.org> wrote: >> On Thu, Apr 26, 2018 at 4:25 PM, Kees Cook <keesc...@chromium.org> wrote: >>> On Thu, Mar 15, 2018 at 7:05 PM, Ben Skeggs <skeg...@gmail.com> wrote: >>>> On 14 March 2018 at 21:08, Thierry Reding <thierry.red...@gmail.com> wrote: >>>>> On Tue, Mar 13, 2018 at 11:24:11AM -0500, Gustavo A. R. Silva wrote: >>>>>> In preparation to enabling -Wvla, remove VLA. In this particular >>>>>> case directly use macro NVKM_MSGQUEUE_CMDLINE_SIZE instead of local >>>>>> variable cmdline_size. Also, remove cmdline_size as it is not >>>>>> actually useful anymore. >>>>>> >>>>>> The use of stack Variable Length Arrays needs to be avoided, as they >>>>>> can be a vector for stack exhaustion, which can be both a runtime bug >>>>>> or a security flaw. Also, in general, as code evolves it is easy to >>>>>> lose track of how big a VLA can get. Thus, we can end up having runtime >>>>>> failures that are hard to debug. >>>>>> >>>>>> Also, fixed as part of the directive to remove all VLAs from >>>>>> the kernel: https://lkml.org/lkml/2018/3/7/621 >>>>>> >>>>>> Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com> >>>>>> --- >>>>>> Changes in v2: >>>>>> - Use sizeof(buf) instead of NVKM_MSGQUEUE_CMDLINE_SIZE. This change >>>>>>is based on the feedback provided by David Laight. Thanks David. >>>>>> >>>>>> drivers/gpu/drm/nouveau/nvkm/subdev/secboot/ls_ucode_msgqueue.c | 7 >>>>>> +++ >>>>>> 1 file changed, 3 insertions(+), 4 deletions(-) >>>>> >>>>> Reviewed-by: Thierry Reding <tred...@nvidia.com> >>>> Thanks everyone. I've taken the patch in my tree. >>> >>> Hi! >>> >>> Just checking in on this -- I don't see this patch in linux-next. Is >>> this queued somewhere else? >> >> Hi, it's been another month; I still don't see this in linux-next. >> Daniel, can this go via drm-misc? > It's already queued in drm-next. Ah-ha, great, thanks! Looks like I just got unlucky with linux-next pausing on the 17th and this getting committed on the 18th. :) But, yes, I see it now: https://cgit.freedesktop.org/drm/drm/commit/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/ls_ucode_msgqueue.c?id=7bf5b70befd7817b9e42acbd2291b2042ea1bf81 -Kees -- Kees Cook Pixel Security ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH v2] drm/nouveau/secboot: remove VLA usage
On Thu, Apr 26, 2018 at 4:25 PM, Kees Cook <keesc...@chromium.org> wrote: > On Thu, Mar 15, 2018 at 7:05 PM, Ben Skeggs <skeg...@gmail.com> wrote: >> On 14 March 2018 at 21:08, Thierry Reding <thierry.red...@gmail.com> wrote: >>> On Tue, Mar 13, 2018 at 11:24:11AM -0500, Gustavo A. R. Silva wrote: >>>> In preparation to enabling -Wvla, remove VLA. In this particular >>>> case directly use macro NVKM_MSGQUEUE_CMDLINE_SIZE instead of local >>>> variable cmdline_size. Also, remove cmdline_size as it is not >>>> actually useful anymore. >>>> >>>> The use of stack Variable Length Arrays needs to be avoided, as they >>>> can be a vector for stack exhaustion, which can be both a runtime bug >>>> or a security flaw. Also, in general, as code evolves it is easy to >>>> lose track of how big a VLA can get. Thus, we can end up having runtime >>>> failures that are hard to debug. >>>> >>>> Also, fixed as part of the directive to remove all VLAs from >>>> the kernel: https://lkml.org/lkml/2018/3/7/621 >>>> >>>> Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com> >>>> --- >>>> Changes in v2: >>>> - Use sizeof(buf) instead of NVKM_MSGQUEUE_CMDLINE_SIZE. This change >>>>is based on the feedback provided by David Laight. Thanks David. >>>> >>>> drivers/gpu/drm/nouveau/nvkm/subdev/secboot/ls_ucode_msgqueue.c | 7 >>>> +++ >>>> 1 file changed, 3 insertions(+), 4 deletions(-) >>> >>> Reviewed-by: Thierry Reding <tred...@nvidia.com> >> Thanks everyone. I've taken the patch in my tree. > > Hi! > > Just checking in on this -- I don't see this patch in linux-next. Is > this queued somewhere else? Hi, it's been another month; I still don't see this in linux-next. Daniel, can this go via drm-misc? -Kees -- Kees Cook Pixel Security ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH v2] drm/nouveau/secboot: remove VLA usage
On Thu, Mar 15, 2018 at 7:05 PM, Ben Skeggs <skeg...@gmail.com> wrote: > On 14 March 2018 at 21:08, Thierry Reding <thierry.red...@gmail.com> wrote: >> On Tue, Mar 13, 2018 at 11:24:11AM -0500, Gustavo A. R. Silva wrote: >>> In preparation to enabling -Wvla, remove VLA. In this particular >>> case directly use macro NVKM_MSGQUEUE_CMDLINE_SIZE instead of local >>> variable cmdline_size. Also, remove cmdline_size as it is not >>> actually useful anymore. >>> >>> The use of stack Variable Length Arrays needs to be avoided, as they >>> can be a vector for stack exhaustion, which can be both a runtime bug >>> or a security flaw. Also, in general, as code evolves it is easy to >>> lose track of how big a VLA can get. Thus, we can end up having runtime >>> failures that are hard to debug. >>> >>> Also, fixed as part of the directive to remove all VLAs from >>> the kernel: https://lkml.org/lkml/2018/3/7/621 >>> >>> Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com> >>> --- >>> Changes in v2: >>> - Use sizeof(buf) instead of NVKM_MSGQUEUE_CMDLINE_SIZE. This change >>>is based on the feedback provided by David Laight. Thanks David. >>> >>> drivers/gpu/drm/nouveau/nvkm/subdev/secboot/ls_ucode_msgqueue.c | 7 +++ >>> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> Reviewed-by: Thierry Reding <tred...@nvidia.com> > Thanks everyone. I've taken the patch in my tree. Hi! Just checking in on this -- I don't see this patch in linux-next. Is this queued somewhere else? Thanks! -Kees -- Kees Cook Pixel Security ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
[Nouveau] [PATCH] drm/nouveau: use designated initializers
Prepare to mark sensitive kernel structures for randomization by making
sure they're using designated initializers. These were identified during
allyesconfig builds of x86, arm, and arm64, with most initializer fixes
extracted from grsecurity.
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
drivers/gpu/drm/nouveau/nouveau_ttm.c | 28 ++--
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c
b/drivers/gpu/drm/nouveau/nouveau_ttm.c
index a6dbe8258040..ec4668a41e01 100644
--- a/drivers/gpu/drm/nouveau/nouveau_ttm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c
@@ -107,10 +107,10 @@ nouveau_vram_manager_new(struct ttm_mem_type_manager *man,
}
const struct ttm_mem_type_manager_func nouveau_vram_manager = {
- nouveau_vram_manager_init,
- nouveau_vram_manager_fini,
- nouveau_vram_manager_new,
- nouveau_vram_manager_del,
+ .init = nouveau_vram_manager_init,
+ .takedown = nouveau_vram_manager_fini,
+ .get_node = nouveau_vram_manager_new,
+ .put_node = nouveau_vram_manager_del,
};
static int
@@ -184,11 +184,11 @@ nouveau_gart_manager_debug(struct ttm_mem_type_manager
*man, const char *prefix)
}
const struct ttm_mem_type_manager_func nouveau_gart_manager = {
- nouveau_gart_manager_init,
- nouveau_gart_manager_fini,
- nouveau_gart_manager_new,
- nouveau_gart_manager_del,
- nouveau_gart_manager_debug
+ .init = nouveau_gart_manager_init,
+ .takedown = nouveau_gart_manager_fini,
+ .get_node = nouveau_gart_manager_new,
+ .put_node = nouveau_gart_manager_del,
+ .debug = nouveau_gart_manager_debug
};
/*XXX*/
@@ -257,11 +257,11 @@ nv04_gart_manager_debug(struct ttm_mem_type_manager *man,
const char *prefix)
}
const struct ttm_mem_type_manager_func nv04_gart_manager = {
- nv04_gart_manager_init,
- nv04_gart_manager_fini,
- nv04_gart_manager_new,
- nv04_gart_manager_del,
- nv04_gart_manager_debug
+ .init = nv04_gart_manager_init,
+ .takedown = nv04_gart_manager_fini,
+ .get_node = nv04_gart_manager_new,
+ .put_node = nv04_gart_manager_del,
+ .debug = nv04_gart_manager_debug
};
int
--
2.7.4
--
Kees Cook
Nexus Security
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
