From 215199e3d9f3dc01a6d10b8229891e6f7f1085e7 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Thu, 24 Aug 2023 21:25:55 -0700 Subject: [PATCH 01/73] hardening: Provide Kconfig fragments for basic options Inspired by Salvatore Mesoraca's earlier[1] efforts to provide some in-tree guidance for kernel hardening Kconfig options, add a new fragment named "hardening-basic.config" (along with some arch-specific fragments) that enable a basic set of kernel hardening options that have the least (or no) performance impact and remove a reasonable set of legacy APIs. Using this fragment is as simple as running "make hardening.config". More extreme fragments can be added[2] in the future to cover all the recognized hardening options, and more per-architecture files can be added too. For now, document the fragments directly via comments. Perhaps .rst documentation can be generated from them in the future (rather than the other way around). [1] https://lore.kernel.org/kernel-hardening/1536516257-30871-1-git-send-email-s.mesoraca16@gmail.com/ [2] https://github.com/KSPP/linux/issues/14 Cc: Salvatore Mesoraca Cc: x86@kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-doc@vger.kernel.org Cc: linux-kbuild@vger.kernel.org Signed-off-by: Kees Cook --- MAINTAINERS | 2 + arch/arm/configs/hardening.config | 7 ++ arch/arm64/configs/hardening.config | 22 ++++++ arch/powerpc/configs/hardening.config | 10 +++ arch/x86/configs/hardening.config | 15 ++++ kernel/configs/hardening.config | 98 +++++++++++++++++++++++++++ 6 files changed, 154 insertions(+) create mode 100644 arch/arm/configs/hardening.config create mode 100644 arch/arm64/configs/hardening.config create mode 100644 arch/powerpc/configs/hardening.config create mode 100644 arch/x86/configs/hardening.config create mode 100644 kernel/configs/hardening.config diff --git a/MAINTAINERS b/MAINTAINERS index bf0f54c24f81..737dcc7a2155 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11398,8 +11398,10 @@ S: Supported T: git git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/hardening F: Documentation/ABI/testing/sysfs-kernel-oops_count F: Documentation/ABI/testing/sysfs-kernel-warn_count +F: arch/*/configs/hardening.config F: include/linux/overflow.h F: include/linux/randomize_kstack.h +F: kernel/configs/hardening.config F: mm/usercopy.c K: \b(add|choose)_random_kstack_offset\b K: \b__check_(object_size|heap_object)\b diff --git a/arch/arm/configs/hardening.config b/arch/arm/configs/hardening.config new file mode 100644 index 000000000000..327349ce6377 --- /dev/null +++ b/arch/arm/configs/hardening.config @@ -0,0 +1,7 @@ +# Basic kernel hardening options (specific to arm) + +# Make sure PXN/PAN emulation is enabled. +CONFIG_CPU_SW_DOMAIN_PAN=y + +# Dangerous; old interfaces and needless additional attack surface. +# CONFIG_OABI_COMPAT is not set diff --git a/arch/arm64/configs/hardening.config b/arch/arm64/configs/hardening.config new file mode 100644 index 000000000000..b0e795208998 --- /dev/null +++ b/arch/arm64/configs/hardening.config @@ -0,0 +1,22 @@ +# Basic kernel hardening options (specific to arm64) + +# Make sure PAN emulation is enabled. +CONFIG_ARM64_SW_TTBR0_PAN=y + +# Software Shadow Stack or PAC +CONFIG_SHADOW_CALL_STACK=y + +# Pointer authentication (ARMv8.3 and later). If hardware actually supports +# it, one can turn off CONFIG_STACKPROTECTOR_STRONG with this enabled. +CONFIG_ARM64_PTR_AUTH=y +CONFIG_ARM64_PTR_AUTH_KERNEL=y + +# Available in ARMv8.5 and later. +CONFIG_ARM64_BTI=y +CONFIG_ARM64_BTI_KERNEL=y +CONFIG_ARM64_MTE=y +CONFIG_KASAN_HW_TAGS=y +CONFIG_ARM64_E0PD=y + +# Available in ARMv8.7 and later. +CONFIG_ARM64_EPAN=y diff --git a/arch/powerpc/configs/hardening.config b/arch/powerpc/configs/hardening.config new file mode 100644 index 000000000000..4e9bba327e8f --- /dev/null +++ b/arch/powerpc/configs/hardening.config @@ -0,0 +1,10 @@ +# PowerPC specific hardening options + +# Block kernel from unexpectedly reading userspace memory. +CONFIG_PPC_KUAP=y + +# Attack surface reduction. +# CONFIG_SCOM_DEBUGFS is not set + +# Disable internal kernel debugger. +# CONFIG_XMON is not set diff --git a/arch/x86/configs/hardening.config b/arch/x86/configs/hardening.config new file mode 100644 index 000000000000..19bb0c7a7669 --- /dev/null +++ b/arch/x86/configs/hardening.config @@ -0,0 +1,15 @@ +# Basic kernel hardening options (specific to x86) + +# Modern libc no longer needs a fixed-position mapping in userspace, remove +# it as a possible target. +CONFIG_LEGACY_VSYSCALL_NONE=y + +# Enable chip-specific IOMMU support. +CONFIG_INTEL_IOMMU=y +CONFIG_INTEL_IOMMU_DEFAULT_ON=y +CONFIG_INTEL_IOMMU_SVM=y +CONFIG_AMD_IOMMU=y +CONFIG_AMD_IOMMU_V2=y + +# Enable CET Shadow Stack for userspace. +CONFIG_X86_USER_SHADOW_STACK=y diff --git a/kernel/configs/hardening.config b/kernel/configs/hardening.config new file mode 100644 index 000000000000..95a400f042b1 --- /dev/null +++ b/kernel/configs/hardening.config @@ -0,0 +1,98 @@ +# Help: Basic kernel hardening options +# +# These are considered the basic kernel hardening, self-protection, and +# attack surface reduction options. They are expected to have low (or +# no) performance impact on most workloads, and have a reasonable level +# of legacy API removals. + +# Make sure reporting of various hardening actions is possible. +CONFIG_BUG=y + +# Basic kernel memory permission enforcement. +CONFIG_STRICT_KERNEL_RWX=y +CONFIG_STRICT_MODULE_RWX=y +CONFIG_VMAP_STACK=y + +# Kernel image and memory ASLR. +CONFIG_RANDOMIZE_BASE=y +CONFIG_RANDOMIZE_MEMORY=y + +# Randomize allocator freelists, harden metadata. +CONFIG_SLAB_FREELIST_RANDOM=y +CONFIG_SLAB_FREELIST_HARDENED=y +CONFIG_SHUFFLE_PAGE_ALLOCATOR=y +CONFIG_RANDOM_KMALLOC_CACHES=y + +# Randomize kernel stack offset on syscall entry. +CONFIG_RANDOMIZE_KSTACK_OFFSET_DEFAULT=y + +# Basic stack frame overflow protection. +CONFIG_STACKPROTECTOR=y +CONFIG_STACKPROTECTOR_STRONG=y + +# Basic buffer length bounds checking. +CONFIG_HARDENED_USERCOPY=y +CONFIG_FORTIFY_SOURCE=y + +# Basic array index bounds checking. +CONFIG_UBSAN=y +CONFIG_UBSAN_TRAP=y +CONFIG_UBSAN_BOUNDS=y +# CONFIG_UBSAN_SHIFT is not set +# CONFIG_UBSAN_DIV_ZERO +# CONFIG_UBSAN_UNREACHABLE +# CONFIG_UBSAN_BOOL +# CONFIG_UBSAN_ENUM +# CONFIG_UBSAN_ALIGNMENT +CONFIG_UBSAN_SANITIZE_ALL=y + +# Linked list integrity checking. +CONFIG_LIST_HARDENED=y + +# Initialize all heap variables to zero on allocation. +CONFIG_INIT_ON_ALLOC_DEFAULT_ON=y + +# Initialize all stack variables to zero on function entry. +CONFIG_INIT_STACK_ALL_ZERO=y + +# Wipe RAM at reboot via EFI. For more details, see: +# https://trustedcomputinggroup.org/resource/pc-client-work-group-platform-reset-attack-mitigation-specification/ +# https://bugzilla.redhat.com/show_bug.cgi?id=1532058 +CONFIG_RESET_ATTACK_MITIGATION=y + +# Disable DMA between EFI hand-off and the kernel's IOMMU setup. +CONFIG_EFI_DISABLE_PCI_DMA=y + +# Force IOMMU TLB invalidation so devices will never be able to access stale +# data content. +CONFIG_IOMMU_SUPPORT=y +CONFIG_IOMMU_DEFAULT_DMA_STRICT=y + +# Do not allow direct physical memory access to non-device memory. +CONFIG_STRICT_DEVMEM=y +CONFIG_IO_STRICT_DEVMEM=y + +# Provide userspace with seccomp BPF API for syscall attack surface reduction. +CONFIG_SECCOMP=y +CONFIG_SECCOMP_FILTER=y + +# Provides some protections against SYN flooding. +CONFIG_SYN_COOKIES=y + +# Attack surface reduction: do not autoload TTY line disciplines. +# CONFIG_LDISC_AUTOLOAD is not set + +# Dangerous; enabling this disables userspace brk ASLR. +# CONFIG_COMPAT_BRK is not set + +# Dangerous; exposes kernel text image layout. +# CONFIG_PROC_KCORE is not set + +# Dangerous; enabling this disables userspace VDSO ASLR. +# CONFIG_COMPAT_VDSO is not set + +# Attack surface reduction: Use the modern PTY interface (devpts) only. +# CONFIG_LEGACY_PTYS is not set + +# Attack surface reduction: Use only modesetting video drivers. +# CONFIG_DRM_LEGACY is not set From 8ebab155ea18a6d6c47b17287a0ab4633334f6eb Mon Sep 17 00:00:00 2001 From: Azeem Shaikh Date: Wed, 30 Aug 2023 16:08:06 +0000 Subject: [PATCH 02/73] init/version.c: Replace strlcpy with strscpy strlcpy() reads the entire source buffer first. This read may exceed the destination size limit. This is both inefficient and can lead to linear read overflows if a source string is not NUL-terminated [1]. In an effort to remove strlcpy() completely [2], replace strlcpy() here with strscpy(). Direct replacement is safe here since return value of -errno is used to check for truncation instead of sizeof(dest). [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy [2] https://github.com/KSPP/linux/issues/89 Signed-off-by: Azeem Shaikh Reviewed-by: Justin Stitt Reviewed-by: Kees Cook Link: https://lore.kernel.org/r/20230830160806.3821893-1-azeemshaikh38@gmail.com Signed-off-by: Kees Cook --- init/version.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/init/version.c b/init/version.c index f117921811b4..94c96f6fbfe6 100644 --- a/init/version.c +++ b/init/version.c @@ -21,10 +21,10 @@ static int __init early_hostname(char *arg) { size_t bufsize = sizeof(init_uts_ns.name.nodename); size_t maxlen = bufsize - 1; - size_t arglen; + ssize_t arglen; - arglen = strlcpy(init_uts_ns.name.nodename, arg, bufsize); - if (arglen > maxlen) { + arglen = strscpy(init_uts_ns.name.nodename, arg, bufsize); + if (arglen < 0) { pr_warn("hostname parameter exceeds %zd characters and will be truncated", maxlen); } From 6cd59324c69a8d30956c70a870dcd0cb14977914 Mon Sep 17 00:00:00 2001 From: Azeem Shaikh Date: Thu, 31 Aug 2023 14:01:04 +0000 Subject: [PATCH 03/73] kobject: Replace strlcpy with strscpy strlcpy() reads the entire source buffer first. This read may exceed the destination size limit. This is both inefficient and can lead to linear read overflows if a source string is not NUL-terminated [1]. In an effort to remove strlcpy() completely [2], replace strlcpy() here with strscpy(). Direct replacement is safe here since return value of -errno is used to check for truncation instead of sizeof(dest). [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy [2] https://github.com/KSPP/linux/issues/89 Signed-off-by: Azeem Shaikh Reviewed-by: Kees Cook Link: https://lore.kernel.org/r/20230831140104.207019-1-azeemshaikh38@gmail.com Signed-off-by: Kees Cook --- lib/kobject_uevent.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c index 7c44b7ae4c5c..fb9a2f06dd1e 100644 --- a/lib/kobject_uevent.c +++ b/lib/kobject_uevent.c @@ -254,10 +254,10 @@ static int init_uevent_argv(struct kobj_uevent_env *env, const char *subsystem) int buffer_size = sizeof(env->buf) - env->buflen; int len; - len = strlcpy(&env->buf[env->buflen], subsystem, buffer_size); - if (len >= buffer_size) { - pr_warn("init_uevent_argv: buffer size of %d too small, needed %d\n", - buffer_size, len); + len = strscpy(&env->buf[env->buflen], subsystem, buffer_size); + if (len < 0) { + pr_warn("%s: insufficient buffer space (%u left) for %s\n", + __func__, buffer_size, subsystem); return -ENOMEM; } From 4bc028de97c289e40b20842e361133225454bc48 Mon Sep 17 00:00:00 2001 From: Tudor Ambarus Date: Thu, 14 Sep 2023 13:37:28 +0000 Subject: [PATCH 04/73] docs: pstore-blk.rst: use "about" as a preposition after "care" Reword the sentence to "care about the {oops/panic, pmsg, console} log." Signed-off-by: Tudor Ambarus Link: https://lore.kernel.org/r/20230914133729.1956907-1-tudor.ambarus@linaro.org Signed-off-by: Kees Cook --- Documentation/admin-guide/pstore-blk.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Documentation/admin-guide/pstore-blk.rst b/Documentation/admin-guide/pstore-blk.rst index 2d22ead9520e..1df5d4f93219 100644 --- a/Documentation/admin-guide/pstore-blk.rst +++ b/Documentation/admin-guide/pstore-blk.rst @@ -76,7 +76,7 @@ kmsg_size ~~~~~~~~~ The chunk size in KB for oops/panic front-end. It **MUST** be a multiple of 4. -It's optional if you do not care oops/panic log. +It's optional if you do not care about the oops/panic log. There are multiple chunks for oops/panic front-end depending on the remaining space except other pstore front-ends. @@ -88,7 +88,7 @@ pmsg_size ~~~~~~~~~ The chunk size in KB for pmsg front-end. It **MUST** be a multiple of 4. -It's optional if you do not care pmsg log. +It's optional if you do not care about the pmsg log. Unlike oops/panic front-end, there is only one chunk for pmsg front-end. @@ -100,7 +100,7 @@ console_size ~~~~~~~~~~~~ The chunk size in KB for console front-end. It **MUST** be a multiple of 4. -It's optional if you do not care console log. +It's optional if you do not care about the console log. Similar to pmsg front-end, there is only one chunk for console front-end. @@ -111,7 +111,7 @@ ftrace_size ~~~~~~~~~~~ The chunk size in KB for ftrace front-end. It **MUST** be a multiple of 4. -It's optional if you do not care console log. +It's optional if you do not care about the console log. Similar to oops front-end, there are multiple chunks for ftrace front-end depending on the count of cpu processors. Each chunk size is equal to From 5ee1a430479914e694584f83a4972e373e3b4c6c Mon Sep 17 00:00:00 2001 From: Tudor Ambarus Date: Thu, 14 Sep 2023 13:37:29 +0000 Subject: [PATCH 05/73] docs: pstore-blk.rst: fix typo, s/console/ftrace The author referred to the ftrace log, but mentioned console log instead. Fix the typo. Signed-off-by: Tudor Ambarus Link: https://lore.kernel.org/r/20230914133729.1956907-2-tudor.ambarus@linaro.org Signed-off-by: Kees Cook --- Documentation/admin-guide/pstore-blk.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/admin-guide/pstore-blk.rst b/Documentation/admin-guide/pstore-blk.rst index 1df5d4f93219..1bb2a1c292aa 100644 --- a/Documentation/admin-guide/pstore-blk.rst +++ b/Documentation/admin-guide/pstore-blk.rst @@ -111,7 +111,7 @@ ftrace_size ~~~~~~~~~~~ The chunk size in KB for ftrace front-end. It **MUST** be a multiple of 4. -It's optional if you do not care about the console log. +It's optional if you do not care about the ftrace log. Similar to oops front-end, there are multiple chunks for ftrace front-end depending on the count of cpu processors. Each chunk size is equal to From ff7a65492c4c88010491ca0995a7e072d2c01879 Mon Sep 17 00:00:00 2001 From: Alejandro Colomar Date: Tue, 29 Aug 2023 20:51:22 +0200 Subject: [PATCH 06/73] elf, uapi: Remove struct tag 'dynamic' Such a generic struct tag shouldn't have been exposed in a public header. Since it's undocumented, we can assume it's a historical accident. And since no software (at least on Debian) relies on this tag, we can safely remove it. Here are the results of a Debian Code Search[1]: $ # packages that contain 'include [<"]linux/elf\.h[">]' $ curl -s https://codesearch.debian.net/results/e5e7c74dfcdae609/packages.txt > include $ # packages that contain '\bstruct dynamic\b' $ curl -s https://codesearch.debian.net/results/b23577e099048c6a/packages.txt > struct $ cat struct include | sort | uniq -d chromium hurd linux qemu qt6-webengine qtwebengine-opensource-src $ # chromium: Seems to hold a copy of the UAPI header. No uses of the tag. $ # hurd: Same thing as chromium. $ # linux: :) $ # qemu: Same thing as chromium. $ # qt6-webengine: Same thing as all. $ # qtwebengine-opensource-src: Yet another copy. Link: https://codesearch.debian.net/ [1] Link: https://lore.kernel.org/linux-mm/87wmxdokum.fsf@email.froward.int.ebiederm.org/T/ Cc: "Eric W. Biederman" Cc: Matthew Wilcox Cc: David Howells Cc: Rolf Eike Beer Cc: Arnd Bergmann Cc: Thomas Gleixner Cc: "Paul E. McKenney" Cc: Dave Jones Signed-off-by: Alejandro Colomar Signed-off-by: Kees Cook --- include/uapi/linux/elf.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h index 9b731976ce2f..9417309b7230 100644 --- a/include/uapi/linux/elf.h +++ b/include/uapi/linux/elf.h @@ -140,7 +140,7 @@ typedef __s64 Elf64_Sxword; #define ELF64_ST_BIND(x) ELF_ST_BIND(x) #define ELF64_ST_TYPE(x) ELF_ST_TYPE(x) -typedef struct dynamic { +typedef struct { Elf32_Sword d_tag; union { Elf32_Sword d_val; From eac80dd4bc22bb754a5476fe5064e662c22f51ba Mon Sep 17 00:00:00 2001 From: Mark Rutland Date: Thu, 21 Sep 2023 17:16:34 +0100 Subject: [PATCH 07/73] lkdtm/bugs: add test for panic() with stuck secondary CPUs Upon a panic() the kernel will use either smp_send_stop() or crash_smp_send_stop() to attempt to stop secondary CPUs via an IPI, which may or may not be an NMI. Generally it's preferable that this is an NMI so that CPUs can be stopped in as many situations as possible, but it's not always possible to provide an NMI, and there are cases where CPUs may be unable to handle the NMI regardless. This patch adds a test for panic() where all other CPUs are stuck with interrupts disabled, which can be used to check whether the kernel gracefully handles CPUs failing to respond to a stop, and whether NMIs actually work to stop CPUs. For example, on arm64 *without* an NMI, this results in: | # echo PANIC_STOP_IRQOFF > /sys/kernel/debug/provoke-crash/DIRECT | lkdtm: Performing direct entry PANIC_STOP_IRQOFF | Kernel panic - not syncing: panic stop irqoff test | CPU: 2 PID: 24 Comm: migration/2 Not tainted 6.5.0-rc3-00077-ge6c782389895-dirty #4 | Hardware name: QEMU QEMU Virtual Machine, BIOS 0.0.0 02/06/2015 | Stopper: multi_cpu_stop+0x0/0x1a0 <- stop_machine_cpuslocked+0x158/0x1a4 | Call trace: | dump_backtrace+0x94/0xec | show_stack+0x18/0x24 | dump_stack_lvl+0x74/0xc0 | dump_stack+0x18/0x24 | panic+0x358/0x3e8 | lkdtm_PANIC+0x0/0x18 | multi_cpu_stop+0x9c/0x1a0 | cpu_stopper_thread+0x84/0x118 | smpboot_thread_fn+0x224/0x248 | kthread+0x114/0x118 | ret_from_fork+0x10/0x20 | SMP: stopping secondary CPUs | SMP: failed to stop secondary CPUs 0-3 | Kernel Offset: 0x401cf3490000 from 0xffff80008000000c0 | PHYS_OFFSET: 0x40000000 | CPU features: 0x00000000,68c167a1,cce6773f | Memory Limit: none | ---[ end Kernel panic - not syncing: panic stop irqoff test ]--- Note the "failed to stop secondary CPUs 0-3" message. On arm64 *with* an NMI, this results in: | # echo PANIC_STOP_IRQOFF > /sys/kernel/debug/provoke-crash/DIRECT | lkdtm: Performing direct entry PANIC_STOP_IRQOFF | Kernel panic - not syncing: panic stop irqoff test | CPU: 1 PID: 19 Comm: migration/1 Not tainted 6.5.0-rc3-00077-ge6c782389895-dirty #4 | Hardware name: QEMU QEMU Virtual Machine, BIOS 0.0.0 02/06/2015 | Stopper: multi_cpu_stop+0x0/0x1a0 <- stop_machine_cpuslocked+0x158/0x1a4 | Call trace: | dump_backtrace+0x94/0xec | show_stack+0x18/0x24 | dump_stack_lvl+0x74/0xc0 | dump_stack+0x18/0x24 | panic+0x358/0x3e8 | lkdtm_PANIC+0x0/0x18 | multi_cpu_stop+0x9c/0x1a0 | cpu_stopper_thread+0x84/0x118 | smpboot_thread_fn+0x224/0x248 | kthread+0x114/0x118 | ret_from_fork+0x10/0x20 | SMP: stopping secondary CPUs | Kernel Offset: 0x55a9c0bc0000 from 0xffff800080000000 | PHYS_OFFSET: 0x40000000 | CPU features: 0x00000000,68c167a1,fce6773f | Memory Limit: none | ---[ end Kernel panic - not syncing: panic stop irqoff test ]--- Note the absence of a "failed to stop secondary CPUs" message, since we don't log anything when secondary CPUs are successfully stopped. Signed-off-by: Mark Rutland Cc: Douglas Anderson Cc: Kees Cook Cc: Stephen Boyd Cc: Sumit Garg Reviewed-by: Kees Cook Reviewed-by: Douglas Anderson Reviewed-by: Stephen Boyd Link: https://lore.kernel.org/r/20230921161634.4063233-1-mark.rutland@arm.com Signed-off-by: Kees Cook --- drivers/misc/lkdtm/bugs.c | 30 ++++++++++++++++++++++++- tools/testing/selftests/lkdtm/tests.txt | 1 + 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/drivers/misc/lkdtm/bugs.c b/drivers/misc/lkdtm/bugs.c index c66cc05a68c4..b080eb2335eb 100644 --- a/drivers/misc/lkdtm/bugs.c +++ b/drivers/misc/lkdtm/bugs.c @@ -6,12 +6,14 @@ * test source files. */ #include "lkdtm.h" +#include #include #include #include #include -#include #include +#include +#include #if IS_ENABLED(CONFIG_X86_32) && !IS_ENABLED(CONFIG_UML) #include @@ -73,6 +75,31 @@ static void lkdtm_PANIC(void) panic("dumptest"); } +static int panic_stop_irqoff_fn(void *arg) +{ + atomic_t *v = arg; + + /* + * As stop_machine() disables interrupts, all CPUs within this function + * have interrupts disabled and cannot take a regular IPI. + * + * The last CPU which enters here will trigger a panic, and as all CPUs + * cannot take a regular IPI, we'll only be able to stop secondaries if + * smp_send_stop() or crash_smp_send_stop() uses an NMI. + */ + if (atomic_inc_return(v) == num_online_cpus()) + panic("panic stop irqoff test"); + + for (;;) + cpu_relax(); +} + +static void lkdtm_PANIC_STOP_IRQOFF(void) +{ + atomic_t v = ATOMIC_INIT(0); + stop_machine(panic_stop_irqoff_fn, &v, cpu_online_mask); +} + static void lkdtm_BUG(void) { BUG(); @@ -638,6 +665,7 @@ static noinline void lkdtm_CORRUPT_PAC(void) static struct crashtype crashtypes[] = { CRASHTYPE(PANIC), + CRASHTYPE(PANIC_STOP_IRQOFF), CRASHTYPE(BUG), CRASHTYPE(WARNING), CRASHTYPE(WARNING_MESSAGE), diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt index 607b8d7e3ea3..71a2990b48b1 100644 --- a/tools/testing/selftests/lkdtm/tests.txt +++ b/tools/testing/selftests/lkdtm/tests.txt @@ -1,4 +1,5 @@ #PANIC +#PANIC_STOP_IRQOFF Crashes entire system BUG kernel BUG at WARNING WARNING: WARNING_MESSAGE message trigger From cf77bf698887c3b9ebed76dea492b07a3c2c7632 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ricardo=20Ca=C3=B1uelo?= Date: Wed, 2 Aug 2023 08:32:52 +0200 Subject: [PATCH 08/73] selftests/lkdtm: Disable CONFIG_UBSAN_TRAP in test config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lkdtm selftest config fragment enables CONFIG_UBSAN_TRAP to make the ARRAY_BOUNDS test kill the calling process when an out-of-bound access is detected by UBSAN. However, after this [1] commit, UBSAN is triggered under many new scenarios that weren't detected before, such as in struct definitions with fixed-size trailing arrays used as flexible arrays. As a result, CONFIG_UBSAN_TRAP=y has become a very aggressive option to enable except for specific situations. `make kselftest-merge` applies CONFIG_UBSAN_TRAP=y to the kernel config for all selftests, which makes many of them fail because of system hangs during boot. This change removes the config option from the lkdtm kselftest and configures the ARRAY_BOUNDS test to look for UBSAN reports rather than relying on the calling process being killed. [1] commit 2d47c6956ab3 ("ubsan: Tighten UBSAN_BOUNDS on GCC")' Signed-off-by: Ricardo Cañuelo Reviewed-by: Kees Cook Link: https://lore.kernel.org/r/20230802063252.1917997-1-ricardo.canuelo@collabora.com Signed-off-by: Kees Cook --- tools/testing/selftests/lkdtm/config | 1 - tools/testing/selftests/lkdtm/tests.txt | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/tools/testing/selftests/lkdtm/config b/tools/testing/selftests/lkdtm/config index 5d52f64dfb43..7afe05e8c4d7 100644 --- a/tools/testing/selftests/lkdtm/config +++ b/tools/testing/selftests/lkdtm/config @@ -9,7 +9,6 @@ CONFIG_INIT_ON_FREE_DEFAULT_ON=y CONFIG_INIT_ON_ALLOC_DEFAULT_ON=y CONFIG_UBSAN=y CONFIG_UBSAN_BOUNDS=y -CONFIG_UBSAN_TRAP=y CONFIG_STACKPROTECTOR_STRONG=y CONFIG_SLUB_DEBUG=y CONFIG_SLUB_DEBUG_ON=y diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt index 71a2990b48b1..368973f05250 100644 --- a/tools/testing/selftests/lkdtm/tests.txt +++ b/tools/testing/selftests/lkdtm/tests.txt @@ -8,7 +8,7 @@ EXCEPTION #EXHAUST_STACK Corrupts memory on failure #CORRUPT_STACK Crashes entire system on success #CORRUPT_STACK_STRONG Crashes entire system on success -ARRAY_BOUNDS +ARRAY_BOUNDS call trace:|UBSAN: array-index-out-of-bounds CORRUPT_LIST_ADD list_add corruption CORRUPT_LIST_DEL list_del corruption STACK_GUARD_PAGE_LEADING From 921f15fe8c8cf9543665bdc556e080a5ecbc34b3 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Mon, 25 Sep 2023 10:17:36 -0700 Subject: [PATCH 09/73] MAINTAINERS: hardening: Add __counted_by regex Since __counted_by annotations may also require that code be changed to get initialization ordering correct, let's get an extra group of eyes on code that is working on these annotations. Signed-off-by: Kees Cook --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 737dcc7a2155..741285b8246e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11405,6 +11405,7 @@ F: kernel/configs/hardening.config F: mm/usercopy.c K: \b(add|choose)_random_kstack_offset\b K: \b__check_(object_size|heap_object)\b +K: \b__counted_by\b KERNEL JANITORS L: kernel-janitors@vger.kernel.org From 5e6a1c803f10afec6f7bf349536ce13f60b4a108 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 22 Sep 2023 10:54:17 -0700 Subject: [PATCH 10/73] accel/ivpu: Annotate struct ivpu_job 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 ivpu_job. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Jacek Lawrynowicz Cc: Stanislaw Gruszka Cc: Oded Gabbay Cc: Nathan Chancellor Cc: Nick Desaulniers Cc: Tom Rix Cc: dri-devel@lists.freedesktop.org Cc: llvm@lists.linux.dev Reviewed-by: Stanislaw Gruszka Link: https://lore.kernel.org/r/20230922175416.work.272-kees@kernel.org Signed-off-by: Kees Cook --- drivers/accel/ivpu/ivpu_job.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/accel/ivpu/ivpu_job.h b/drivers/accel/ivpu/ivpu_job.h index aa1f0b9479b0..5514c2d8a609 100644 --- a/drivers/accel/ivpu/ivpu_job.h +++ b/drivers/accel/ivpu/ivpu_job.h @@ -51,7 +51,7 @@ struct ivpu_job { u32 job_id; u32 engine_idx; size_t bo_count; - struct ivpu_bo *bos[]; + struct ivpu_bo *bos[] __counted_by(bo_count); }; int ivpu_submit_ioctl(struct drm_device *dev, void *data, struct drm_file *file); From 4ae7f6320aeb599c10b057abe7fcfcc277532959 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Mon, 25 Sep 2023 10:30:58 -0700 Subject: [PATCH 11/73] MAINTAINERS: hardening: Add Gustavo as Reviewer It's an oversight to not have already listed Gustavo here. Add him as a Reviewer. Cc: Gustavo A. R. Silva Signed-off-by: Kees Cook --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 741285b8246e..5f18ed0fbd42 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11393,6 +11393,7 @@ F: usr/ KERNEL HARDENING (not covered by other areas) M: Kees Cook +R: Gustavo A. R. Silva L: linux-hardening@vger.kernel.org S: Supported T: git git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/hardening From 585a018627b4d7ed37387211f667916840b5c5ea Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Thu, 28 Sep 2023 20:24:29 -0700 Subject: [PATCH 12/73] binfmt_elf: Support segments with 0 filesz and misaligned starts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implement a helper elf_load() that wraps elf_map() and performs all of the necessary work to ensure that when "memsz > filesz" the bytes described by "memsz > filesz" are zeroed. An outstanding issue is if the first segment has filesz 0, and has a randomized location. But that is the same as today. In this change I replaced an open coded padzero() that did not clear all of the way to the end of the page, with padzero() that does. I also stopped checking the return of padzero() as there is at least one known case where testing for failure is the wrong thing to do. It looks like binfmt_elf_fdpic may have the proper set of tests for when error handling can be safely completed. I found a couple of commits in the old history https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git, that look very interesting in understanding this code. commit 39b56d902bf3 ("[PATCH] binfmt_elf: clearing bss may fail") commit c6e2227e4a3e ("[SPARC64]: Missing user access return value checks in fs/binfmt_elf.c and fs/compat.c") commit 5bf3be033f50 ("v2.4.10.1 -> v2.4.10.2") Looking at commit 39b56d902bf3 ("[PATCH] binfmt_elf: clearing bss may fail"): > commit 39b56d902bf35241e7cba6cc30b828ed937175ad > Author: Pavel Machek > Date: Wed Feb 9 22:40:30 2005 -0800 > > [PATCH] binfmt_elf: clearing bss may fail > > So we discover that Borland's Kylix application builder emits weird elf > files which describe a non-writeable bss segment. > > So remove the clear_user() check at the place where we zero out the bss. I > don't _think_ there are any security implications here (plus we've never > checked that clear_user() return value, so whoops if it is a problem). > > Signed-off-by: Pavel Machek > Signed-off-by: Andrew Morton > Signed-off-by: Linus Torvalds It seems pretty clear that binfmt_elf_fdpic with skipping clear_user() for non-writable segments and otherwise calling clear_user(), aka padzero(), and checking it's return code is the right thing to do. I just skipped the error checking as that avoids breaking things. And notably, it looks like Borland's Kylix died in 2005 so it might be safe to just consider read-only segments with memsz > filesz an error. Reported-by: Sebastian Ott Reported-by: Thomas Weißschuh Closes: https://lkml.kernel.org/r/20230914-bss-alloc-v1-1-78de67d2c6dd@weissschuh.net Signed-off-by: "Eric W. Biederman" Link: https://lore.kernel.org/r/87sf71f123.fsf@email.froward.int.ebiederm.org Tested-by: Pedro Falcato Signed-off-by: Sebastian Ott Link: https://lore.kernel.org/r/20230929032435.2391507-1-keescook@chromium.org Signed-off-by: Kees Cook --- fs/binfmt_elf.c | 111 +++++++++++++++++++++--------------------------- 1 file changed, 48 insertions(+), 63 deletions(-) diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index 7b3d2d491407..2a615f476e44 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -110,25 +110,6 @@ static struct linux_binfmt elf_format = { #define BAD_ADDR(x) (unlikely((unsigned long)(x) >= TASK_SIZE)) -static int set_brk(unsigned long start, unsigned long end, int prot) -{ - start = ELF_PAGEALIGN(start); - end = ELF_PAGEALIGN(end); - if (end > start) { - /* - * Map the last of the bss segment. - * If the header is requesting these pages to be - * executable, honour that (ppc32 needs this). - */ - int error = vm_brk_flags(start, end - start, - prot & PROT_EXEC ? VM_EXEC : 0); - if (error) - return error; - } - current->mm->start_brk = current->mm->brk = end; - return 0; -} - /* We need to explicitly zero any fractional pages after the data section (i.e. bss). This would contain the junk from the file that should not @@ -406,6 +387,51 @@ static unsigned long elf_map(struct file *filep, unsigned long addr, return(map_addr); } +static unsigned long elf_load(struct file *filep, unsigned long addr, + const struct elf_phdr *eppnt, int prot, int type, + unsigned long total_size) +{ + unsigned long zero_start, zero_end; + unsigned long map_addr; + + if (eppnt->p_filesz) { + map_addr = elf_map(filep, addr, eppnt, prot, type, total_size); + if (BAD_ADDR(map_addr)) + return map_addr; + if (eppnt->p_memsz > eppnt->p_filesz) { + zero_start = map_addr + ELF_PAGEOFFSET(eppnt->p_vaddr) + + eppnt->p_filesz; + zero_end = map_addr + ELF_PAGEOFFSET(eppnt->p_vaddr) + + eppnt->p_memsz; + + /* Zero the end of the last mapped page */ + padzero(zero_start); + } + } else { + map_addr = zero_start = ELF_PAGESTART(addr); + zero_end = zero_start + ELF_PAGEOFFSET(eppnt->p_vaddr) + + eppnt->p_memsz; + } + if (eppnt->p_memsz > eppnt->p_filesz) { + /* + * Map the last of the segment. + * If the header is requesting these pages to be + * executable, honour that (ppc32 needs this). + */ + int error; + + zero_start = ELF_PAGEALIGN(zero_start); + zero_end = ELF_PAGEALIGN(zero_end); + + error = vm_brk_flags(zero_start, zero_end - zero_start, + prot & PROT_EXEC ? VM_EXEC : 0); + if (error) + map_addr = error; + } + return map_addr; +} + + static unsigned long total_mapping_size(const struct elf_phdr *phdr, int nr) { elf_addr_t min_addr = -1; @@ -829,7 +855,6 @@ static int load_elf_binary(struct linux_binprm *bprm) struct elf_phdr *elf_ppnt, *elf_phdata, *interp_elf_phdata = NULL; struct elf_phdr *elf_property_phdata = NULL; unsigned long elf_bss, elf_brk; - int bss_prot = 0; int retval, i; unsigned long elf_entry; unsigned long e_entry; @@ -1040,33 +1065,6 @@ out_free_interp: if (elf_ppnt->p_type != PT_LOAD) continue; - if (unlikely (elf_brk > elf_bss)) { - unsigned long nbyte; - - /* There was a PT_LOAD segment with p_memsz > p_filesz - before this one. Map anonymous pages, if needed, - and clear the area. */ - retval = set_brk(elf_bss + load_bias, - elf_brk + load_bias, - bss_prot); - if (retval) - goto out_free_dentry; - nbyte = ELF_PAGEOFFSET(elf_bss); - if (nbyte) { - nbyte = ELF_MIN_ALIGN - nbyte; - if (nbyte > elf_brk - elf_bss) - nbyte = elf_brk - elf_bss; - if (clear_user((void __user *)elf_bss + - load_bias, nbyte)) { - /* - * This bss-zeroing can fail if the ELF - * file specifies odd protections. So - * we don't check the return value - */ - } - } - } - elf_prot = make_prot(elf_ppnt->p_flags, &arch_state, !!interpreter, false); @@ -1162,7 +1160,7 @@ out_free_interp: } } - error = elf_map(bprm->file, load_bias + vaddr, elf_ppnt, + error = elf_load(bprm->file, load_bias + vaddr, elf_ppnt, elf_prot, elf_flags, total_size); if (BAD_ADDR(error)) { retval = IS_ERR_VALUE(error) ? @@ -1217,10 +1215,8 @@ out_free_interp: if (end_data < k) end_data = k; k = elf_ppnt->p_vaddr + elf_ppnt->p_memsz; - if (k > elf_brk) { - bss_prot = elf_prot; + if (k > elf_brk) elf_brk = k; - } } e_entry = elf_ex->e_entry + load_bias; @@ -1232,18 +1228,7 @@ out_free_interp: start_data += load_bias; end_data += load_bias; - /* Calling set_brk effectively mmaps the pages that we need - * for the bss and break sections. We must do this before - * mapping in the interpreter, to make sure it doesn't wind - * up getting placed where the bss needs to go. - */ - retval = set_brk(elf_bss, elf_brk, bss_prot); - if (retval) - goto out_free_dentry; - if (likely(elf_bss != elf_brk) && unlikely(padzero(elf_bss))) { - retval = -EFAULT; /* Nobody gets to see this, but.. */ - goto out_free_dentry; - } + current->mm->start_brk = current->mm->brk = ELF_PAGEALIGN(elf_brk); if (interpreter) { elf_entry = load_elf_interp(interp_elf_ex, From 8ed2ef21ff564cf4a25c098ace510ee6513c9836 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Thu, 28 Sep 2023 20:24:30 -0700 Subject: [PATCH 13/73] binfmt_elf: elf_bss no longer used by load_elf_binary() With the BSS handled generically via the new filesz/memsz mismatch handling logic in elf_load(), elf_bss no longer needs to be tracked. Drop the variable. Cc: Eric Biederman Cc: Alexander Viro Cc: Christian Brauner Cc: linux-fsdevel@vger.kernel.org Cc: linux-mm@kvack.org Suggested-by: Eric Biederman Tested-by: Pedro Falcato Signed-off-by: Sebastian Ott Link: https://lore.kernel.org/r/20230929032435.2391507-2-keescook@chromium.org Signed-off-by: Kees Cook --- fs/binfmt_elf.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index 2a615f476e44..0214d5a949fc 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -854,7 +854,7 @@ static int load_elf_binary(struct linux_binprm *bprm) unsigned long error; struct elf_phdr *elf_ppnt, *elf_phdata, *interp_elf_phdata = NULL; struct elf_phdr *elf_property_phdata = NULL; - unsigned long elf_bss, elf_brk; + unsigned long elf_brk; int retval, i; unsigned long elf_entry; unsigned long e_entry; @@ -1045,7 +1045,6 @@ out_free_interp: if (retval < 0) goto out_free_dentry; - elf_bss = 0; elf_brk = 0; start_code = ~0UL; @@ -1208,8 +1207,6 @@ out_free_interp: k = elf_ppnt->p_vaddr + elf_ppnt->p_filesz; - if (k > elf_bss) - elf_bss = k; if ((elf_ppnt->p_flags & PF_X) && end_code < k) end_code = k; if (end_data < k) @@ -1221,7 +1218,6 @@ out_free_interp: e_entry = elf_ex->e_entry + load_bias; phdr_addr += load_bias; - elf_bss += load_bias; elf_brk += load_bias; start_code += load_bias; end_code += load_bias; From 8b04d32678e3c46b8a738178e0e55918eaa3be17 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Thu, 28 Sep 2023 20:24:31 -0700 Subject: [PATCH 14/73] binfmt_elf: Use elf_load() for interpreter Handle arbitrary memsz>filesz in interpreter ELF segments, instead of only supporting it in the last segment (which is expected to be the BSS). Cc: Eric Biederman Cc: Alexander Viro Cc: Christian Brauner Cc: linux-fsdevel@vger.kernel.org Cc: linux-mm@kvack.org Reported-by: Pedro Falcato Closes: https://lore.kernel.org/lkml/20221106021657.1145519-1-pedro.falcato@gmail.com/ Tested-by: Pedro Falcato Signed-off-by: Sebastian Ott Link: https://lore.kernel.org/r/20230929032435.2391507-3-keescook@chromium.org Signed-off-by: Kees Cook --- fs/binfmt_elf.c | 46 +--------------------------------------------- 1 file changed, 1 insertion(+), 45 deletions(-) diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index 0214d5a949fc..db47cb802f89 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -622,8 +622,6 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex, struct elf_phdr *eppnt; unsigned long load_addr = 0; int load_addr_set = 0; - unsigned long last_bss = 0, elf_bss = 0; - int bss_prot = 0; unsigned long error = ~0UL; unsigned long total_size; int i; @@ -660,7 +658,7 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex, else if (no_base && interp_elf_ex->e_type == ET_DYN) load_addr = -vaddr; - map_addr = elf_map(interpreter, load_addr + vaddr, + map_addr = elf_load(interpreter, load_addr + vaddr, eppnt, elf_prot, elf_type, total_size); total_size = 0; error = map_addr; @@ -686,51 +684,9 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex, error = -ENOMEM; goto out; } - - /* - * Find the end of the file mapping for this phdr, and - * keep track of the largest address we see for this. - */ - k = load_addr + eppnt->p_vaddr + eppnt->p_filesz; - if (k > elf_bss) - elf_bss = k; - - /* - * Do the same thing for the memory mapping - between - * elf_bss and last_bss is the bss section. - */ - k = load_addr + eppnt->p_vaddr + eppnt->p_memsz; - if (k > last_bss) { - last_bss = k; - bss_prot = elf_prot; - } } } - /* - * Now fill out the bss section: first pad the last page from - * the file up to the page boundary, and zero it from elf_bss - * up to the end of the page. - */ - if (padzero(elf_bss)) { - error = -EFAULT; - goto out; - } - /* - * Next, align both the file and mem bss up to the page size, - * since this is where elf_bss was just zeroed up to, and where - * last_bss will end after the vm_brk_flags() below. - */ - elf_bss = ELF_PAGEALIGN(elf_bss); - last_bss = ELF_PAGEALIGN(last_bss); - /* Finally, if there is still more bss to allocate, do it. */ - if (last_bss > elf_bss) { - error = vm_brk_flags(elf_bss, last_bss - elf_bss, - bss_prot & PROT_EXEC ? VM_EXEC : 0); - if (error) - goto out; - } - error = load_addr; out: return error; From 6ad33b53c9b8a1c99bcd2fb96123d5d45bc88d7b Mon Sep 17 00:00:00 2001 From: "Gustavo A. R. Silva" Date: Wed, 16 Aug 2023 12:04:06 -0600 Subject: [PATCH 15/73] nouveau/svm: Replace one-element array with flexible-array member in struct nouveau_svm 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 Link: https://lore.kernel.org/r/087a1c335228bd245192bbb2fb347c9af1be5750.1692208802.git.gustavoars@kernel.org Signed-off-by: Kees Cook --- drivers/gpu/drm/nouveau/nouveau_svm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c index 186351ecf72f..00444ad82d18 100644 --- a/drivers/gpu/drm/nouveau/nouveau_svm.c +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c @@ -67,7 +67,7 @@ struct nouveau_svm { struct nouveau_svmm *svmm; } **fault; int fault_nr; - } buffer[1]; + } buffer[]; }; #define FAULT_ACCESS_READ 0 @@ -1063,7 +1063,7 @@ nouveau_svm_init(struct nouveau_drm *drm) if (drm->client.device.info.family > NV_DEVICE_INFO_V0_PASCAL) return; - if (!(drm->svm = svm = kzalloc(sizeof(*drm->svm), GFP_KERNEL))) + if (!(drm->svm = svm = kzalloc(struct_size(drm->svm, buffer, 1), GFP_KERNEL))) return; drm->svm->drm = drm; From 4cb2e89fea5fe4238c554fcb62afed5231e1d020 Mon Sep 17 00:00:00 2001 From: "Gustavo A. R. Silva" Date: Wed, 16 Aug 2023 12:05:06 -0600 Subject: [PATCH 16/73] nouveau/svm: Split assignment from if conditional Fix checkpatch.pl ERROR: do not use assignment in if condition. Signed-off-by: "Gustavo A. R. Silva" Reviewed-by: Kees Cook Link: https://lore.kernel.org/r/6b900e80b5587187c68efc788f5b042ca747d374.1692208802.git.gustavoars@kernel.org Signed-off-by: Kees Cook --- drivers/gpu/drm/nouveau/nouveau_svm.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c index 00444ad82d18..cc03e0c22ff3 100644 --- a/drivers/gpu/drm/nouveau/nouveau_svm.c +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c @@ -1063,7 +1063,8 @@ nouveau_svm_init(struct nouveau_drm *drm) if (drm->client.device.info.family > NV_DEVICE_INFO_V0_PASCAL) return; - if (!(drm->svm = svm = kzalloc(struct_size(drm->svm, buffer, 1), GFP_KERNEL))) + drm->svm = svm = kzalloc(struct_size(drm->svm, buffer, 1), GFP_KERNEL); + if (!drm->svm) return; drm->svm->drm = drm; From d77008421afda6208b1256c9b218457acd174ca6 Mon Sep 17 00:00:00 2001 From: Elena Reshetova Date: Thu, 17 Aug 2023 21:14:57 -0700 Subject: [PATCH 17/73] groups: Convert group_info.usage to refcount_t atomic_t variables are currently used to implement reference counters with the following properties: - counter is initialized to 1 using atomic_set() - a resource is freed upon counter reaching zero - once counter reaches zero, its further increments aren't allowed - counter schema uses basic atomic operations (set, inc, inc_not_zero, dec_and_test, etc.) Such atomic variables should be converted to a newly provided refcount_t type and API that prevents accidental counter overflows and underflows. This is important since overflows and underflows can lead to use-after-free situation and be exploitable. The variable group_info.usage is used as pure reference counter. Convert it to refcount_t and fix up the operations. **Important note for maintainers: Some functions from refcount_t API defined in refcount.h have different memory ordering guarantees than their atomic counterparts. Please check Documentation/core-api/refcount-vs-atomic.rst for more information. Normally the differences should not matter since refcount_t provides enough guarantees to satisfy the refcounting use cases, but in some rare cases it might matter. Please double check that you don't have some undocumented memory guarantees for this variable usage. For the group_info.usage it might make a difference in following places: - put_group_info(): decrement in refcount_dec_and_test() only provides RELEASE ordering and ACQUIRE ordering on success vs. fully ordered atomic counterpart Suggested-by: Kees Cook Signed-off-by: Elena Reshetova Reviewed-by: David Windsor Reviewed-by: Hans Liljestrand Link: https://lore.kernel.org/r/20230818041456.gonna.009-kees@kernel.org Signed-off-by: Kees Cook --- include/linux/cred.h | 7 ++++--- kernel/cred.c | 2 +- kernel/groups.c | 2 +- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/include/linux/cred.h b/include/linux/cred.h index f923528d5cc4..92f8d772da6f 100644 --- a/include/linux/cred.h +++ b/include/linux/cred.h @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -23,7 +24,7 @@ struct inode; * COW Supplementary groups list */ struct group_info { - atomic_t usage; + refcount_t usage; int ngroups; kgid_t gid[]; } __randomize_layout; @@ -39,7 +40,7 @@ struct group_info { */ static inline struct group_info *get_group_info(struct group_info *gi) { - atomic_inc(&gi->usage); + refcount_inc(&gi->usage); return gi; } @@ -49,7 +50,7 @@ static inline struct group_info *get_group_info(struct group_info *gi) */ #define put_group_info(group_info) \ do { \ - if (atomic_dec_and_test(&(group_info)->usage)) \ + if (refcount_dec_and_test(&(group_info)->usage)) \ groups_free(group_info); \ } while (0) diff --git a/kernel/cred.c b/kernel/cred.c index 98cb4eca23fb..4dc0b27b5462 100644 --- a/kernel/cred.c +++ b/kernel/cred.c @@ -36,7 +36,7 @@ do { \ static struct kmem_cache *cred_jar; /* init to 2 - one for init_task, one to ensure it is never freed */ -static struct group_info init_groups = { .usage = ATOMIC_INIT(2) }; +static struct group_info init_groups = { .usage = REFCOUNT_INIT(2) }; /* * The initial credentials for the initial task diff --git a/kernel/groups.c b/kernel/groups.c index 9aaed2a31073..9b43da22647d 100644 --- a/kernel/groups.c +++ b/kernel/groups.c @@ -19,7 +19,7 @@ struct group_info *groups_alloc(int gidsetsize) if (!gi) return NULL; - atomic_set(&gi->usage, 1); + refcount_set(&gi->usage, 1); gi->ngroups = gidsetsize; return gi; } From e0bbf92682ad1df36ef43104a036469ac0ab3a4a Mon Sep 17 00:00:00 2001 From: Justin Stitt Date: Mon, 11 Sep 2023 17:52:44 +0000 Subject: [PATCH 18/73] um,ethertap: Replace deprecated strncpy() with strscpy() `strncpy` is deprecated for use on NUL-terminated destination strings [1]. `gate_buf` should always be NUL-terminated and does not require NUL-padding. It is used as a string arg inside an argv array given to `run_helper()`. Due to this, let's use `strscpy` as it guarantees NUL-terminated on the destination buffer preventing potential buffer overreads [2]. This exact invocation was changed from `strcpy` to `strncpy` in commit 7879b1d94badb ("um,ethertap: use strncpy") back in 2015. Let's continue hardening our `str*cpy` apis and use the newer and safer `strscpy`! Link: 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-hardening@vger.kernel.org Cc: Kees Cook Signed-off-by: Justin Stitt Reviewed-by: Kees Cook Link: https://lore.kernel.org/r/20230911-strncpy-arch-um-os-linux-drivers-ethertap_user-c-v1-1-d9e53f52ab32@google.com Signed-off-by: Kees Cook --- arch/um/os-Linux/drivers/ethertap_user.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/um/os-Linux/drivers/ethertap_user.c b/arch/um/os-Linux/drivers/ethertap_user.c index 9483021d86dd..3363851a4ae8 100644 --- a/arch/um/os-Linux/drivers/ethertap_user.c +++ b/arch/um/os-Linux/drivers/ethertap_user.c @@ -105,7 +105,7 @@ static int etap_tramp(char *dev, char *gate, int control_me, sprintf(data_fd_buf, "%d", data_remote); sprintf(version_buf, "%d", UML_NET_VERSION); if (gate != NULL) { - strncpy(gate_buf, gate, 15); + strscpy(gate_buf, gate, sizeof(gate_buf)); args = setup_args; } else args = nosetup_args; From a952abcdaa22116d940ca9cb9253caad1622ae93 Mon Sep 17 00:00:00 2001 From: Justin Stitt Date: Mon, 11 Sep 2023 20:51:04 +0000 Subject: [PATCH 19/73] auxdisplay: panel: Replace deprecated strncpy() with strtomem_pad() `strncpy` is deprecated and as such we should prefer more robust and less ambiguous interfaces. In this case, all of `press_str`, `repeat_str` and `release_str` are explicitly marked as nonstring: | struct { /* valid when type == INPUT_TYPE_KBD */ | char press_str[sizeof(void *) + sizeof(int)] __nonstring; | char repeat_str[sizeof(void *) + sizeof(int)] __nonstring; | char release_str[sizeof(void *) + sizeof(int)] __nonstring; | } kbd; ... which makes `strtomem_pad` a suitable replacement as it is functionally the same whilst being more obvious about its behavior. 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-hardening@vger.kernel.org Cc: Kees Cook Signed-off-by: Justin Stitt Reviewed-by: Kees Cook Link: https://lore.kernel.org/r/20230911-strncpy-drivers-auxdisplay-panel-c-v1-1-b60bd0ae8552@google.com Signed-off-by: Kees Cook --- drivers/auxdisplay/panel.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/auxdisplay/panel.c b/drivers/auxdisplay/panel.c index eba04c0de7eb..e20d35bdf5fe 100644 --- a/drivers/auxdisplay/panel.c +++ b/drivers/auxdisplay/panel.c @@ -1449,10 +1449,9 @@ static struct logical_input *panel_bind_key(const char *name, const char *press, key->rise_time = 1; key->fall_time = 1; - strncpy(key->u.kbd.press_str, press, sizeof(key->u.kbd.press_str)); - strncpy(key->u.kbd.repeat_str, repeat, sizeof(key->u.kbd.repeat_str)); - strncpy(key->u.kbd.release_str, release, - sizeof(key->u.kbd.release_str)); + strtomem_pad(key->u.kbd.press_str, press, '\0'); + strtomem_pad(key->u.kbd.repeat_str, repeat, '\0'); + strtomem_pad(key->u.kbd.release_str, release, '\0'); list_add(&key->list, &logical_inputs); return key; } From de055e6116742291a31a21a10108d426574988ff Mon Sep 17 00:00:00 2001 From: Justin Stitt Date: Tue, 12 Sep 2023 22:52:04 +0000 Subject: [PATCH 20/73] bus: fsl-mc: Replace deprecated strncpy() with strscpy_pad() `strncpy` is deprecated for use on NUL-terminated destination strings [1]. We need to prefer more robust and less ambiguous string interfaces. `obj_desc->(type|label)` are expected to be NUL-terminated strings as per "include/linux/fsl/mc.h +143" | ... | * struct fsl_mc_obj_desc - Object descriptor | * @type: Type of object: NULL terminated string | ... It seems `cmd_params->obj_type` is also expected to be a NUL-terminated string. A suitable replacement is `strscpy_pad` due to the fact that it guarantees NUL-termination on the destination buffer whilst keeping the NUL-padding behavior that `strncpy` provides. Padding may not strictly be necessary but let's opt to keep it as this ensures no functional change. 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-hardening@vger.kernel.org Cc: Kees Cook Signed-off-by: Justin Stitt Reviewed-by: Kees Cook Link: https://lore.kernel.org/r/20230912-strncpy-drivers-bus-fsl-mc-dprc-c-v1-1-cdb56aa3f4f4@google.com Signed-off-by: Kees Cook --- drivers/bus/fsl-mc/dprc.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/drivers/bus/fsl-mc/dprc.c b/drivers/bus/fsl-mc/dprc.c index d129338b8bc0..dd1b5c0fb7e2 100644 --- a/drivers/bus/fsl-mc/dprc.c +++ b/drivers/bus/fsl-mc/dprc.c @@ -450,10 +450,8 @@ int dprc_get_obj(struct fsl_mc_io *mc_io, obj_desc->ver_major = le16_to_cpu(rsp_params->version_major); obj_desc->ver_minor = le16_to_cpu(rsp_params->version_minor); obj_desc->flags = le16_to_cpu(rsp_params->flags); - strncpy(obj_desc->type, rsp_params->type, 16); - obj_desc->type[15] = '\0'; - strncpy(obj_desc->label, rsp_params->label, 16); - obj_desc->label[15] = '\0'; + strscpy_pad(obj_desc->type, rsp_params->type, 16); + strscpy_pad(obj_desc->label, rsp_params->label, 16); return 0; } EXPORT_SYMBOL_GPL(dprc_get_obj); @@ -491,8 +489,7 @@ int dprc_set_obj_irq(struct fsl_mc_io *mc_io, cmd_params->irq_addr = cpu_to_le64(irq_cfg->paddr); cmd_params->irq_num = cpu_to_le32(irq_cfg->irq_num); cmd_params->obj_id = cpu_to_le32(obj_id); - strncpy(cmd_params->obj_type, obj_type, 16); - cmd_params->obj_type[15] = '\0'; + strscpy_pad(cmd_params->obj_type, obj_type, 16); /* send command to mc*/ return mc_send_command(mc_io, &cmd); @@ -564,8 +561,7 @@ int dprc_get_obj_region(struct fsl_mc_io *mc_io, cmd_params = (struct dprc_cmd_get_obj_region *)cmd.params; cmd_params->obj_id = cpu_to_le32(obj_id); cmd_params->region_index = region_index; - strncpy(cmd_params->obj_type, obj_type, 16); - cmd_params->obj_type[15] = '\0'; + strscpy_pad(cmd_params->obj_type, obj_type, 16); /* send command to mc*/ err = mc_send_command(mc_io, &cmd); From 0faf84caee63a5f331bda130265fdceb7d4101b5 Mon Sep 17 00:00:00 2001 From: Justin Stitt Date: Wed, 13 Sep 2023 00:07:21 +0000 Subject: [PATCH 21/73] cpufreq: Replace deprecated strncpy() with strscpy() `strncpy` is deprecated for use on NUL-terminated destination strings [1]. We should prefer more robust and less ambiguous string interfaces. Both `policy->last_governor` and `default_governor` are expected to be NUL-terminated which is shown by their heavy usage with other string apis like `strcmp`. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer. 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-hardening@vger.kernel.org Signed-off-by: Justin Stitt Reviewed-by: Kees Cook Acked-by: Viresh Kumar Link: https://lore.kernel.org/r/20230913-strncpy-drivers-cpufreq-cpufreq-c-v1-1-f1608bfeff63@google.com Signed-off-by: Kees Cook --- drivers/cpufreq/cpufreq.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 60ed89000e82..15c440e5c773 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1650,7 +1650,7 @@ static void __cpufreq_offline(unsigned int cpu, struct cpufreq_policy *policy) } if (has_target()) - strncpy(policy->last_governor, policy->governor->name, + strscpy(policy->last_governor, policy->governor->name, CPUFREQ_NAME_LEN); else policy->last_policy = policy->policy; @@ -2996,7 +2996,7 @@ static int __init cpufreq_core_init(void) BUG_ON(!cpufreq_global_kobject); if (!strlen(default_governor)) - strncpy(default_governor, gov->name, CPUFREQ_NAME_LEN); + strscpy(default_governor, gov->name, CPUFREQ_NAME_LEN); return 0; } From b545465e22f5fec2862132c01a5d2abd3c4c4d50 Mon Sep 17 00:00:00 2001 From: Justin Stitt Date: Wed, 13 Sep 2023 00:23:19 +0000 Subject: [PATCH 22/73] cpuidle: dt: Replace deprecated strncpy() with strscpy() `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. With this, we can also drop the now unnecessary `CPUIDLE_(NAME|DESC)_LEN - 1` pieces. 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-hardening@vger.kernel.org Signed-off-by: Justin Stitt Reviewed-by: Kees Cook Link: https://lore.kernel.org/r/20230913-strncpy-drivers-cpuidle-dt_idle_states-c-v1-1-d16a0dbe5658@google.com Signed-off-by: Kees Cook --- drivers/cpuidle/dt_idle_states.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/cpuidle/dt_idle_states.c b/drivers/cpuidle/dt_idle_states.c index 12fec92a85fd..97feb7d8fb23 100644 --- a/drivers/cpuidle/dt_idle_states.c +++ b/drivers/cpuidle/dt_idle_states.c @@ -84,8 +84,8 @@ static int init_state_node(struct cpuidle_state *idle_state, * replace with kstrdup and pointer assignment when name * and desc become string pointers */ - strncpy(idle_state->name, state_node->name, CPUIDLE_NAME_LEN - 1); - strncpy(idle_state->desc, desc, CPUIDLE_DESC_LEN - 1); + strscpy(idle_state->name, state_node->name, CPUIDLE_NAME_LEN); + strscpy(idle_state->desc, desc, CPUIDLE_DESC_LEN); return 0; } From 9b9056a3137b2e00273f889bfdf498ef6570e332 Mon Sep 17 00:00:00 2001 From: Justin Stitt Date: Wed, 13 Sep 2023 19:38:44 +0000 Subject: [PATCH 23/73] firmware: tegra: bpmp: Replace deprecated strncpy() with strscpy_pad() `strncpy` is deprecated for use on NUL-terminated destination strings [1]. We should prefer more robust and less ambiguous string interfaces. It seems like the filename stored at `namevirt` is expected to be NUL-terminated. A suitable replacement is `strscpy_pad` due to the fact that it guarantees NUL-termination on the destination buffer whilst maintaining the NUL-padding behavior that strncpy provides. 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-hardening@vger.kernel.org Signed-off-by: Justin Stitt Reviewed-by: Kees Cook Link: https://lore.kernel.org/r/20230913-strncpy-drivers-firmware-tegra-bpmp-debugfs-c-v1-1-828b0a8914b5@google.com Signed-off-by: Kees Cook --- drivers/firmware/tegra/bpmp-debugfs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/firmware/tegra/bpmp-debugfs.c b/drivers/firmware/tegra/bpmp-debugfs.c index 6dfe3d34109e..bbcdd9fed3fb 100644 --- a/drivers/firmware/tegra/bpmp-debugfs.c +++ b/drivers/firmware/tegra/bpmp-debugfs.c @@ -610,7 +610,7 @@ static int debugfs_show(struct seq_file *m, void *p) } len = strlen(filename); - strncpy(namevirt, filename, namesize); + strscpy_pad(namevirt, filename, namesize); err = mrq_debugfs_read(bpmp, namephys, len, dataphys, datasize, &nbytes); @@ -661,7 +661,7 @@ static ssize_t debugfs_store(struct file *file, const char __user *buf, } len = strlen(filename); - strncpy(namevirt, filename, namesize); + strscpy_pad(namevirt, filename, namesize); if (copy_from_user(datavirt, buf, count)) { err = -EFAULT; From abe6db6c43fa59c4755f210e92d6fbe97a0ad1aa Mon Sep 17 00:00:00 2001 From: Justin Stitt Date: Thu, 14 Sep 2023 22:20:55 +0000 Subject: [PATCH 24/73] HID: prodikeys: Replace deprecated strncpy() with strscpy() `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-hardening@vger.kernel.org Signed-off-by: Justin Stitt Reviewed-by: Kees Cook Link: https://lore.kernel.org/r/20230914-strncpy-drivers-hid-hid-prodikeys-c-v1-1-10c00550f2c2@google.com Signed-off-by: Kees Cook --- drivers/hid/hid-prodikeys.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/hid/hid-prodikeys.c b/drivers/hid/hid-prodikeys.c index e4e9471d0f1e..c16d2ba6ea16 100644 --- a/drivers/hid/hid-prodikeys.c +++ b/drivers/hid/hid-prodikeys.c @@ -639,9 +639,9 @@ static int pcmidi_snd_initialise(struct pcmidi_snd *pm) goto fail; } - strncpy(card->driver, shortname, sizeof(card->driver)); - strncpy(card->shortname, shortname, sizeof(card->shortname)); - strncpy(card->longname, longname, sizeof(card->longname)); + strscpy(card->driver, shortname, sizeof(card->driver)); + strscpy(card->shortname, shortname, sizeof(card->shortname)); + strscpy(card->longname, longname, sizeof(card->longname)); /* Set up rawmidi */ err = snd_rawmidi_new(card, card->shortname, 0, @@ -652,7 +652,7 @@ static int pcmidi_snd_initialise(struct pcmidi_snd *pm) goto fail; } pm->rwmidi = rwmidi; - strncpy(rwmidi->name, card->shortname, sizeof(rwmidi->name)); + strscpy(rwmidi->name, card->shortname, sizeof(rwmidi->name)); rwmidi->info_flags = SNDRV_RAWMIDI_INFO_INPUT; rwmidi->private_data = pm; From 66f8a4a0cc69ea1ddbebb6afcd8bd47511e9929c Mon Sep 17 00:00:00 2001 From: Justin Stitt Date: Tue, 19 Sep 2023 05:22:51 +0000 Subject: [PATCH 25/73] hwmon: (ibmpowernv) Replace deprecated strncpy() with memcpy() `strncpy` is deprecated for use on NUL-terminated destination strings [1]. A suitable replacement is `memcpy` as we've already precisely calculated the number of bytes to copy while `buf` has been explicitly zero-initialized: | char buf[8] = { 0 }; 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-hardening@vger.kernel.org Signed-off-by: Justin Stitt Tested-by: Michael Ellerman Acked-by: Michael Ellerman Link: https://lore.kernel.org/r/20230919-strncpy-drivers-hwmon-ibmpowernv-c-v2-1-37d3e64172bc@google.com Signed-off-by: Kees Cook --- drivers/hwmon/ibmpowernv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c index 594254d6a72d..70ca833259ab 100644 --- a/drivers/hwmon/ibmpowernv.c +++ b/drivers/hwmon/ibmpowernv.c @@ -234,7 +234,7 @@ static int get_sensor_index_attr(const char *name, u32 *index, char *attr) if (copy_len >= sizeof(buf)) return -EINVAL; - strncpy(buf, hash_pos + 1, copy_len); + memcpy(buf, hash_pos + 1, copy_len); err = kstrtou32(buf, 10, index); if (err) From 8046da444df5fc35ed12b6b52c090dc89c8a5f96 Mon Sep 17 00:00:00 2001 From: Justin Stitt Date: Thu, 14 Sep 2023 23:10:34 +0000 Subject: [PATCH 26/73] hwmon: (asus_wmi_sensors) Replace deprecated strncpy() with strscpy() `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. If, for any reason, NUL-padding is needed let's opt for `strscpy_pad`. 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-hardening@vger.kernel.org Signed-off-by: Justin Stitt Reviewed-by: Kees Cook Link: https://lore.kernel.org/r/20230914-strncpy-drivers-hwmon-asus_wmi_sensors-c-v1-1-e1703cf91693@google.com Signed-off-by: Kees Cook --- drivers/hwmon/asus_wmi_sensors.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/hwmon/asus_wmi_sensors.c b/drivers/hwmon/asus_wmi_sensors.c index 6e8a908171f0..c2dd7ff882f2 100644 --- a/drivers/hwmon/asus_wmi_sensors.c +++ b/drivers/hwmon/asus_wmi_sensors.c @@ -300,7 +300,7 @@ static int asus_wmi_sensor_info(int index, struct asus_wmi_sensor_info *s) goto out_free_obj; } - strncpy(s->name, name_obj.string.pointer, sizeof(s->name) - 1); + strscpy(s->name, name_obj.string.pointer, sizeof(s->name)); data_type_obj = obj->package.elements[1]; if (data_type_obj.type != ACPI_TYPE_INTEGER) { From 6b343a46428255e7f383deda53b1ad38db513897 Mon Sep 17 00:00:00 2001 From: Justin Stitt Date: Mon, 18 Sep 2023 07:47:29 +0000 Subject: [PATCH 27/73] EDAC/mc_sysfs: Replace deprecated strncpy() with memcpy() `strncpy` is deprecated for use on NUL-terminated destination strings [1]. We've already calculated bounds, possible truncation with '\0' or '\n' and manually NUL-terminated. The situation is now just a literal byte copy from one buffer to another, let's treat it as such and use a less ambiguous interface in memcpy. 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-hardening@vger.kernel.org Signed-off-by: Justin Stitt Reviewed-by: Kees Cook Link: https://lore.kernel.org/r/20230918-strncpy-drivers-edac-edac_mc_sysfs-c-v4-1-38a23d2fcdd8@google.com Signed-off-by: Kees Cook --- drivers/edac/edac_mc_sysfs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c index 15f63452a9be..5116873c3330 100644 --- a/drivers/edac/edac_mc_sysfs.c +++ b/drivers/edac/edac_mc_sysfs.c @@ -229,7 +229,7 @@ static ssize_t channel_dimm_label_store(struct device *dev, if (copy_count == 0 || copy_count >= sizeof(rank->dimm->label)) return -EINVAL; - strncpy(rank->dimm->label, data, copy_count); + memcpy(rank->dimm->label, data, copy_count); rank->dimm->label[copy_count] = '\0'; return count; @@ -535,7 +535,7 @@ static ssize_t dimmdev_label_store(struct device *dev, if (copy_count == 0 || copy_count >= sizeof(dimm->label)) return -EINVAL; - strncpy(dimm->label, data, copy_count); + memcpy(dimm->label, data, copy_count); dimm->label[copy_count] = '\0'; return count; From 8fddc4b660273f96f2d216b04642b070a59c019c Mon Sep 17 00:00:00 2001 From: "Gustavo A. R. Silva" Date: Fri, 15 Sep 2023 12:43:20 -0600 Subject: [PATCH 28/73] drm/gud: Use size_add() in call to struct_size() If, for any reason, the open-coded arithmetic causes a wraparound, the protection that `struct_size()` adds against potential integer overflows is defeated. Fix this by hardening call to `struct_size()` with `size_add()`. Fixes: 40e1a70b4aed ("drm: Add GUD USB Display driver") Signed-off-by: "Gustavo A. R. Silva" Reviewed-by: Kees Cook Link: https://lore.kernel.org/r/ZQSlyHKPdw/zsy4c@work Signed-off-by: Kees Cook --- drivers/gpu/drm/gud/gud_pipe.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c index d2f199ea3c11..a02f75be81f0 100644 --- a/drivers/gpu/drm/gud/gud_pipe.c +++ b/drivers/gpu/drm/gud/gud_pipe.c @@ -503,7 +503,7 @@ int gud_pipe_check(struct drm_simple_display_pipe *pipe, return -ENOENT; len = struct_size(req, properties, - GUD_PROPERTIES_MAX_NUM + GUD_CONNECTOR_PROPERTIES_MAX_NUM); + size_add(GUD_PROPERTIES_MAX_NUM, GUD_CONNECTOR_PROPERTIES_MAX_NUM)); req = kzalloc(len, GFP_KERNEL); if (!req) return -ENOMEM; From b7fa76e03b0d83a035b20e0ef1a3d65a4557f76c Mon Sep 17 00:00:00 2001 From: "Gustavo A. R. Silva" Date: Fri, 15 Sep 2023 13:20:14 -0600 Subject: [PATCH 29/73] usb: atm: Use size_add() in call to struct_size() If, for any reason, the open-coded arithmetic causes a wraparound, the protection that `struct_size()` adds against potential integer overflows is defeated. Fix this by hardening call to `struct_size()` with `size_add()`. Fixes: b626871a7cda ("usb: atm: Use struct_size() helper") Signed-off-by: "Gustavo A. R. Silva" Reviewed-by: Kees Cook Link: https://lore.kernel.org/r/ZQSuboEIhvATAdxN@work Signed-off-by: Kees Cook --- drivers/usb/atm/usbatm.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/usb/atm/usbatm.c b/drivers/usb/atm/usbatm.c index 1cdb8758ae01..2da6615fbb6f 100644 --- a/drivers/usb/atm/usbatm.c +++ b/drivers/usb/atm/usbatm.c @@ -1018,7 +1018,8 @@ int usbatm_usb_probe(struct usb_interface *intf, const struct usb_device_id *id, size_t size; /* instance init */ - size = struct_size(instance, urbs, num_rcv_urbs + num_snd_urbs); + size = struct_size(instance, urbs, + size_add(num_rcv_urbs, num_snd_urbs)); instance = kzalloc(size, GFP_KERNEL); if (!instance) return -ENOMEM; From e91673b8dc59d7370492c653b84680503cac3aec Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 15 Sep 2023 13:15:23 -0700 Subject: [PATCH 30/73] ocfs2: Annotate struct ocfs2_slot_info 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 ocfs2_slot_info. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Mark Fasheh Cc: Joel Becker Cc: Joseph Qi Cc: ocfs2-devel@lists.linux.dev Reviewed-by: "Gustavo A. R. Silva" Reviewed-by: Joseph Qi Link: https://lore.kernel.org/r/20230915201522.never.979-kees@kernel.org Signed-off-by: Kees Cook --- fs/ocfs2/slot_map.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/ocfs2/slot_map.c b/fs/ocfs2/slot_map.c index da7718cef735..e544c704b583 100644 --- a/fs/ocfs2/slot_map.c +++ b/fs/ocfs2/slot_map.c @@ -37,7 +37,7 @@ struct ocfs2_slot_info { unsigned int si_blocks; struct buffer_head **si_bh; unsigned int si_num_slots; - struct ocfs2_slot si_slots[]; + struct ocfs2_slot si_slots[] __counted_by(si_num_slots); }; From 5234193ee2b997e59326b047610a8f3f64a0ce02 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 15 Sep 2023 13:15:17 -0700 Subject: [PATCH 31/73] ceph: Annotate struct ceph_osd_request 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 ceph_osd_request. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Ilya Dryomov Cc: Xiubo Li Cc: Jeff Layton Cc: ceph-devel@vger.kernel.org Reviewed-by: "Gustavo A. R. Silva" Reviewed-by: Xiubo Li Link: https://lore.kernel.org/r/20230915201517.never.373-kees@kernel.org Signed-off-by: Kees Cook --- include/linux/ceph/osd_client.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/ceph/osd_client.h b/include/linux/ceph/osd_client.h index bf9823956758..b8610e9d2471 100644 --- a/include/linux/ceph/osd_client.h +++ b/include/linux/ceph/osd_client.h @@ -278,7 +278,7 @@ struct ceph_osd_request { int r_attempts; u32 r_map_dne_bound; - struct ceph_osd_req_op r_ops[]; + struct ceph_osd_req_op r_ops[] __counted_by(r_num_ops); }; struct ceph_request_redirect { From aade15333c3b4fe24403fd9a52e15fb61993face Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 15 Sep 2023 13:14:56 -0700 Subject: [PATCH 32/73] afs: Annotate struct afs_permits 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 afs_permits. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: David Howells Cc: Marc Dionne Cc: linux-afs@lists.infradead.org Reviewed-by: "Gustavo A. R. Silva" Link: https://lore.kernel.org/r/20230915201456.never.529-kees@kernel.org Signed-off-by: Kees Cook --- fs/afs/internal.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/afs/internal.h b/fs/afs/internal.h index da73b97e19a9..813efd3784aa 100644 --- a/fs/afs/internal.h +++ b/fs/afs/internal.h @@ -705,7 +705,7 @@ struct afs_permits { refcount_t usage; unsigned short nr_permits; /* Number of records */ bool invalidated; /* Invalidated due to key change */ - struct afs_permit permits[]; /* List of permits sorted by key pointer */ + struct afs_permit permits[] __counted_by(nr_permits); /* List of permits sorted by key pointer */ }; /* From 2d26302bdff1300029a67931739f5770eac22493 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 15 Sep 2023 13:14:49 -0700 Subject: [PATCH 33/73] afs: Annotate struct afs_addr_list 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 afs_addr_list. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: David Howells Cc: Marc Dionne Cc: linux-afs@lists.infradead.org Reviewed-by: "Gustavo A. R. Silva" Link: https://lore.kernel.org/r/20230915201449.never.649-kees@kernel.org Signed-off-by: Kees Cook --- fs/afs/internal.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/afs/internal.h b/fs/afs/internal.h index 813efd3784aa..469a717467a4 100644 --- a/fs/afs/internal.h +++ b/fs/afs/internal.h @@ -87,7 +87,7 @@ struct afs_addr_list { enum dns_lookup_status status:8; unsigned long failed; /* Mask of addrs that failed locally/ICMP */ unsigned long responded; /* Mask of addrs that responded */ - struct sockaddr_rxrpc addrs[]; + struct sockaddr_rxrpc addrs[] __counted_by(max_addrs); #define AFS_MAX_ADDRESSES ((unsigned int)(sizeof(unsigned long) * 8)) }; From d5ae1c3b970eee69022c5f05b8c8da18427dc999 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 15 Sep 2023 12:58:16 -0700 Subject: [PATCH 34/73] usb: Annotate struct urb_priv 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 urb_priv. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Alan Stern Cc: Greg Kroah-Hartman Cc: Mathias Nyman Cc: linux-usb@vger.kernel.org Link: https://lore.kernel.org/r/20230915195812.never.371-kees@kernel.org Signed-off-by: Kees Cook --- drivers/usb/host/ohci.h | 2 +- drivers/usb/host/xhci.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/usb/host/ohci.h b/drivers/usb/host/ohci.h index aac6285b37f8..631dda6174b4 100644 --- a/drivers/usb/host/ohci.h +++ b/drivers/usb/host/ohci.h @@ -337,7 +337,7 @@ typedef struct urb_priv { u16 length; // # tds in this request u16 td_cnt; // tds already serviced struct list_head pending; - struct td *td[]; // all TDs in this request + struct td *td[] __counted_by(length); // all TDs in this request } urb_priv_t; diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index 7e282b4522c0..2f21c3a8565c 100644 --- a/drivers/usb/host/xhci.h +++ b/drivers/usb/host/xhci.h @@ -1666,7 +1666,7 @@ struct xhci_scratchpad { struct urb_priv { int num_tds; int num_tds_done; - struct xhci_td td[]; + struct xhci_td td[] __counted_by(num_tds); }; /* From c7c4ac7f4779bc186b3b8f9b55b6f19735480ef2 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 15 Sep 2023 12:58:49 -0700 Subject: [PATCH 35/73] usb: gadget: f_fs: Annotate struct ffs_buffer 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 ffs_buffer. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Greg Kroah-Hartman Cc: John Keeping Cc: Udipto Goswami Cc: Linyu Yuan Cc: linux-usb@vger.kernel.org Reviewed-by: "Gustavo A. R. Silva" Link: https://lore.kernel.org/r/20230915195849.never.275-kees@kernel.org Signed-off-by: Kees Cook --- drivers/usb/gadget/function/f_fs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c index 6e9ef35a43a7..af400d083777 100644 --- a/drivers/usb/gadget/function/f_fs.c +++ b/drivers/usb/gadget/function/f_fs.c @@ -202,7 +202,7 @@ struct ffs_epfile { struct ffs_buffer { size_t length; char *data; - char storage[]; + char storage[] __counted_by(length); }; /* ffs_io_data structure ***************************************************/ From 182717026e2c8e6cc3b5757f601c3fee15f64ecb Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 15 Sep 2023 12:59:39 -0700 Subject: [PATCH 36/73] usb: gadget: f_midi: Annotate struct f_midi 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 f_midi. Additionally, since the element count member must be set before accessing the annotated flexible array member, move its initialization earlier. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Greg Kroah-Hartman Cc: John Keeping Cc: Peter Chen Cc: Hulk Robot Cc: Allen Pais Cc: Will McVicker Cc: Davidlohr Bueso Cc: Zhang Qilong Cc: linux-usb@vger.kernel.org Reviewed-by: "Gustavo A. R. Silva" Link: https://lore.kernel.org/r/20230915195938.never.611-kees@kernel.org Signed-off-by: Kees Cook --- drivers/usb/gadget/function/f_midi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c index 2d02f25f9597..5335845d697b 100644 --- a/drivers/usb/gadget/function/f_midi.c +++ b/drivers/usb/gadget/function/f_midi.c @@ -99,7 +99,7 @@ struct f_midi { unsigned int in_last_port; unsigned char free_ref; - struct gmidi_in_port in_ports_array[/* in_ports */]; + struct gmidi_in_port in_ports_array[] __counted_by(in_ports); }; static inline struct f_midi *func_to_midi(struct usb_function *f) @@ -1349,6 +1349,7 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi) status = -ENOMEM; goto setup_fail; } + midi->in_ports = opts->in_ports; for (i = 0; i < opts->in_ports; i++) midi->in_ports_array[i].cable = i; @@ -1359,7 +1360,6 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi) status = -ENOMEM; goto midi_free; } - midi->in_ports = opts->in_ports; midi->out_ports = opts->out_ports; midi->index = opts->index; midi->buflen = opts->buflen; From 150849c5e2630ddf29411bb5334a548cc10d4814 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 15 Sep 2023 13:03:16 -0700 Subject: [PATCH 37/73] drbd: Annotate struct fifo_buffer with __counted_by MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 fifo_buffer. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Philipp Reisner Cc: Lars Ellenberg Cc: Christoph Böhmwalder Cc: Jens Axboe Cc: drbd-dev@lists.linbit.com Cc: linux-block@vger.kernel.org Reviewed-by: "Gustavo A. R. Silva" Link: https://lore.kernel.org/r/20230915200316.never.707-kees@kernel.org Signed-off-by: Kees Cook --- drivers/block/drbd/drbd_int.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/block/drbd/drbd_int.h b/drivers/block/drbd/drbd_int.h index a30a5ed811be..7eecc53fae3d 100644 --- a/drivers/block/drbd/drbd_int.h +++ b/drivers/block/drbd/drbd_int.h @@ -553,7 +553,7 @@ struct fifo_buffer { unsigned int head_index; unsigned int size; int total; /* sum of all values */ - int values[]; + int values[] __counted_by(size); }; extern struct fifo_buffer *fifo_alloc(unsigned int fifo_size); From e3260d90c8f35c03ce182bfd2eeea75805586c25 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 15 Sep 2023 13:03:36 -0700 Subject: [PATCH 38/73] dm raid: Annotate struct raid_set 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 raid_set. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Alasdair Kergon Cc: Mike Snitzer Cc: dm-devel@redhat.com Reviewed-by: "Gustavo A. R. Silva" Link: https://lore.kernel.org/r/20230915200335.never.098-kees@kernel.org Signed-off-by: Kees Cook --- drivers/md/dm-raid.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c index 5f9991765f27..9755788e8b78 100644 --- a/drivers/md/dm-raid.c +++ b/drivers/md/dm-raid.c @@ -254,7 +254,7 @@ struct raid_set { int mode; } journal_dev; - struct raid_dev dev[]; + struct raid_dev dev[] __counted_by(raid_disks); }; static void rs_config_backup(struct raid_set *rs, struct rs_layout *l) From 6521ba56ca86c19328f956090e367eacee4eb9d1 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 15 Sep 2023 13:03:45 -0700 Subject: [PATCH 39/73] dm crypt: Annotate struct crypt_config 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 crypt_config. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Alasdair Kergon Cc: Mike Snitzer Cc: dm-devel@redhat.com Reviewed-by: "Gustavo A. R. Silva" Link: https://lore.kernel.org/r/20230915200344.never.272-kees@kernel.org Signed-off-by: Kees Cook --- drivers/md/dm-crypt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c index f2662c21a6df..f276e9460feb 100644 --- a/drivers/md/dm-crypt.c +++ b/drivers/md/dm-crypt.c @@ -224,7 +224,7 @@ struct crypt_config { struct mutex bio_alloc_lock; u8 *authenc_key; /* space for keys in authenc() format (if used) */ - u8 key[]; + u8 key[] __counted_by(key_size); }; #define MIN_IOS 64 From 694b3b9d7acf771c4289babbe0f810556625b6b3 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 15 Sep 2023 13:03:53 -0700 Subject: [PATCH 40/73] dm: Annotate struct stripe_c 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 stripe_c. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Alasdair Kergon Cc: Mike Snitzer Cc: dm-devel@redhat.com Reviewed-by: "Gustavo A. R. Silva" Link: https://lore.kernel.org/r/20230915200352.never.118-kees@kernel.org Signed-off-by: Kees Cook --- drivers/md/dm-stripe.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/md/dm-stripe.c b/drivers/md/dm-stripe.c index e2854a3cbd28..5e70f5ae394d 100644 --- a/drivers/md/dm-stripe.c +++ b/drivers/md/dm-stripe.c @@ -44,7 +44,7 @@ struct stripe_c { /* Work struct used for triggering events*/ struct work_struct trigger_event; - struct stripe stripe[]; + struct stripe stripe[] __counted_by(stripes); }; /* From 37d27cf1f5836a5f889e66bee755254d4a1faf72 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 15 Sep 2023 13:04:01 -0700 Subject: [PATCH 41/73] dm: Annotate struct dm_stat 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 dm_stat. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Alasdair Kergon Cc: Mike Snitzer Cc: dm-devel@redhat.com Reviewed-by: "Gustavo A. R. Silva" Link: https://lore.kernel.org/r/20230915200400.never.585-kees@kernel.org Signed-off-by: Kees Cook --- drivers/md/dm-stats.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/md/dm-stats.c b/drivers/md/dm-stats.c index db2d997a6c18..bdc14ec99814 100644 --- a/drivers/md/dm-stats.c +++ b/drivers/md/dm-stats.c @@ -56,7 +56,7 @@ struct dm_stat { size_t percpu_alloc_size; size_t histogram_alloc_size; struct dm_stat_percpu *stat_percpu[NR_CPUS]; - struct dm_stat_shared stat_shared[]; + struct dm_stat_shared stat_shared[] __counted_by(n_entries); }; #define STAT_PRECISE_TIMESTAMPS 1 From 96d7c65939793c2efc16e43c81a92aed9eed0d78 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 15 Sep 2023 13:04:08 -0700 Subject: [PATCH 42/73] dm: Annotate struct dm_bio_prison 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 dm_bio_prison. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Alasdair Kergon Cc: Mike Snitzer Cc: dm-devel@redhat.com Reviewed-by: "Gustavo A. R. Silva" Link: https://lore.kernel.org/r/20230915200407.never.611-kees@kernel.org Signed-off-by: Kees Cook --- drivers/md/dm-bio-prison-v1.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/md/dm-bio-prison-v1.c b/drivers/md/dm-bio-prison-v1.c index 92afdca760ae..9ab32abe5ed4 100644 --- a/drivers/md/dm-bio-prison-v1.c +++ b/drivers/md/dm-bio-prison-v1.c @@ -26,7 +26,7 @@ struct prison_region { struct dm_bio_prison { mempool_t cell_pool; unsigned int num_locks; - struct prison_region regions[]; + struct prison_region regions[] __counted_by(num_locks); }; static struct kmem_cache *_cell_cache; From c0c64aac4916a2592bd174547b279a408368dd31 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 15 Sep 2023 13:14:27 -0700 Subject: [PATCH 43/73] nfs41: Annotate struct nfs4_file_layout_dsaddr 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 nfs4_file_layout_dsaddr. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Trond Myklebust Cc: Anna Schumaker Cc: "Gustavo A. R. Silva" Cc: linux-nfs@vger.kernel.org Link: https://lore.kernel.org/r/20230915201427.never.771-kees@kernel.org Signed-off-by: Kees Cook --- fs/nfs/filelayout/filelayout.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/nfs/filelayout/filelayout.h b/fs/nfs/filelayout/filelayout.h index aed0748fd6ec..c7bb5da93307 100644 --- a/fs/nfs/filelayout/filelayout.h +++ b/fs/nfs/filelayout/filelayout.h @@ -51,7 +51,7 @@ struct nfs4_file_layout_dsaddr { u32 stripe_count; u8 *stripe_indices; u32 ds_num; - struct nfs4_pnfs_ds *ds_list[]; + struct nfs4_pnfs_ds *ds_list[] __counted_by(ds_num); }; struct nfs4_filelayout_segment { From 1c674013547d0b9131dce0acdda74c2a5b95510e Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 15 Sep 2023 13:14:39 -0700 Subject: [PATCH 44/73] NFS/flexfiles: Annotate struct nfs4_ff_layout_segment 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 nfs4_ff_layout_segment. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Trond Myklebust Cc: Anna Schumaker Cc: linux-nfs@vger.kernel.org Reviewed-by: "Gustavo A. R. Silva" Link: https://lore.kernel.org/r/20230915201434.never.346-kees@kernel.org Signed-off-by: Kees Cook --- fs/nfs/flexfilelayout/flexfilelayout.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/nfs/flexfilelayout/flexfilelayout.h b/fs/nfs/flexfilelayout/flexfilelayout.h index 354a031c69b1..f84b3fb0dddd 100644 --- a/fs/nfs/flexfilelayout/flexfilelayout.h +++ b/fs/nfs/flexfilelayout/flexfilelayout.h @@ -99,7 +99,7 @@ struct nfs4_ff_layout_segment { u64 stripe_unit; u32 flags; u32 mirror_array_cnt; - struct nfs4_ff_layout_mirror *mirror_array[]; + struct nfs4_ff_layout_mirror *mirror_array[] __counted_by(mirror_array_cnt); }; struct nfs4_flexfile_layout { From 5c80c4fced22ae719d37db754144e75688eb52c8 Mon Sep 17 00:00:00 2001 From: Justin Stitt Date: Fri, 22 Sep 2023 11:58:06 +0000 Subject: [PATCH 45/73] isdn: replace deprecated strncpy with strscpy `strncpy` is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. We expect `iclock->name` to be NUL-terminated based on its use within printk: | printk(KERN_DEBUG "%s: %s %d\n", __func__, iclock->name, | iclock->pri); `iclock` is zero-initialized and as such is already NUL-padded which means strncpy is doing extra work here by eagerly NUL-padding the destination buffer. Considering the above, 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-hardening@vger.kernel.org Signed-off-by: Justin Stitt Reviewed-by: Kees Cook Link: https://lore.kernel.org/r/20230922-strncpy-drivers-isdn-misdn-clock-c-v1-1-3ba2a5ae627a@google.com Signed-off-by: Kees Cook --- drivers/isdn/mISDN/clock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/isdn/mISDN/clock.c b/drivers/isdn/mISDN/clock.c index 01d878168ef2..f71eb61db131 100644 --- a/drivers/isdn/mISDN/clock.c +++ b/drivers/isdn/mISDN/clock.c @@ -96,7 +96,7 @@ struct mISDNclock printk(KERN_ERR "%s: No memory for clock entry.\n", __func__); return NULL; } - strncpy(iclock->name, name, sizeof(iclock->name) - 1); + strscpy(iclock->name, name, sizeof(iclock->name)); iclock->pri = pri; iclock->priv = priv; iclock->ctl = ctl; From cba58fcbc4ab75d8814ec43db32d4830670526f8 Mon Sep 17 00:00:00 2001 From: Justin Stitt Date: Fri, 22 Sep 2023 11:49:14 +0000 Subject: [PATCH 46/73] isdn: kcapi: replace deprecated strncpy with strscpy_pad `strncpy` is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. `buf` is used in this context as a data buffer with 64 bytes of memory to be occupied by capi_manufakturer. We see the caller capi20_get_manufacturer() passes data.manufacturer as its `buf` argument which is then later passed over to user space. Due to this, let's keep the NUL-padding that strncpy provided by using strscpy_pad so as to not leak any stack data. | cdev->errcode = capi20_get_manufacturer(data.contr, data.manufacturer); | if (cdev->errcode) | return -EIO; | | if (copy_to_user(argp, data.manufacturer, | sizeof(data.manufacturer))) | return -EFAULT; Perhaps this would also be a good instance to use `strtomem_pad` for but in my testing the compiler was not able to determine the size of `buf` -- even with all the hints. 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-hardening@vger.kernel.org Signed-off-by: Justin Stitt Reviewed-by: Kees Cook Link: https://lore.kernel.org/r/20230922-strncpy-drivers-isdn-capi-kcapi-c-v1-1-55fcf8b075fb@google.com Signed-off-by: Kees Cook --- drivers/isdn/capi/kcapi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/isdn/capi/kcapi.c b/drivers/isdn/capi/kcapi.c index ae24848af233..136ba9fe55e0 100644 --- a/drivers/isdn/capi/kcapi.c +++ b/drivers/isdn/capi/kcapi.c @@ -732,7 +732,7 @@ u16 capi20_get_manufacturer(u32 contr, u8 buf[CAPI_MANUFACTURER_LEN]) u16 ret; if (contr == 0) { - strncpy(buf, capi_manufakturer, CAPI_MANUFACTURER_LEN); + strscpy_pad(buf, capi_manufakturer, CAPI_MANUFACTURER_LEN); return CAPI_NOERROR; } @@ -740,7 +740,7 @@ u16 capi20_get_manufacturer(u32 contr, u8 buf[CAPI_MANUFACTURER_LEN]) ctr = get_capi_ctr_by_nr(contr); if (ctr && ctr->state == CAPI_CTR_RUNNING) { - strncpy(buf, ctr->manu, CAPI_MANUFACTURER_LEN); + strscpy_pad(buf, ctr->manu, CAPI_MANUFACTURER_LEN); ret = CAPI_NOERROR; } else ret = CAPI_REGNOTINSTALLED; From d5ca24f639588811af57ceac513183fa2004bd3a Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Thu, 28 Sep 2023 20:24:32 -0700 Subject: [PATCH 47/73] binfmt_elf: Use elf_load() for library While load_elf_library() is a libc5-ism, we can still replace most of its contents with elf_load() as well, further simplifying the code. Some historical context: - libc4 was a.out and used uselib (a.out support has been removed) - libc5 was ELF and used uselib (there may still be users) - libc6 is ELF and has never used uselib Cc: Alexander Viro Cc: Christian Brauner Cc: linux-fsdevel@vger.kernel.org Cc: linux-mm@kvack.org Suggested-by: Eric Biederman Tested-by: Pedro Falcato Signed-off-by: Sebastian Ott Link: https://lore.kernel.org/r/20230929032435.2391507-4-keescook@chromium.org Signed-off-by: Kees Cook --- fs/binfmt_elf.c | 24 ++++-------------------- 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index db47cb802f89..f7012c1b8b05 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -1306,7 +1306,6 @@ static int load_elf_library(struct file *file) { struct elf_phdr *elf_phdata; struct elf_phdr *eppnt; - unsigned long elf_bss, bss, len; int retval, error, i, j; struct elfhdr elf_ex; @@ -1351,30 +1350,15 @@ static int load_elf_library(struct file *file) eppnt++; /* Now use mmap to map the library into memory. */ - error = vm_mmap(file, - ELF_PAGESTART(eppnt->p_vaddr), - (eppnt->p_filesz + - ELF_PAGEOFFSET(eppnt->p_vaddr)), + error = elf_load(file, ELF_PAGESTART(eppnt->p_vaddr), + eppnt, PROT_READ | PROT_WRITE | PROT_EXEC, MAP_FIXED_NOREPLACE | MAP_PRIVATE, - (eppnt->p_offset - - ELF_PAGEOFFSET(eppnt->p_vaddr))); + 0); + if (error != ELF_PAGESTART(eppnt->p_vaddr)) goto out_free_ph; - elf_bss = eppnt->p_vaddr + eppnt->p_filesz; - if (padzero(elf_bss)) { - error = -EFAULT; - goto out_free_ph; - } - - len = ELF_PAGEALIGN(eppnt->p_filesz + eppnt->p_vaddr); - bss = ELF_PAGEALIGN(eppnt->p_memsz + eppnt->p_vaddr); - if (bss > len) { - error = vm_brk(len, bss - len); - if (error) - goto out_free_ph; - } error = 0; out_free_ph: From f9c0a39d95301a36baacfd3495374c6128d662fa Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Thu, 28 Sep 2023 20:24:33 -0700 Subject: [PATCH 48/73] binfmt_elf: Only report padzero() errors when PROT_WRITE Errors with padzero() should be caught unless we're expecting a pathological (non-writable) segment. Report -EFAULT only when PROT_WRITE is present. Additionally add some more documentation to padzero(), elf_map(), and elf_load(). Cc: Eric Biederman Cc: Alexander Viro Cc: Christian Brauner Cc: linux-fsdevel@vger.kernel.org Cc: linux-mm@kvack.org Suggested-by: Eric Biederman Tested-by: Pedro Falcato Signed-off-by: Sebastian Ott Link: https://lore.kernel.org/r/20230929032435.2391507-5-keescook@chromium.org Signed-off-by: Kees Cook --- fs/binfmt_elf.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index f7012c1b8b05..5397b552fbeb 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -110,19 +110,19 @@ static struct linux_binfmt elf_format = { #define BAD_ADDR(x) (unlikely((unsigned long)(x) >= TASK_SIZE)) -/* We need to explicitly zero any fractional pages - after the data section (i.e. bss). This would - contain the junk from the file that should not - be in memory +/* + * We need to explicitly zero any trailing portion of the page that follows + * p_filesz when it ends before the page ends (e.g. bss), otherwise this + * memory will contain the junk from the file that should not be present. */ -static int padzero(unsigned long elf_bss) +static int padzero(unsigned long address) { unsigned long nbyte; - nbyte = ELF_PAGEOFFSET(elf_bss); + nbyte = ELF_PAGEOFFSET(address); if (nbyte) { nbyte = ELF_MIN_ALIGN - nbyte; - if (clear_user((void __user *) elf_bss, nbyte)) + if (clear_user((void __user *)address, nbyte)) return -EFAULT; } return 0; @@ -348,6 +348,11 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec, return 0; } +/* + * Map "eppnt->p_filesz" bytes from "filep" offset "eppnt->p_offset" + * into memory at "addr". (Note that p_filesz is rounded up to the + * next page, so any extra bytes from the file must be wiped.) + */ static unsigned long elf_map(struct file *filep, unsigned long addr, const struct elf_phdr *eppnt, int prot, int type, unsigned long total_size) @@ -387,6 +392,11 @@ static unsigned long elf_map(struct file *filep, unsigned long addr, return(map_addr); } +/* + * Map "eppnt->p_filesz" bytes from "filep" offset "eppnt->p_offset" + * into memory at "addr". Memory from "p_filesz" through "p_memsz" + * rounded up to the next page is zeroed. + */ static unsigned long elf_load(struct file *filep, unsigned long addr, const struct elf_phdr *eppnt, int prot, int type, unsigned long total_size) @@ -404,8 +414,12 @@ static unsigned long elf_load(struct file *filep, unsigned long addr, zero_end = map_addr + ELF_PAGEOFFSET(eppnt->p_vaddr) + eppnt->p_memsz; - /* Zero the end of the last mapped page */ - padzero(zero_start); + /* + * Zero the end of the last mapped page but ignore + * any errors if the segment isn't writable. + */ + if (padzero(zero_start) && (prot & PROT_WRITE)) + return -EFAULT; } } else { map_addr = zero_start = ELF_PAGESTART(addr); From 2632bb84d1d53cfd6cf65261064273ded4f759d5 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Thu, 28 Sep 2023 20:24:34 -0700 Subject: [PATCH 49/73] mm: Remove unused vm_brk() With fs/binfmt_elf.c fully refactored to use the new elf_load() helper, there are no more users of vm_brk(), so remove it. Cc: Andrew Morton Cc: linux-mm@kvack.org Suggested-by: Eric Biederman Tested-by: Pedro Falcato Signed-off-by: Sebastian Ott Link: https://lore.kernel.org/r/20230929032435.2391507-6-keescook@chromium.org Signed-off-by: Kees Cook --- include/linux/mm.h | 3 +-- mm/mmap.c | 6 ------ mm/nommu.c | 5 ----- 3 files changed, 1 insertion(+), 13 deletions(-) diff --git a/include/linux/mm.h b/include/linux/mm.h index bf5d0b1b16f4..216dd0c6dcf8 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -3308,8 +3308,7 @@ static inline void mm_populate(unsigned long addr, unsigned long len) static inline void mm_populate(unsigned long addr, unsigned long len) {} #endif -/* These take the mm semaphore themselves */ -extern int __must_check vm_brk(unsigned long, unsigned long); +/* This takes the mm semaphore itself */ extern int __must_check vm_brk_flags(unsigned long, unsigned long, unsigned long); extern int vm_munmap(unsigned long, size_t); extern unsigned long __must_check vm_mmap(struct file *, unsigned long, diff --git a/mm/mmap.c b/mm/mmap.c index b56a7f0c9f85..34d2337ace59 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -3174,12 +3174,6 @@ limits_failed: } EXPORT_SYMBOL(vm_brk_flags); -int vm_brk(unsigned long addr, unsigned long len) -{ - return vm_brk_flags(addr, len, 0); -} -EXPORT_SYMBOL(vm_brk); - /* Release all mmaps. */ void exit_mmap(struct mm_struct *mm) { diff --git a/mm/nommu.c b/mm/nommu.c index 7f9e9e5a0e12..23c43c208f2b 100644 --- a/mm/nommu.c +++ b/mm/nommu.c @@ -1531,11 +1531,6 @@ void exit_mmap(struct mm_struct *mm) mmap_write_unlock(mm); } -int vm_brk(unsigned long addr, unsigned long len) -{ - return -ENOMEM; -} - /* * expand (or shrink) an existing mapping, potentially moving it at the same * time (controlled by the MREMAP_MAYMOVE flag and available VM space) From 553e41d1bc95a09e32b6beb0a2b1c20dc4ea9b19 Mon Sep 17 00:00:00 2001 From: Greg Ungerer Date: Wed, 27 Sep 2023 23:29:33 +1000 Subject: [PATCH 50/73] binfmt_elf_fdpic: clean up debug warnings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The binfmt_elf_fdpic loader has some debug trace that can be enabled at build time. The recent 64-bit additions cause some warnings if that debug is enabled, such as: fs/binfmt_elf_fdpic.c: In function ‘elf_fdpic_map_file’: fs/binfmt_elf_fdpic.c:46:33: warning: format ‘%x’ expects argument of type ‘unsigned int’, but argument 3 has type ‘Elf64_Addr’ {aka ‘long long unsigned int’} [-Wformat=] 46 | #define kdebug(fmt, ...) printk("FDPIC "fmt"\n" ,##__VA_ARGS__ ) | ^~~~~~~~ ./include/linux/printk.h:427:25: note: in definition of macro ‘printk_index_wrap’ 427 | _p_func(_fmt, ##__VA_ARGS__); \ | ^~~~ Cast values to the largest possible type (which is equivilent to unsigned long long in this case) and use appropriate format specifiers to match. Fixes: b922bf04d2c1 ("binfmt_elf_fdpic: support 64-bit systems") Signed-off-by: Greg Ungerer Link: https://lore.kernel.org/r/20230927132933.3290734-1-gerg@kernel.org Signed-off-by: Kees Cook --- fs/binfmt_elf_fdpic.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c index 43b2a2851ba3..97c3e8551aac 100644 --- a/fs/binfmt_elf_fdpic.c +++ b/fs/binfmt_elf_fdpic.c @@ -900,10 +900,12 @@ static int elf_fdpic_map_file(struct elf_fdpic_params *params, kdebug("- DYNAMIC[]: %lx", params->dynamic_addr); seg = loadmap->segs; for (loop = 0; loop < loadmap->nsegs; loop++, seg++) - kdebug("- LOAD[%d] : %08x-%08x [va=%x ms=%x]", + kdebug("- LOAD[%d] : %08llx-%08llx [va=%llx ms=%llx]", loop, - seg->addr, seg->addr + seg->p_memsz - 1, - seg->p_vaddr, seg->p_memsz); + (unsigned long long) seg->addr, + (unsigned long long) seg->addr + seg->p_memsz - 1, + (unsigned long long) seg->p_vaddr, + (unsigned long long) seg->p_memsz); return 0; @@ -1082,9 +1084,10 @@ static int elf_fdpic_map_file_by_direct_mmap(struct elf_fdpic_params *params, maddr = vm_mmap(file, maddr, phdr->p_memsz + disp, prot, flags, phdr->p_offset - disp); - kdebug("mmap[%d] sz=%lx pr=%x fl=%x of=%lx --> %08lx", - loop, phdr->p_memsz + disp, prot, flags, - phdr->p_offset - disp, maddr); + kdebug("mmap[%d] sz=%llx pr=%x fl=%x of=%llx --> %08lx", + loop, (unsigned long long) phdr->p_memsz + disp, + prot, flags, (unsigned long long) phdr->p_offset - disp, + maddr); if (IS_ERR_VALUE(maddr)) return (int) maddr; @@ -1146,8 +1149,9 @@ static int elf_fdpic_map_file_by_direct_mmap(struct elf_fdpic_params *params, #else if (excess > 0) { - kdebug("clear[%d] ad=%lx sz=%lx", - loop, maddr + phdr->p_filesz, excess); + kdebug("clear[%d] ad=%llx sz=%lx", loop, + (unsigned long long) maddr + phdr->p_filesz, + excess); if (clear_user((void *) maddr + phdr->p_filesz, excess)) return -EFAULT; } From cfa36f889f232eb32e15b4ea6a688a5c5a9d19e9 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 22 Sep 2023 10:52:00 -0700 Subject: [PATCH 51/73] sparc: Annotate struct cpuinfo_tree 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 cpuinfo_tree. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: "David S. Miller" Cc: sparclinux@vger.kernel.org Reviewed-by: "Gustavo A. R. Silva" Link: https://lore.kernel.org/r/20230922175159.work.357-kees@kernel.org Signed-off-by: Kees Cook --- arch/sparc/kernel/cpumap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/sparc/kernel/cpumap.c b/arch/sparc/kernel/cpumap.c index f07ea88a83af..8fcf2d8c6bd2 100644 --- a/arch/sparc/kernel/cpumap.c +++ b/arch/sparc/kernel/cpumap.c @@ -50,7 +50,7 @@ struct cpuinfo_tree { /* Offsets into nodes[] for each level of the tree */ struct cpuinfo_level level[CPUINFO_LVL_MAX]; - struct cpuinfo_node nodes[]; + struct cpuinfo_node nodes[] __counted_by(total_nodes); }; From 4a530cb932af31b0c919a109bc107dd186653381 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 22 Sep 2023 10:50:53 -0700 Subject: [PATCH 52/73] hwmon: Annotate struct gsc_hwmon_platform_data 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 gsc_hwmon_platform_data. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Tim Harvey Reviewed-by: "Gustavo A. R. Silva" Link: https://lore.kernel.org/r/20230922175053.work.564-kees@kernel.org Signed-off-by: Kees Cook --- include/linux/platform_data/gsc_hwmon.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/platform_data/gsc_hwmon.h b/include/linux/platform_data/gsc_hwmon.h index f2781aa7eff8..70e8a6bec0f6 100644 --- a/include/linux/platform_data/gsc_hwmon.h +++ b/include/linux/platform_data/gsc_hwmon.h @@ -40,6 +40,6 @@ struct gsc_hwmon_platform_data { unsigned int resolution; unsigned int vreference; unsigned int fan_base; - struct gsc_hwmon_channel channels[]; + struct gsc_hwmon_channel channels[] __counted_by(nchannels); }; #endif From 51a71ab21f61ceb104aad2c9d29cd7e445adf1c5 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 22 Sep 2023 10:51:02 -0700 Subject: [PATCH 53/73] virt: acrn: Annotate struct vm_memory_region_batch 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 vm_memory_region_batch. Additionally, since the element count member must be set before accessing the annotated flexible array member, move its initialization earlier. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Fei Li Reviewed-by: "Gustavo A. R. Silva" Link: https://lore.kernel.org/r/20230922175102.work.020-kees@kernel.org Signed-off-by: Kees Cook --- drivers/virt/acrn/acrn_drv.h | 2 +- drivers/virt/acrn/mm.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/virt/acrn/acrn_drv.h b/drivers/virt/acrn/acrn_drv.h index 5663c17ad37c..fb8438094f6f 100644 --- a/drivers/virt/acrn/acrn_drv.h +++ b/drivers/virt/acrn/acrn_drv.h @@ -60,7 +60,7 @@ struct vm_memory_region_batch { u16 reserved[3]; u32 regions_num; u64 regions_gpa; - struct vm_memory_region_op regions_op[]; + struct vm_memory_region_op regions_op[] __counted_by(regions_num); }; /** diff --git a/drivers/virt/acrn/mm.c b/drivers/virt/acrn/mm.c index b4ad8d452e9a..fa5d9ca6be57 100644 --- a/drivers/virt/acrn/mm.c +++ b/drivers/virt/acrn/mm.c @@ -250,11 +250,11 @@ int acrn_vm_ram_map(struct acrn_vm *vm, struct acrn_vm_memmap *memmap) ret = -ENOMEM; goto unmap_kernel_map; } + regions_info->regions_num = nr_regions; /* Fill each vm_memory_region_op */ vm_region = regions_info->regions_op; regions_info->vmid = vm->vmid; - regions_info->regions_num = nr_regions; regions_info->regions_gpa = virt_to_phys(vm_region); user_vm_pa = memmap->user_vm_pa; i = 0; From a48e1f656b3c9b8192b6ca6fc92ef4daa30535fb Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 22 Sep 2023 10:51:21 -0700 Subject: [PATCH 54/73] KVM: Annotate struct kvm_irq_routing_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 kvm_irq_routing_table. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Paolo Bonzini Cc: kvm@vger.kernel.org Reviewed-by: "Gustavo A. R. Silva" Link: https://lore.kernel.org/r/20230922175121.work.660-kees@kernel.org Signed-off-by: Kees Cook --- include/linux/kvm_host.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index fb6c6109fdca..4944136efaa2 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -664,7 +664,7 @@ struct kvm_irq_routing_table { * Array indexed by gsi. Each entry contains list of irq chips * the gsi is connected to. */ - struct hlist_head map[]; + struct hlist_head map[] __counted_by(nr_rt_entries); }; #endif From 0f768682452867588d640b1bc1a5afdd1b59c584 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 22 Sep 2023 10:51:32 -0700 Subject: [PATCH 55/73] irqchip/imx-intmux: Annotate struct intmux_data 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 intmux_data. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Thomas Gleixner Cc: Marc Zyngier Cc: Shawn Guo Cc: Sascha Hauer Cc: Pengutronix Kernel Team Cc: Fabio Estevam Cc: NXP Linux Team Cc: linux-arm-kernel@lists.infradead.org Reviewed-by: "Gustavo A. R. Silva" Link: https://lore.kernel.org/r/20230922175131.work.718-kees@kernel.org Signed-off-by: Kees Cook --- drivers/irqchip/irq-imx-intmux.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/irqchip/irq-imx-intmux.c b/drivers/irqchip/irq-imx-intmux.c index 6d9a08238c9d..aa041e4dfee0 100644 --- a/drivers/irqchip/irq-imx-intmux.c +++ b/drivers/irqchip/irq-imx-intmux.c @@ -73,7 +73,7 @@ struct intmux_data { void __iomem *regs; struct clk *ipg_clk; int channum; - struct intmux_irqchip_data irqchip_data[]; + struct intmux_irqchip_data irqchip_data[] __counted_by(channum); }; static void imx_intmux_irq_mask(struct irq_data *d) From 86748637bff4d1fd1c4fb3c7e5aedf0baca44a93 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 22 Sep 2023 10:53:41 -0700 Subject: [PATCH 56/73] drivers: thermal: tsens: Annotate struct tsens_priv 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 tsens_priv. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Andy Gross Cc: Bjorn Andersson Cc: Konrad Dybcio Cc: Amit Kucheria Cc: Thara Gopinath Cc: "Rafael J. Wysocki" Cc: Daniel Lezcano Cc: Zhang Rui Cc: linux-arm-msm@vger.kernel.org Cc: linux-pm@vger.kernel.org Reviewed-by: "Gustavo A. R. Silva" Link: https://lore.kernel.org/r/20230922175341.work.919-kees@kernel.org Signed-off-by: Kees Cook --- drivers/thermal/qcom/tsens.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h index 2805de1c6827..cb637fa289ca 100644 --- a/drivers/thermal/qcom/tsens.h +++ b/drivers/thermal/qcom/tsens.h @@ -585,7 +585,7 @@ struct tsens_priv { struct dentry *debug_root; struct dentry *debug; - struct tsens_sensor sensor[]; + struct tsens_sensor sensor[] __counted_by(num_sensors); }; /** From c5225cd073c65a6d7e8e311ec0114792a671982a Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 22 Sep 2023 10:53:51 -0700 Subject: [PATCH 57/73] mailbox: zynqmp: Annotate struct zynqmp_ipi_pdata 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 zynqmp_ipi_pdata. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Jassi Brar Cc: Michal Simek Cc: linux-arm-kernel@lists.infradead.org Reviewed-by: Justin Stitt Reviewed-by: "Gustavo A. R. Silva" Acked-by: Michal Simek Link: https://lore.kernel.org/r/20230922175351.work.018-kees@kernel.org Signed-off-by: Kees Cook --- drivers/mailbox/zynqmp-ipi-mailbox.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/mailbox/zynqmp-ipi-mailbox.c b/drivers/mailbox/zynqmp-ipi-mailbox.c index e4fcac97dbfa..7fa533e80dd9 100644 --- a/drivers/mailbox/zynqmp-ipi-mailbox.c +++ b/drivers/mailbox/zynqmp-ipi-mailbox.c @@ -108,7 +108,7 @@ struct zynqmp_ipi_pdata { unsigned int method; u32 local_id; int num_mboxes; - struct zynqmp_ipi_mbox ipi_mboxes[]; + struct zynqmp_ipi_mbox ipi_mboxes[] __counted_by(num_mboxes); }; static struct device_driver zynqmp_ipi_mbox_driver = { From 381fdb73d1e2a48244de7260550e453d1003bb8e Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 6 Oct 2023 21:09:28 -0700 Subject: [PATCH 58/73] randstruct: Fix gcc-plugin performance mode to stay in group The performance mode of the gcc-plugin randstruct was shuffling struct members outside of the cache-line groups. Limit the range to the specified group indexes. Cc: linux-hardening@vger.kernel.org Cc: stable@vger.kernel.org Reported-by: Lukas Loidolt Closes: https://lore.kernel.org/all/f3ca77f0-e414-4065-83a5-ae4c4d25545d@student.tuwien.ac.at Fixes: 313dd1b62921 ("gcc-plugins: Add the randstruct plugin") Signed-off-by: Kees Cook --- scripts/gcc-plugins/randomize_layout_plugin.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/scripts/gcc-plugins/randomize_layout_plugin.c b/scripts/gcc-plugins/randomize_layout_plugin.c index 951b74ba1b24..366395cab490 100644 --- a/scripts/gcc-plugins/randomize_layout_plugin.c +++ b/scripts/gcc-plugins/randomize_layout_plugin.c @@ -191,12 +191,14 @@ static void partition_struct(tree *fields, unsigned long length, struct partitio static void performance_shuffle(tree *newtree, unsigned long length, ranctx *prng_state) { - unsigned long i, x; + unsigned long i, x, index; struct partition_group size_group[length]; unsigned long num_groups = 0; unsigned long randnum; partition_struct(newtree, length, (struct partition_group *)&size_group, &num_groups); + + /* FIXME: this group shuffle is currently a no-op. */ for (i = num_groups - 1; i > 0; i--) { struct partition_group tmp; randnum = ranval(prng_state) % (i + 1); @@ -206,11 +208,14 @@ static void performance_shuffle(tree *newtree, unsigned long length, ranctx *prn } for (x = 0; x < num_groups; x++) { - for (i = size_group[x].start + size_group[x].length - 1; i > size_group[x].start; i--) { + for (index = size_group[x].length - 1; index > 0; index--) { tree tmp; + + i = size_group[x].start + index; if (DECL_BIT_FIELD_TYPE(newtree[i])) continue; - randnum = ranval(prng_state) % (i + 1); + randnum = ranval(prng_state) % (index + 1); + randnum += size_group[x].start; // we could handle this case differently if desired if (DECL_BIT_FIELD_TYPE(newtree[randnum])) continue; From 1c5976ef0f7ad76319df748ccb99a4c7ba2ba464 Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Thu, 28 Oct 2021 12:31:13 +0200 Subject: [PATCH 59/73] binfmt_misc: cleanup on filesystem umount Currently, registering a new binary type pins the binfmt_misc filesystem. Specifically, this means that as long as there is at least one binary type registered the binfmt_misc filesystem survives all umounts, i.e. the superblock is not destroyed. Meaning that a umount followed by another mount will end up with the same superblock and the same binary type handlers. This is a behavior we tend to discourage for any new filesystems (apart from a few special filesystems such as e.g. configfs or debugfs). A umount operation without the filesystem being pinned - by e.g. someone holding a file descriptor to an open file - should usually result in the destruction of the superblock and all associated resources. This makes introspection easier and leads to clearly defined, simple and clean semantics. An administrator can rely on the fact that a umount will guarantee a clean slate making it possible to reinitialize a filesystem. Right now all binary types would need to be explicitly deleted before that can happen. This allows us to remove the heavy-handed calls to simple_pin_fs() and simple_release_fs() when creating and deleting binary types. This in turn allows us to replace the current brittle pinning mechanism abusing dget() which has caused a range of bugs judging from prior fixes in [2] and [3]. The additional dget() in load_misc_binary() pins the dentry but only does so for the sake to prevent ->evict_inode() from freeing the node when a user removes the binary type and kill_node() is run. Which would mean ->interpreter and ->interp_file would be freed causing a UAF. This isn't really nicely documented nor is it very clean because it relies on simple_pin_fs() pinning the filesystem as long as at least one binary type exists. Otherwise it would cause load_misc_binary() to hold on to a dentry belonging to a superblock that has been shutdown. Replace that implicit pinning with a clean and simple per-node refcount and get rid of the ugly dget() pinning. A similar mechanism exists for e.g. binderfs (cf. [4]). All the cleanup work can now be done in ->evict_inode(). In a follow-up patch we will make it possible to use binfmt_misc in sandboxes. We will use the cleaner semantics where a umount for the filesystem will cause the superblock and all resources to be deallocated. In preparation for this apply the same semantics to the initial binfmt_misc mount. Note, that this is a user-visible change and as such a uapi change but one that we can reasonably risk. We've discussed this in earlier versions of this patchset (cf. [1]). The main user and provider of binfmt_misc is systemd. Systemd provides binfmt_misc via autofs since it is configurable as a kernel module and is used by a few exotic packages and users. As such a binfmt_misc mount is triggered when /proc/sys/fs/binfmt_misc is accessed and is only provided on demand. Other autofs on demand filesystems include EFI ESP which systemd umounts if the mountpoint stays idle for a certain amount of time. This doesn't apply to the binfmt_misc autofs mount which isn't touched once it is mounted meaning this change can't accidently wipe binary type handlers without someone having explicitly unmounted binfmt_misc. After speaking to systemd folks they don't expect this change to affect them. In line with our general policy, if we see a regression for systemd or other users with this change we will switch back to the old behavior for the initial binfmt_misc mount and have binary types pin the filesystem again. But while we touch this code let's take the chance and let's improve on the status quo. [1]: https://lore.kernel.org/r/20191216091220.465626-2-laurent@vivier.eu [2]: commit 43a4f2619038 ("exec: binfmt_misc: fix race between load_misc_binary() and kill_node()" [3]: commit 83f918274e4b ("exec: binfmt_misc: shift filp_close(interp_file) from kill_node() to bm_evict_inode()") [4]: commit f0fe2c0f050d ("binder: prevent UAF for binderfs devices II") Link: https://lore.kernel.org/r/20211028103114.2849140-1-brauner@kernel.org (v1) Cc: Sargun Dhillon Cc: Serge Hallyn Cc: Jann Horn Cc: Henning Schild Cc: Andrei Vagin Cc: Al Viro Cc: Laurent Vivier Cc: linux-fsdevel@vger.kernel.org Acked-by: Serge Hallyn Signed-off-by: Christian Brauner Signed-off-by: Christian Brauner Signed-off-by: Kees Cook --- /* v2 */ - Christian Brauner : - Add more comments that explain what's going on. - Rename functions while changing them to better reflect what they are doing to make the code easier to understand. - In the first version when a specific binary type handler was removed either through a write to the entry's file or all binary type handlers were removed by a write to the binfmt_misc mount's status file all cleanup work happened during inode eviction. That includes removal of the relevant entries from entry list. While that works fine I disliked that model after thinking about it for a bit. Because it means that there was a window were someone has already removed a or all binary handlers but they could still be safely reached from load_misc_binary() when it has managed to take the read_lock() on the entries list while inode eviction was already happening. Again, that perfectly benign but it's cleaner to remove the binary handler from the list immediately meaning that ones the write to then entry's file or the binfmt_misc status file returns the binary type cannot be executed anymore. That gives stronger guarantees to the user. --- fs/binfmt_misc.c | 216 ++++++++++++++++++++++++++++++++++++----------- 1 file changed, 168 insertions(+), 48 deletions(-) diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c index e0108d17b085..cf5ed5cd4102 100644 --- a/fs/binfmt_misc.c +++ b/fs/binfmt_misc.c @@ -60,12 +60,11 @@ typedef struct { char *name; struct dentry *dentry; struct file *interp_file; + refcount_t users; /* sync removal with load_misc_binary() */ } Node; static DEFINE_RWLOCK(entries_lock); static struct file_system_type bm_fs_type; -static struct vfsmount *bm_mnt; -static int entry_count; /* * Max length of the register string. Determined by: @@ -82,19 +81,23 @@ static int entry_count; */ #define MAX_REGISTER_LENGTH 1920 -/* - * Check if we support the binfmt - * if we do, return the node, else NULL - * locking is done in load_misc_binary +/** + * search_binfmt_handler - search for a binary handler for @bprm + * @misc: handle to binfmt_misc instance + * @bprm: binary for which we are looking for a handler + * + * Search for a binary type handler for @bprm in the list of registered binary + * type handlers. + * + * Return: binary type list entry on success, NULL on failure */ -static Node *check_file(struct linux_binprm *bprm) +static Node *search_binfmt_handler(struct linux_binprm *bprm) { char *p = strrchr(bprm->interp, '.'); - struct list_head *l; + Node *e; /* Walk all the registered handlers. */ - list_for_each(l, &entries) { - Node *e = list_entry(l, Node, list); + list_for_each_entry(e, &entries, list) { char *s; int j; @@ -123,9 +126,49 @@ static Node *check_file(struct linux_binprm *bprm) if (j == e->size) return e; } + return NULL; } +/** + * get_binfmt_handler - try to find a binary type handler + * @misc: handle to binfmt_misc instance + * @bprm: binary for which we are looking for a handler + * + * Try to find a binfmt handler for the binary type. If one is found take a + * reference to protect against removal via bm_{entry,status}_write(). + * + * Return: binary type list entry on success, NULL on failure + */ +static Node *get_binfmt_handler(struct linux_binprm *bprm) +{ + Node *e; + + read_lock(&entries_lock); + e = search_binfmt_handler(bprm); + if (e) + refcount_inc(&e->users); + read_unlock(&entries_lock); + return e; +} + +/** + * put_binfmt_handler - put binary handler node + * @e: node to put + * + * Free node syncing with load_misc_binary() and defer final free to + * load_misc_binary() in case it is using the binary type handler we were + * requested to remove. + */ +static void put_binfmt_handler(Node *e) +{ + if (refcount_dec_and_test(&e->users)) { + if (e->flags & MISC_FMT_OPEN_FILE) + filp_close(e->interp_file, NULL); + kfree(e); + } +} + /* * the loader itself */ @@ -139,12 +182,7 @@ static int load_misc_binary(struct linux_binprm *bprm) if (!enabled) return retval; - /* to keep locking time low, we copy the interpreter string */ - read_lock(&entries_lock); - fmt = check_file(bprm); - if (fmt) - dget(fmt->dentry); - read_unlock(&entries_lock); + fmt = get_binfmt_handler(bprm); if (!fmt) return retval; @@ -198,7 +236,16 @@ static int load_misc_binary(struct linux_binprm *bprm) retval = 0; ret: - dput(fmt->dentry); + + /* + * If we actually put the node here all concurrent calls to + * load_misc_binary() will have finished. We also know + * that for the refcount to be zero ->evict_inode() must have removed + * the node to be deleted from the list. All that is left for us is to + * close and free. + */ + put_binfmt_handler(fmt); + return retval; } @@ -552,30 +599,90 @@ static struct inode *bm_get_inode(struct super_block *sb, int mode) return inode; } +/** + * bm_evict_inode - cleanup data associated with @inode + * @inode: inode to which the data is attached + * + * Cleanup the binary type handler data associated with @inode if a binary type + * entry is removed or the filesystem is unmounted and the super block is + * shutdown. + * + * If the ->evict call was not caused by a super block shutdown but by a write + * to remove the entry or all entries via bm_{entry,status}_write() the entry + * will have already been removed from the list. We keep the list_empty() check + * to make that explicit. +*/ static void bm_evict_inode(struct inode *inode) { Node *e = inode->i_private; - if (e && e->flags & MISC_FMT_OPEN_FILE) - filp_close(e->interp_file, NULL); - clear_inode(inode); - kfree(e); + + if (e) { + write_lock(&entries_lock); + if (!list_empty(&e->list)) + list_del_init(&e->list); + write_unlock(&entries_lock); + put_binfmt_handler(e); + } } -static void kill_node(Node *e) +/** + * unlink_binfmt_dentry - remove the dentry for the binary type handler + * @dentry: dentry associated with the binary type handler + * + * Do the actual filesystem work to remove a dentry for a registered binary + * type handler. Since binfmt_misc only allows simple files to be created + * directly under the root dentry of the filesystem we ensure that we are + * indeed passed a dentry directly beneath the root dentry, that the inode + * associated with the root dentry is locked, and that it is a regular file we + * are asked to remove. + */ +static void unlink_binfmt_dentry(struct dentry *dentry) { - struct dentry *dentry; + struct dentry *parent = dentry->d_parent; + struct inode *inode, *parent_inode; + /* All entries are immediate descendants of the root dentry. */ + if (WARN_ON_ONCE(dentry->d_sb->s_root != parent)) + return; + + /* We only expect to be called on regular files. */ + inode = d_inode(dentry); + if (WARN_ON_ONCE(!S_ISREG(inode->i_mode))) + return; + + /* The parent inode must be locked. */ + parent_inode = d_inode(parent); + if (WARN_ON_ONCE(!inode_is_locked(parent_inode))) + return; + + if (simple_positive(dentry)) { + dget(dentry); + simple_unlink(parent_inode, dentry); + d_delete(dentry); + dput(dentry); + } +} + +/** + * remove_binfmt_handler - remove a binary type handler + * @misc: handle to binfmt_misc instance + * @e: binary type handler to remove + * + * Remove a binary type handler from the list of binary type handlers and + * remove its associated dentry. This is called from + * binfmt_{entry,status}_write(). In the future, we might want to think about + * adding a proper ->unlink() method to binfmt_misc instead of forcing caller's + * to use writes to files in order to delete binary type handlers. But it has + * worked for so long that it's not a pressing issue. + */ +static void remove_binfmt_handler(Node *e) +{ write_lock(&entries_lock); list_del_init(&e->list); write_unlock(&entries_lock); - - dentry = e->dentry; - drop_nlink(d_inode(dentry)); - d_drop(dentry); - dput(dentry); - simple_release_fs(&bm_mnt, &entry_count); + unlink_binfmt_dentry(e->dentry); } /* / */ @@ -602,8 +709,8 @@ bm_entry_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos) static ssize_t bm_entry_write(struct file *file, const char __user *buffer, size_t count, loff_t *ppos) { - struct dentry *root; - Node *e = file_inode(file)->i_private; + struct inode *inode = file_inode(file); + Node *e = inode->i_private; int res = parse_command(buffer, count); switch (res) { @@ -617,13 +724,22 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer, break; case 3: /* Delete this handler. */ - root = file_inode(file)->i_sb->s_root; - inode_lock(d_inode(root)); + inode = d_inode(inode->i_sb->s_root); + inode_lock(inode); + /* + * In order to add new element or remove elements from the list + * via bm_{entry,register,status}_write() inode_lock() on the + * root inode must be held. + * The lock is exclusive ensuring that the list can't be + * modified. Only load_misc_binary() can access but does so + * read-only. So we only need to take the write lock when we + * actually remove the entry from the list. + */ if (!list_empty(&e->list)) - kill_node(e); + remove_binfmt_handler(e); - inode_unlock(d_inode(root)); + inode_unlock(inode); break; default: return res; @@ -682,13 +798,7 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer, if (!inode) goto out2; - err = simple_pin_fs(&bm_fs_type, &bm_mnt, &entry_count); - if (err) { - iput(inode); - inode = NULL; - goto out2; - } - + refcount_set(&e->users, 1); e->dentry = dget(dentry); inode->i_private = e; inode->i_fop = &bm_entry_operations; @@ -732,7 +842,8 @@ static ssize_t bm_status_write(struct file *file, const char __user *buffer, size_t count, loff_t *ppos) { int res = parse_command(buffer, count); - struct dentry *root; + Node *e, *next; + struct inode *inode; switch (res) { case 1: @@ -745,13 +856,22 @@ static ssize_t bm_status_write(struct file *file, const char __user *buffer, break; case 3: /* Delete all handlers. */ - root = file_inode(file)->i_sb->s_root; - inode_lock(d_inode(root)); + inode = d_inode(file_inode(file)->i_sb->s_root); + inode_lock(inode); - while (!list_empty(&entries)) - kill_node(list_first_entry(&entries, Node, list)); + /* + * In order to add new element or remove elements from the list + * via bm_{entry,register,status}_write() inode_lock() on the + * root inode must be held. + * The lock is exclusive ensuring that the list can't be + * modified. Only load_misc_binary() can access but does so + * read-only. So we only need to take the write lock when we + * actually remove the entry from the list. + */ + list_for_each_entry_safe(e, next, &entries, list) + remove_binfmt_handler(e); - inode_unlock(d_inode(root)); + inode_unlock(inode); break; default: return res; From 21ca59b365c091d583f36ac753eaa8baf947be6f Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Thu, 28 Oct 2021 12:31:14 +0200 Subject: [PATCH 60/73] binfmt_misc: enable sandboxed mounts Enable unprivileged sandboxes to create their own binfmt_misc mounts. This is based on Laurent's work in [1] but has been significantly reworked to fix various issues we identified in earlier versions. While binfmt_misc can currently only be mounted in the initial user namespace, binary types registered in this binfmt_misc instance are available to all sandboxes (Either by having them installed in the sandbox or by registering the binary type with the F flag causing the interpreter to be opened right away). So binfmt_misc binary types are already delegated to sandboxes implicitly. However, while a sandbox has access to all registered binary types in binfmt_misc a sandbox cannot currently register its own binary types in binfmt_misc. This has prevented various use-cases some of which were already outlined in [1] but we have a range of issues associated with this (cf. [3]-[5] below which are just a small sample). Extend binfmt_misc to be mountable in non-initial user namespaces. Similar to other filesystem such as nfsd, mqueue, and sunrpc we use keyed superblock management. The key determines whether we need to create a new superblock or can reuse an already existing one. We use the user namespace of the mount as key. This means a new binfmt_misc superblock is created once per user namespace creation. Subsequent mounts of binfmt_misc in the same user namespace will mount the same binfmt_misc instance. We explicitly do not create a new binfmt_misc superblock on every binfmt_misc mount as the semantics for load_misc_binary() line up with the keying model. This also allows us to retrieve the relevant binfmt_misc instance based on the caller's user namespace which can be done in a simple (bounded to 32 levels) loop. Similar to the current binfmt_misc semantics allowing access to the binary types in the initial binfmt_misc instance we do allow sandboxes access to their parent's binfmt_misc mounts if they do not have created a separate binfmt_misc instance. Overall, this will unblock the use-cases mentioned below and in general will also allow to support and harden execution of another architecture's binaries in tight sandboxes. For instance, using the unshare binary it possible to start a chroot of another architecture and configure the binfmt_misc interpreter without being root to run the binaries in this chroot and without requiring the host to modify its binary type handlers. Henning had already posted a few experiments in the cover letter at [1]. But here's an additional example where an unprivileged container registers qemu-user-static binary handlers for various binary types in its separate binfmt_misc mount and is then seamlessly able to start containers with a different architecture without affecting the host: root [lxc monitor] /var/snap/lxd/common/lxd/containers f1 1000000 \_ /sbin/init 1000000 \_ /lib/systemd/systemd-journald 1000000 \_ /lib/systemd/systemd-udevd 1000100 \_ /lib/systemd/systemd-networkd 1000101 \_ /lib/systemd/systemd-resolved 1000000 \_ /usr/sbin/cron -f 1000103 \_ /usr/bin/dbus-daemon --system --address=systemd: --nofork --nopidfile --systemd-activation --syslog-only 1000000 \_ /usr/bin/python3 /usr/bin/networkd-dispatcher --run-startup-triggers 1000104 \_ /usr/sbin/rsyslogd -n -iNONE 1000000 \_ /lib/systemd/systemd-logind 1000000 \_ /sbin/agetty -o -p -- \u --noclear --keep-baud console 115200,38400,9600 vt220 1000107 \_ dnsmasq --conf-file=/dev/null -u lxc-dnsmasq --strict-order --bind-interfaces --pid-file=/run/lxc/dnsmasq.pid --liste 1000000 \_ [lxc monitor] /var/lib/lxc f1-s390x 1100000 \_ /usr/bin/qemu-s390x-static /sbin/init 1100000 \_ /usr/bin/qemu-s390x-static /lib/systemd/systemd-journald 1100000 \_ /usr/bin/qemu-s390x-static /usr/sbin/cron -f 1100103 \_ /usr/bin/qemu-s390x-static /usr/bin/dbus-daemon --system --address=systemd: --nofork --nopidfile --systemd-ac 1100000 \_ /usr/bin/qemu-s390x-static /usr/bin/python3 /usr/bin/networkd-dispatcher --run-startup-triggers 1100104 \_ /usr/bin/qemu-s390x-static /usr/sbin/rsyslogd -n -iNONE 1100000 \_ /usr/bin/qemu-s390x-static /lib/systemd/systemd-logind 1100000 \_ /usr/bin/qemu-s390x-static /sbin/agetty -o -p -- \u --noclear --keep-baud console 115200,38400,9600 vt220 1100000 \_ /usr/bin/qemu-s390x-static /sbin/agetty -o -p -- \u --noclear --keep-baud pts/0 115200,38400,9600 vt220 1100000 \_ /usr/bin/qemu-s390x-static /sbin/agetty -o -p -- \u --noclear --keep-baud pts/1 115200,38400,9600 vt220 1100000 \_ /usr/bin/qemu-s390x-static /sbin/agetty -o -p -- \u --noclear --keep-baud pts/2 115200,38400,9600 vt220 1100000 \_ /usr/bin/qemu-s390x-static /sbin/agetty -o -p -- \u --noclear --keep-baud pts/3 115200,38400,9600 vt220 1100000 \_ /usr/bin/qemu-s390x-static /lib/systemd/systemd-udevd [1]: https://lore.kernel.org/all/20191216091220.465626-1-laurent@vivier.eu [2]: https://discuss.linuxcontainers.org/t/binfmt-misc-permission-denied [3]: https://discuss.linuxcontainers.org/t/lxd-binfmt-support-for-qemu-static-interpreters [4]: https://discuss.linuxcontainers.org/t/3-1-0-binfmt-support-service-in-unprivileged-guest-requires-write-access-on-hosts-proc-sys-fs-binfmt-misc [5]: https://discuss.linuxcontainers.org/t/qemu-user-static-not-working-4-11 Link: https://lore.kernel.org/r/20191216091220.465626-2-laurent@vivier.eu (origin) Link: https://lore.kernel.org/r/20211028103114.2849140-2-brauner@kernel.org (v1) Cc: Sargun Dhillon Cc: Serge Hallyn Cc: Jann Horn Cc: Henning Schild Cc: Andrei Vagin Cc: Al Viro Cc: Laurent Vivier Cc: linux-fsdevel@vger.kernel.org Signed-off-by: Laurent Vivier Signed-off-by: Christian Brauner Signed-off-by: Christian Brauner Signed-off-by: Kees Cook --- /* v2 */ - Serge Hallyn : - Use GFP_KERNEL_ACCOUNT for userspace triggered allocations when a new binary type handler is registered. - Christian Brauner : - Switch authorship to me. I refused to do that earlier even though Laurent said I should do so because I think it's genuinely bad form. But by now I have changed so many things that it'd be unfair to blame Laurent for any potential bugs in here. - Add more comments that explain what's going on. - Rename functions while changing them to better reflect what they are doing to make the code easier to understand. - In the first version when a specific binary type handler was removed either through a write to the entry's file or all binary type handlers were removed by a write to the binfmt_misc mount's status file all cleanup work happened during inode eviction. That includes removal of the relevant entries from entry list. While that works fine I disliked that model after thinking about it for a bit. Because it means that there was a window were someone has already removed a or all binary handlers but they could still be safely reached from load_misc_binary() when it has managed to take the read_lock() on the entries list while inode eviction was already happening. Again, that perfectly benign but it's cleaner to remove the binary handler from the list immediately meaning that ones the write to then entry's file or the binfmt_misc status file returns the binary type cannot be executed anymore. That gives stronger guarantees to the user. --- fs/binfmt_misc.c | 202 +++++++++++++++++++++++++++------ include/linux/binfmts.h | 10 ++ include/linux/user_namespace.h | 8 ++ kernel/user.c | 13 +++ kernel/user_namespace.c | 3 + 5 files changed, 202 insertions(+), 34 deletions(-) diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c index cf5ed5cd4102..deacc105119d 100644 --- a/fs/binfmt_misc.c +++ b/fs/binfmt_misc.c @@ -40,9 +40,6 @@ enum { VERBOSE_STATUS = 1 /* make it zero to save 400 bytes kernel memory */ }; -static LIST_HEAD(entries); -static int enabled = 1; - enum {Enabled, Magic}; #define MISC_FMT_PRESERVE_ARGV0 (1UL << 31) #define MISC_FMT_OPEN_BINARY (1UL << 30) @@ -63,7 +60,6 @@ typedef struct { refcount_t users; /* sync removal with load_misc_binary() */ } Node; -static DEFINE_RWLOCK(entries_lock); static struct file_system_type bm_fs_type; /* @@ -91,13 +87,14 @@ static struct file_system_type bm_fs_type; * * Return: binary type list entry on success, NULL on failure */ -static Node *search_binfmt_handler(struct linux_binprm *bprm) +static Node *search_binfmt_handler(struct binfmt_misc *misc, + struct linux_binprm *bprm) { char *p = strrchr(bprm->interp, '.'); Node *e; /* Walk all the registered handlers. */ - list_for_each_entry(e, &entries, list) { + list_for_each_entry(e, &misc->entries, list) { char *s; int j; @@ -140,15 +137,16 @@ static Node *search_binfmt_handler(struct linux_binprm *bprm) * * Return: binary type list entry on success, NULL on failure */ -static Node *get_binfmt_handler(struct linux_binprm *bprm) +static Node *get_binfmt_handler(struct binfmt_misc *misc, + struct linux_binprm *bprm) { Node *e; - read_lock(&entries_lock); - e = search_binfmt_handler(bprm); + read_lock(&misc->entries_lock); + e = search_binfmt_handler(misc, bprm); if (e) refcount_inc(&e->users); - read_unlock(&entries_lock); + read_unlock(&misc->entries_lock); return e; } @@ -169,6 +167,35 @@ static void put_binfmt_handler(Node *e) } } +/** + * load_binfmt_misc - load the binfmt_misc of the caller's user namespace + * + * To be called in load_misc_binary() to load the relevant struct binfmt_misc. + * If a user namespace doesn't have its own binfmt_misc mount it can make use + * of its ancestor's binfmt_misc handlers. This mimicks the behavior of + * pre-namespaced binfmt_misc where all registered binfmt_misc handlers where + * available to all user and user namespaces on the system. + * + * Return: the binfmt_misc instance of the caller's user namespace + */ +static struct binfmt_misc *load_binfmt_misc(void) +{ + const struct user_namespace *user_ns; + struct binfmt_misc *misc; + + user_ns = current_user_ns(); + while (user_ns) { + /* Pairs with smp_store_release() in bm_fill_super(). */ + misc = smp_load_acquire(&user_ns->binfmt_misc); + if (misc) + return misc; + + user_ns = user_ns->parent; + } + + return &init_binfmt_misc; +} + /* * the loader itself */ @@ -176,13 +203,14 @@ static int load_misc_binary(struct linux_binprm *bprm) { Node *fmt; struct file *interp_file = NULL; - int retval; + int retval = -ENOEXEC; + struct binfmt_misc *misc; - retval = -ENOEXEC; - if (!enabled) + misc = load_binfmt_misc(); + if (!misc->enabled) return retval; - fmt = get_binfmt_handler(bprm); + fmt = get_binfmt_handler(misc, bprm); if (!fmt) return retval; @@ -240,9 +268,9 @@ ret: /* * If we actually put the node here all concurrent calls to * load_misc_binary() will have finished. We also know - * that for the refcount to be zero ->evict_inode() must have removed - * the node to be deleted from the list. All that is left for us is to - * close and free. + * that for the refcount to be zero someone must have concurently + * removed the binary type handler from the list and it's our job to + * free it. */ put_binfmt_handler(fmt); @@ -334,7 +362,7 @@ static Node *create_entry(const char __user *buffer, size_t count) err = -ENOMEM; memsize = sizeof(Node) + count + 8; - e = kmalloc(memsize, GFP_KERNEL); + e = kmalloc(memsize, GFP_KERNEL_ACCOUNT); if (!e) goto out; @@ -446,7 +474,7 @@ static Node *create_entry(const char __user *buffer, size_t count) if (e->mask) { int i; - char *masked = kmalloc(e->size, GFP_KERNEL); + char *masked = kmalloc(e->size, GFP_KERNEL_ACCOUNT); print_hex_dump_bytes( KBUILD_MODNAME ": register: mask[decoded]: ", @@ -599,6 +627,22 @@ static struct inode *bm_get_inode(struct super_block *sb, int mode) return inode; } +/** + * i_binfmt_misc - retrieve struct binfmt_misc from a binfmt_misc inode + * @inode: inode of the relevant binfmt_misc instance + * + * This helper retrieves struct binfmt_misc from a binfmt_misc inode. This can + * be done without any memory barriers because we are guaranteed that + * user_ns->binfmt_misc is fully initialized. It was fully initialized when the + * binfmt_misc mount was first created. + * + * Return: struct binfmt_misc of the relevant binfmt_misc instance + */ +static struct binfmt_misc *i_binfmt_misc(struct inode *inode) +{ + return inode->i_sb->s_user_ns->binfmt_misc; +} + /** * bm_evict_inode - cleanup data associated with @inode * @inode: inode to which the data is attached @@ -619,10 +663,13 @@ static void bm_evict_inode(struct inode *inode) clear_inode(inode); if (e) { - write_lock(&entries_lock); + struct binfmt_misc *misc; + + misc = i_binfmt_misc(inode); + write_lock(&misc->entries_lock); if (!list_empty(&e->list)) list_del_init(&e->list); - write_unlock(&entries_lock); + write_unlock(&misc->entries_lock); put_binfmt_handler(e); } } @@ -677,11 +724,11 @@ static void unlink_binfmt_dentry(struct dentry *dentry) * to use writes to files in order to delete binary type handlers. But it has * worked for so long that it's not a pressing issue. */ -static void remove_binfmt_handler(Node *e) +static void remove_binfmt_handler(struct binfmt_misc *misc, Node *e) { - write_lock(&entries_lock); + write_lock(&misc->entries_lock); list_del_init(&e->list); - write_unlock(&entries_lock); + write_unlock(&misc->entries_lock); unlink_binfmt_dentry(e->dentry); } @@ -737,7 +784,7 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer, * actually remove the entry from the list. */ if (!list_empty(&e->list)) - remove_binfmt_handler(e); + remove_binfmt_handler(i_binfmt_misc(inode), e); inode_unlock(inode); break; @@ -763,6 +810,7 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer, struct inode *inode; struct super_block *sb = file_inode(file)->i_sb; struct dentry *root = sb->s_root, *dentry; + struct binfmt_misc *misc; int err = 0; struct file *f = NULL; @@ -772,7 +820,18 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer, return PTR_ERR(e); if (e->flags & MISC_FMT_OPEN_FILE) { + const struct cred *old_cred; + + /* + * Now that we support unprivileged binfmt_misc mounts make + * sure we use the credentials that the register @file was + * opened with to also open the interpreter. Before that this + * didn't matter much as only a privileged process could open + * the register file. + */ + old_cred = override_creds(file->f_cred); f = open_exec(e->interpreter); + revert_creds(old_cred); if (IS_ERR(f)) { pr_notice("register: failed to install interpreter file %s\n", e->interpreter); @@ -804,9 +863,10 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer, inode->i_fop = &bm_entry_operations; d_instantiate(dentry, inode); - write_lock(&entries_lock); - list_add(&e->list, &entries); - write_unlock(&entries_lock); + misc = i_binfmt_misc(inode); + write_lock(&misc->entries_lock); + list_add(&e->list, &misc->entries); + write_unlock(&misc->entries_lock); err = 0; out2: @@ -833,26 +893,31 @@ static const struct file_operations bm_register_operations = { static ssize_t bm_status_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos) { - char *s = enabled ? "enabled\n" : "disabled\n"; + struct binfmt_misc *misc; + char *s; + misc = i_binfmt_misc(file_inode(file)); + s = misc->enabled ? "enabled\n" : "disabled\n"; return simple_read_from_buffer(buf, nbytes, ppos, s, strlen(s)); } static ssize_t bm_status_write(struct file *file, const char __user *buffer, size_t count, loff_t *ppos) { + struct binfmt_misc *misc; int res = parse_command(buffer, count); Node *e, *next; struct inode *inode; + misc = i_binfmt_misc(file_inode(file)); switch (res) { case 1: /* Disable all handlers. */ - enabled = 0; + misc->enabled = false; break; case 2: /* Enable all handlers. */ - enabled = 1; + misc->enabled = true; break; case 3: /* Delete all handlers. */ @@ -868,8 +933,8 @@ static ssize_t bm_status_write(struct file *file, const char __user *buffer, * read-only. So we only need to take the write lock when we * actually remove the entry from the list. */ - list_for_each_entry_safe(e, next, &entries, list) - remove_binfmt_handler(e); + list_for_each_entry_safe(e, next, &misc->entries, list) + remove_binfmt_handler(misc, e); inode_unlock(inode); break; @@ -888,32 +953,100 @@ static const struct file_operations bm_status_operations = { /* Superblock handling */ +static void bm_put_super(struct super_block *sb) +{ + struct user_namespace *user_ns = sb->s_fs_info; + + sb->s_fs_info = NULL; + put_user_ns(user_ns); +} + static const struct super_operations s_ops = { .statfs = simple_statfs, .evict_inode = bm_evict_inode, + .put_super = bm_put_super, }; static int bm_fill_super(struct super_block *sb, struct fs_context *fc) { int err; + struct user_namespace *user_ns = sb->s_user_ns; + struct binfmt_misc *misc; static const struct tree_descr bm_files[] = { [2] = {"status", &bm_status_operations, S_IWUSR|S_IRUGO}, [3] = {"register", &bm_register_operations, S_IWUSR}, /* last one */ {""} }; + if (WARN_ON(user_ns != current_user_ns())) + return -EINVAL; + + /* + * Lazily allocate a new binfmt_misc instance for this namespace, i.e. + * do it here during the first mount of binfmt_misc. We don't need to + * waste memory for every user namespace allocation. It's likely much + * more common to not mount a separate binfmt_misc instance than it is + * to mount one. + * + * While multiple superblocks can exist they are keyed by userns in + * s_fs_info for binfmt_misc. Hence, the vfs guarantees that + * bm_fill_super() is called exactly once whenever a binfmt_misc + * superblock for a userns is created. This in turn lets us conclude + * that when a binfmt_misc superblock is created for the first time for + * a userns there's no one racing us. Therefore we don't need any + * barriers when we dereference binfmt_misc. + */ + misc = user_ns->binfmt_misc; + if (!misc) { + /* + * If it turns out that most user namespaces actually want to + * register their own binary type handler and therefore all + * create their own separate binfm_misc mounts we should + * consider turning this into a kmem cache. + */ + misc = kzalloc(sizeof(struct binfmt_misc), GFP_KERNEL); + if (!misc) + return -ENOMEM; + + INIT_LIST_HEAD(&misc->entries); + rwlock_init(&misc->entries_lock); + + /* Pairs with smp_load_acquire() in load_binfmt_misc(). */ + smp_store_release(&user_ns->binfmt_misc, misc); + } + + /* + * When the binfmt_misc superblock for this userns is shutdown + * ->enabled might have been set to false and we don't reinitialize + * ->enabled again in put_super() as someone might already be mounting + * binfmt_misc again. It also would be pointless since by the time + * ->put_super() is called we know that the binary type list for this + * bintfmt_misc mount is empty making load_misc_binary() return + * -ENOEXEC independent of whether ->enabled is true. Instead, if + * someone mounts binfmt_misc for the first time or again we simply + * reset ->enabled to true. + */ + misc->enabled = true; + err = simple_fill_super(sb, BINFMTFS_MAGIC, bm_files); if (!err) sb->s_op = &s_ops; return err; } +static void bm_free(struct fs_context *fc) +{ + if (fc->s_fs_info) + put_user_ns(fc->s_fs_info); +} + static int bm_get_tree(struct fs_context *fc) { - return get_tree_single(fc, bm_fill_super); + return get_tree_keyed(fc, bm_fill_super, get_user_ns(fc->user_ns)); } static const struct fs_context_operations bm_context_ops = { + .free = bm_free, .get_tree = bm_get_tree, }; @@ -932,6 +1065,7 @@ static struct file_system_type bm_fs_type = { .owner = THIS_MODULE, .name = "binfmt_misc", .init_fs_context = bm_init_fs_context, + .fs_flags = FS_USERNS_MOUNT, .kill_sb = kill_litter_super, }; MODULE_ALIAS_FS("binfmt_misc"); diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h index 8d51f69f9f5e..70f97f685bff 100644 --- a/include/linux/binfmts.h +++ b/include/linux/binfmts.h @@ -90,6 +90,16 @@ struct linux_binfmt { #endif } __randomize_layout; +#if IS_ENABLED(CONFIG_BINFMT_MISC) +struct binfmt_misc { + struct list_head entries; + rwlock_t entries_lock; + bool enabled; +} __randomize_layout; + +extern struct binfmt_misc init_binfmt_misc; +#endif + extern void __register_binfmt(struct linux_binfmt *fmt, int insert); /* Registration of default binfmt handlers */ diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h index 45f09bec02c4..6030a8235617 100644 --- a/include/linux/user_namespace.h +++ b/include/linux/user_namespace.h @@ -65,6 +65,10 @@ enum rlimit_type { UCOUNT_RLIMIT_COUNTS, }; +#if IS_ENABLED(CONFIG_BINFMT_MISC) +struct binfmt_misc; +#endif + struct user_namespace { struct uid_gid_map uid_map; struct uid_gid_map gid_map; @@ -102,6 +106,10 @@ struct user_namespace { struct ucounts *ucounts; long ucount_max[UCOUNT_COUNTS]; long rlimit_max[UCOUNT_RLIMIT_COUNTS]; + +#if IS_ENABLED(CONFIG_BINFMT_MISC) + struct binfmt_misc *binfmt_misc; +#endif } __randomize_layout; struct ucounts { diff --git a/kernel/user.c b/kernel/user.c index d667debeafd6..03cedc366dc9 100644 --- a/kernel/user.c +++ b/kernel/user.c @@ -18,8 +18,18 @@ #include #include #include +#include #include +#if IS_ENABLED(CONFIG_BINFMT_MISC) +struct binfmt_misc init_binfmt_misc = { + .entries = LIST_HEAD_INIT(init_binfmt_misc.entries), + .enabled = true, + .entries_lock = __RW_LOCK_UNLOCKED(init_binfmt_misc.entries_lock), +}; +EXPORT_SYMBOL_GPL(init_binfmt_misc); +#endif + /* * userns count is 1 for root user, 1 for init_uts_ns, * and 1 for... ? @@ -67,6 +77,9 @@ struct user_namespace init_user_ns = { .keyring_name_list = LIST_HEAD_INIT(init_user_ns.keyring_name_list), .keyring_sem = __RWSEM_INITIALIZER(init_user_ns.keyring_sem), #endif +#if IS_ENABLED(CONFIG_BINFMT_MISC) + .binfmt_misc = &init_binfmt_misc, +#endif }; EXPORT_SYMBOL_GPL(init_user_ns); diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c index 1d8e47bed3f1..d52a894ecf57 100644 --- a/kernel/user_namespace.c +++ b/kernel/user_namespace.c @@ -213,6 +213,9 @@ static void free_user_ns(struct work_struct *work) kfree(ns->projid_map.forward); kfree(ns->projid_map.reverse); } +#if IS_ENABLED(CONFIG_BINFMT_MISC) + kfree(ns->binfmt_misc); +#endif retire_userns_sysctls(ns); key_free_user_ns(ns); ns_free_inum(&ns->ns); From faed498d0db78adc1eee6bab3a8480bcb7e17e6e Mon Sep 17 00:00:00 2001 From: Lukas Bulwahn Date: Thu, 12 Oct 2023 06:50:40 +0200 Subject: [PATCH 61/73] hardening: x86: drop reference to removed config AMD_IOMMU_V2 Commit 5a0b11a180a9 ("iommu/amd: Remove iommu_v2 module") removes the config AMD_IOMMU_V2. Remove the reference to this config in the x86 architecture-specific hardening config fragment as well. Signed-off-by: Lukas Bulwahn Reviewed-by: Vasant Hegde Link: https://lore.kernel.org/r/20231012045040.22088-1-lukas.bulwahn@gmail.com Signed-off-by: Kees Cook --- arch/x86/configs/hardening.config | 1 - 1 file changed, 1 deletion(-) diff --git a/arch/x86/configs/hardening.config b/arch/x86/configs/hardening.config index 19bb0c7a7669..7b497f3b7bc3 100644 --- a/arch/x86/configs/hardening.config +++ b/arch/x86/configs/hardening.config @@ -9,7 +9,6 @@ CONFIG_INTEL_IOMMU=y CONFIG_INTEL_IOMMU_DEFAULT_ON=y CONFIG_INTEL_IOMMU_SVM=y CONFIG_AMD_IOMMU=y -CONFIG_AMD_IOMMU_V2=y # Enable CET Shadow Stack for userspace. CONFIG_X86_USER_SHADOW_STACK=y From a19d48f7c5d57c0f0405a7d4334d1d38fe9d3c1c Mon Sep 17 00:00:00 2001 From: Jiasheng Jiang Date: Fri, 23 Jun 2023 10:27:06 +0800 Subject: [PATCH 62/73] pstore/platform: Add check for kstrdup Add check for the return value of kstrdup() and return the error if it fails in order to avoid NULL pointer dereference. Fixes: 563ca40ddf40 ("pstore/platform: Switch pstore_info::name to const") Signed-off-by: Jiasheng Jiang Link: https://lore.kernel.org/r/20230623022706.32125-1-jiasheng@iscas.ac.cn Signed-off-by: Kees Cook --- fs/pstore/platform.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c index e5bca9a004cc..03425928d2fb 100644 --- a/fs/pstore/platform.c +++ b/fs/pstore/platform.c @@ -464,6 +464,8 @@ out: */ int pstore_register(struct pstore_info *psi) { + char *new_backend; + if (backend && strcmp(backend, psi->name)) { pr_warn("backend '%s' already in use: ignoring '%s'\n", backend, psi->name); @@ -484,11 +486,16 @@ int pstore_register(struct pstore_info *psi) return -EINVAL; } + new_backend = kstrdup(psi->name, GFP_KERNEL); + if (!new_backend) + return -ENOMEM; + mutex_lock(&psinfo_lock); if (psinfo) { pr_warn("backend '%s' already loaded: ignoring '%s'\n", psinfo->name, psi->name); mutex_unlock(&psinfo_lock); + kfree(new_backend); return -EBUSY; } @@ -521,7 +528,7 @@ int pstore_register(struct pstore_info *psi) * Update the module parameter backend, so it is visible * through /sys/module/pstore/parameters/backend */ - backend = kstrdup(psi->name, GFP_KERNEL); + backend = new_backend; pr_info("Registered %s as persistent store backend\n", psi->name); From 0e108725f6cc5b3be9e607f89c9fbcbb236367b7 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Wed, 18 Oct 2023 10:53:58 -0700 Subject: [PATCH 63/73] string: Adjust strtomem() logic to allow for smaller sources Arnd noticed we have a case where a shorter source string is being copied into a destination byte array, but this results in a strnlen() call that exceeds the size of the source. This is seen with -Wstringop-overread: In file included from ../include/linux/uuid.h:11, from ../include/linux/mod_devicetable.h:14, from ../include/linux/cpufeature.h:12, from ../arch/x86/coco/tdx/tdx.c:7: ../arch/x86/coco/tdx/tdx.c: In function 'tdx_panic.constprop': ../include/linux/string.h:284:9: error: 'strnlen' specified bound 64 exceeds source size 60 [-Werror=stringop-overread] 284 | memcpy_and_pad(dest, _dest_len, src, strnlen(src, _dest_len), pad); \ | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../arch/x86/coco/tdx/tdx.c:124:9: note: in expansion of macro 'strtomem_pad' 124 | strtomem_pad(message.str, msg, '\0'); | ^~~~~~~~~~~~ Use the smaller of the two buffer sizes when calling strnlen(). When src length is unknown (SIZE_MAX), it is adjusted to use dest length, which is what the original code did. Reported-by: Arnd Bergmann Fixes: dfbafa70bde2 ("string: Introduce strtomem() and strtomem_pad()") Tested-by: Arnd Bergmann Cc: Andy Shevchenko Cc: linux-hardening@vger.kernel.org Signed-off-by: Kees Cook --- include/linux/string.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/include/linux/string.h b/include/linux/string.h index dbfc66400050..9e3cb6923b0e 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -277,10 +277,12 @@ void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count, */ #define strtomem_pad(dest, src, pad) do { \ const size_t _dest_len = __builtin_object_size(dest, 1); \ + const size_t _src_len = __builtin_object_size(src, 1); \ \ BUILD_BUG_ON(!__builtin_constant_p(_dest_len) || \ _dest_len == (size_t)-1); \ - memcpy_and_pad(dest, _dest_len, src, strnlen(src, _dest_len), pad); \ + memcpy_and_pad(dest, _dest_len, src, \ + strnlen(src, min(_src_len, _dest_len)), pad); \ } while (0) /** @@ -298,10 +300,11 @@ void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count, */ #define strtomem(dest, src) do { \ const size_t _dest_len = __builtin_object_size(dest, 1); \ + const size_t _src_len = __builtin_object_size(src, 1); \ \ BUILD_BUG_ON(!__builtin_constant_p(_dest_len) || \ _dest_len == (size_t)-1); \ - memcpy(dest, src, min(_dest_len, strnlen(src, _dest_len))); \ + memcpy(dest, src, strnlen(src, min(_src_len, _dest_len))); \ } while (0) /** From 8d7af820310cb547c4cb7bdc321c161833eba57f Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Wed, 18 Oct 2023 17:46:20 -0700 Subject: [PATCH 64/73] MAINTAINERS: Include stackleak paths in hardening entry While most of the gcc-plugins are self-contained in the scripts/gcc-plugins directory, stackleak actually has some additional files. Add those so changes are directed to the hardening list. Suggested-by: Mark Rutland Acked-by: Mark Rutland Link: https://lore.kernel.org/r/20231019004616.work.960-kees@kernel.org Signed-off-by: Kees Cook --- MAINTAINERS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 5f18ed0fbd42..a4de29c0036f 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8615,6 +8615,8 @@ L: linux-hardening@vger.kernel.org S: Maintained T: git git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/hardening F: Documentation/kbuild/gcc-plugins.rst +F: include/linux/stackleak.h +F: kernel/stackleak.c F: scripts/Makefile.gcc-plugins F: scripts/gcc-plugins/ From 68a8f644575bde6b35c5fa8a3e6f024aa580e071 Mon Sep 17 00:00:00 2001 From: "Gustavo A. R. Silva" Date: Mon, 9 Oct 2023 13:52:45 -0600 Subject: [PATCH 65/73] ima: Add __counted_by for struct modsig and use struct_size() 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). Also, relocate `hdr->raw_pkcs7_len = sig_len;` so that the __counted_by annotation has effect, and flex-array member `raw_pkcs7` can be properly bounds-checked at run-time. While there, use struct_size() helper, instead of the open-coded version, to calculate the size for the allocation of the whole flexible structure, including of course, the flexible-array member. This code was found with the help of Coccinelle, and audited and fixed manually. Signed-off-by: "Gustavo A. R. Silva" Reviewed-by: Mimi Zohar Reviewed-by: Kees Cook Link: https://lore.kernel.org/r/ZSRaDcJNARUUWUwS@work Signed-off-by: Kees Cook --- security/integrity/ima/ima_modsig.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/security/integrity/ima/ima_modsig.c b/security/integrity/ima/ima_modsig.c index 3e7bee30080f..3265d744d5ce 100644 --- a/security/integrity/ima/ima_modsig.c +++ b/security/integrity/ima/ima_modsig.c @@ -29,7 +29,7 @@ struct modsig { * storing the signature. */ int raw_pkcs7_len; - u8 raw_pkcs7[]; + u8 raw_pkcs7[] __counted_by(raw_pkcs7_len); }; /* @@ -65,10 +65,11 @@ int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len, buf_len -= sig_len + sizeof(*sig); /* Allocate sig_len additional bytes to hold the raw PKCS#7 data. */ - hdr = kzalloc(sizeof(*hdr) + sig_len, GFP_KERNEL); + hdr = kzalloc(struct_size(hdr, raw_pkcs7, sig_len), GFP_KERNEL); if (!hdr) return -ENOMEM; + hdr->raw_pkcs7_len = sig_len; hdr->pkcs7_msg = pkcs7_parse_message(buf + buf_len, sig_len); if (IS_ERR(hdr->pkcs7_msg)) { rc = PTR_ERR(hdr->pkcs7_msg); @@ -77,7 +78,6 @@ int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len, } memcpy(hdr->raw_pkcs7, buf + buf_len, sig_len); - hdr->raw_pkcs7_len = sig_len; /* We don't know the hash algorithm yet. */ hdr->hash_algo = HASH_ALGO__LAST; From 31de287345f41bbfaec36a5c8cbdba035cf76442 Mon Sep 17 00:00:00 2001 From: Jarkko Sakkinen Date: Wed, 11 Oct 2023 02:08:25 +0300 Subject: [PATCH 66/73] KEYS: trusted: Rollback init_trusted() consistently Do bind neither static calls nor trusted_key_exit() before a successful init, in order to maintain a consistent state. In addition, depart the init_trusted() in the case of a real error (i.e. getting back something else than -ENODEV). Reported-by: Linus Torvalds Closes: https://lore.kernel.org/linux-integrity/CAHk-=whOPoLaWM8S8GgoOPT7a2+nMH5h3TLKtn=R_3w4R1_Uvg@mail.gmail.com/ Cc: stable@vger.kernel.org # v5.13+ Fixes: 5d0682be3189 ("KEYS: trusted: Add generic trusted keys framework") Signed-off-by: Jarkko Sakkinen --- security/keys/trusted-keys/trusted_core.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/security/keys/trusted-keys/trusted_core.c b/security/keys/trusted-keys/trusted_core.c index 85fb5c22529a..fee1ab2c734d 100644 --- a/security/keys/trusted-keys/trusted_core.c +++ b/security/keys/trusted-keys/trusted_core.c @@ -358,17 +358,17 @@ static int __init init_trusted(void) if (!get_random) get_random = kernel_get_random; - static_call_update(trusted_key_seal, - trusted_key_sources[i].ops->seal); - static_call_update(trusted_key_unseal, - trusted_key_sources[i].ops->unseal); - static_call_update(trusted_key_get_random, - get_random); - trusted_key_exit = trusted_key_sources[i].ops->exit; - migratable = trusted_key_sources[i].ops->migratable; - ret = trusted_key_sources[i].ops->init(); - if (!ret) + if (!ret) { + static_call_update(trusted_key_seal, trusted_key_sources[i].ops->seal); + static_call_update(trusted_key_unseal, trusted_key_sources[i].ops->unseal); + static_call_update(trusted_key_get_random, get_random); + + trusted_key_exit = trusted_key_sources[i].ops->exit; + migratable = trusted_key_sources[i].ops->migratable; + } + + if (!ret || ret != -ENODEV) break; } From c745cd1718b7825d69315fe7127e2e289e617598 Mon Sep 17 00:00:00 2001 From: Sumit Garg Date: Tue, 22 Aug 2023 16:59:33 +0530 Subject: [PATCH 67/73] KEYS: trusted: tee: Refactor register SHM usage The OP-TEE driver using the old SMC based ABI permits overlapping shared buffers, but with the new FF-A based ABI each physical page may only be registered once. As the key and blob buffer are allocated adjancently, there is no need for redundant register shared memory invocation. Also, it is incompatibile with FF-A based ABI limitation. So refactor register shared memory implementation to use only single invocation to register both key and blob buffers. [jarkko: Added cc to stable.] Cc: stable@vger.kernel.org # v5.16+ Fixes: 4615e5a34b95 ("optee: add FF-A support") Reported-by: Jens Wiklander Signed-off-by: Sumit Garg Tested-by: Jens Wiklander Reviewed-by: Jens Wiklander Signed-off-by: Jarkko Sakkinen --- security/keys/trusted-keys/trusted_tee.c | 64 ++++++++---------------- 1 file changed, 20 insertions(+), 44 deletions(-) diff --git a/security/keys/trusted-keys/trusted_tee.c b/security/keys/trusted-keys/trusted_tee.c index ac3e270ade69..aa3d477de6db 100644 --- a/security/keys/trusted-keys/trusted_tee.c +++ b/security/keys/trusted-keys/trusted_tee.c @@ -65,24 +65,16 @@ static int trusted_tee_seal(struct trusted_key_payload *p, char *datablob) int ret; struct tee_ioctl_invoke_arg inv_arg; struct tee_param param[4]; - struct tee_shm *reg_shm_in = NULL, *reg_shm_out = NULL; + struct tee_shm *reg_shm = NULL; memset(&inv_arg, 0, sizeof(inv_arg)); memset(¶m, 0, sizeof(param)); - reg_shm_in = tee_shm_register_kernel_buf(pvt_data.ctx, p->key, - p->key_len); - if (IS_ERR(reg_shm_in)) { - dev_err(pvt_data.dev, "key shm register failed\n"); - return PTR_ERR(reg_shm_in); - } - - reg_shm_out = tee_shm_register_kernel_buf(pvt_data.ctx, p->blob, - sizeof(p->blob)); - if (IS_ERR(reg_shm_out)) { - dev_err(pvt_data.dev, "blob shm register failed\n"); - ret = PTR_ERR(reg_shm_out); - goto out; + reg_shm = tee_shm_register_kernel_buf(pvt_data.ctx, p->key, + sizeof(p->key) + sizeof(p->blob)); + if (IS_ERR(reg_shm)) { + dev_err(pvt_data.dev, "shm register failed\n"); + return PTR_ERR(reg_shm); } inv_arg.func = TA_CMD_SEAL; @@ -90,13 +82,13 @@ static int trusted_tee_seal(struct trusted_key_payload *p, char *datablob) inv_arg.num_params = 4; param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT; - param[0].u.memref.shm = reg_shm_in; + param[0].u.memref.shm = reg_shm; param[0].u.memref.size = p->key_len; param[0].u.memref.shm_offs = 0; param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT; - param[1].u.memref.shm = reg_shm_out; + param[1].u.memref.shm = reg_shm; param[1].u.memref.size = sizeof(p->blob); - param[1].u.memref.shm_offs = 0; + param[1].u.memref.shm_offs = sizeof(p->key); ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param); if ((ret < 0) || (inv_arg.ret != 0)) { @@ -107,11 +99,7 @@ static int trusted_tee_seal(struct trusted_key_payload *p, char *datablob) p->blob_len = param[1].u.memref.size; } -out: - if (reg_shm_out) - tee_shm_free(reg_shm_out); - if (reg_shm_in) - tee_shm_free(reg_shm_in); + tee_shm_free(reg_shm); return ret; } @@ -124,24 +112,16 @@ static int trusted_tee_unseal(struct trusted_key_payload *p, char *datablob) int ret; struct tee_ioctl_invoke_arg inv_arg; struct tee_param param[4]; - struct tee_shm *reg_shm_in = NULL, *reg_shm_out = NULL; + struct tee_shm *reg_shm = NULL; memset(&inv_arg, 0, sizeof(inv_arg)); memset(¶m, 0, sizeof(param)); - reg_shm_in = tee_shm_register_kernel_buf(pvt_data.ctx, p->blob, - p->blob_len); - if (IS_ERR(reg_shm_in)) { - dev_err(pvt_data.dev, "blob shm register failed\n"); - return PTR_ERR(reg_shm_in); - } - - reg_shm_out = tee_shm_register_kernel_buf(pvt_data.ctx, p->key, - sizeof(p->key)); - if (IS_ERR(reg_shm_out)) { - dev_err(pvt_data.dev, "key shm register failed\n"); - ret = PTR_ERR(reg_shm_out); - goto out; + reg_shm = tee_shm_register_kernel_buf(pvt_data.ctx, p->key, + sizeof(p->key) + sizeof(p->blob)); + if (IS_ERR(reg_shm)) { + dev_err(pvt_data.dev, "shm register failed\n"); + return PTR_ERR(reg_shm); } inv_arg.func = TA_CMD_UNSEAL; @@ -149,11 +129,11 @@ static int trusted_tee_unseal(struct trusted_key_payload *p, char *datablob) inv_arg.num_params = 4; param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT; - param[0].u.memref.shm = reg_shm_in; + param[0].u.memref.shm = reg_shm; param[0].u.memref.size = p->blob_len; - param[0].u.memref.shm_offs = 0; + param[0].u.memref.shm_offs = sizeof(p->key); param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT; - param[1].u.memref.shm = reg_shm_out; + param[1].u.memref.shm = reg_shm; param[1].u.memref.size = sizeof(p->key); param[1].u.memref.shm_offs = 0; @@ -166,11 +146,7 @@ static int trusted_tee_unseal(struct trusted_key_payload *p, char *datablob) p->key_len = param[1].u.memref.size; } -out: - if (reg_shm_out) - tee_shm_free(reg_shm_out); - if (reg_shm_in) - tee_shm_free(reg_shm_in); + tee_shm_free(reg_shm); return ret; } From 3edc22655647378dea01900f7b04e017ff96bda9 Mon Sep 17 00:00:00 2001 From: Michal Suchanek Date: Thu, 7 Sep 2023 18:52:19 +0200 Subject: [PATCH 68/73] integrity: powerpc: Do not select CA_MACHINE_KEYRING No other platform needs CA_MACHINE_KEYRING, either. This is policy that should be decided by the administrator, not Kconfig dependencies. Cc: stable@vger.kernel.org # v6.6+ Fixes: d7d91c4743c4 ("integrity: PowerVM machine keyring enablement") Signed-off-by: Michal Suchanek Signed-off-by: Jarkko Sakkinen --- security/integrity/Kconfig | 2 -- 1 file changed, 2 deletions(-) diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig index 232191ee09e3..b6e074ac0227 100644 --- a/security/integrity/Kconfig +++ b/security/integrity/Kconfig @@ -68,8 +68,6 @@ config INTEGRITY_MACHINE_KEYRING depends on INTEGRITY_ASYMMETRIC_KEYS depends on SYSTEM_BLACKLIST_KEYRING depends on LOAD_UEFI_KEYS || LOAD_PPC_KEYS - select INTEGRITY_CA_MACHINE_KEYRING if LOAD_PPC_KEYS - select INTEGRITY_CA_MACHINE_KEYRING_MAX if LOAD_PPC_KEYS help If set, provide a keyring to which Machine Owner Keys (MOK) may be added. This keyring shall contain just MOK keys. Unlike keys From 03acb9ccec3f8cbcc0ed93c188b7a119ef30ef64 Mon Sep 17 00:00:00 2001 From: YueHaibing Date: Sat, 29 Jul 2023 10:40:26 +0800 Subject: [PATCH 69/73] keys: Remove unused extern declarations Since commit b2a4df200d57 ("KEYS: Expand the capacity of a keyring") iterate_over_keyring() is never used, so can be removed. And commit b5f545c880a2 ("[PATCH] keys: Permit running process to instantiate keys") left behind keyring_search_instkey(). Fixes: b2a4df200d57 ("KEYS: Expand the capacity of a keyring") Fixes: b5f545c880a2 ("[PATCH] keys: Permit running process to instantiate keys") Signed-off-by: YueHaibing Reviewed-by: Paul Moore Signed-off-by: Jarkko Sakkinen --- security/keys/internal.h | 7 ------- 1 file changed, 7 deletions(-) diff --git a/security/keys/internal.h b/security/keys/internal.h index 3c1e7122076b..471cf36dedc0 100644 --- a/security/keys/internal.h +++ b/security/keys/internal.h @@ -109,13 +109,6 @@ extern void __key_link_end(struct key *keyring, extern key_ref_t find_key_to_update(key_ref_t keyring_ref, const struct keyring_index_key *index_key); -extern struct key *keyring_search_instkey(struct key *keyring, - key_serial_t target_id); - -extern int iterate_over_keyring(const struct key *keyring, - int (*func)(const struct key *key, void *data), - void *data); - struct keyring_search_context { struct keyring_index_key index_key; const struct cred *cred; From bf5abc17bc434f27e20d3702b1992d9b5e4a7239 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 22 Sep 2023 10:51:15 -0700 Subject: [PATCH 70/73] virtio_console: Annotate struct port_buffer 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 port_buffer. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Amit Shah Cc: Arnd Bergmann Cc: Greg Kroah-Hartman Cc: virtualization@lists.linux-foundation.org Reviewed-by: "Gustavo A. R. Silva" Reviewed-by: Amit Shah Link: https://lore.kernel.org/r/20230922175115.work.059-kees@kernel.org Signed-off-by: Kees Cook --- drivers/char/virtio_console.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c index 680d1ef2a217..431e9e5bf9c1 100644 --- a/drivers/char/virtio_console.c +++ b/drivers/char/virtio_console.c @@ -106,7 +106,7 @@ struct port_buffer { unsigned int sgpages; /* sg is used if spages > 0. sg must be the last in is struct */ - struct scatterlist sg[]; + struct scatterlist sg[] __counted_by(sgpages); }; /* From 15fcedd43a0810a5482ef759a18ab852d012cead Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 22 Sep 2023 10:52:24 -0700 Subject: [PATCH 71/73] kexec: Annotate struct crash_mem 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 crash_mem. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Eric Biederman Cc: kexec@lists.infradead.org Acked-by: Baoquan He Link: https://lore.kernel.org/r/20230922175224.work.712-kees@kernel.org Signed-off-by: Kees Cook --- include/linux/crash_core.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h index 0c06561bf5ff..08704c29fdb4 100644 --- a/include/linux/crash_core.h +++ b/include/linux/crash_core.h @@ -92,7 +92,7 @@ int parse_crashkernel_low(char *cmdline, unsigned long long system_ram, struct crash_mem { unsigned int max_nr_ranges; unsigned int nr_ranges; - struct range ranges[]; + struct range ranges[] __counted_by(max_nr_ranges); }; extern int crash_exclude_mem_range(struct crash_mem *mem, From fed2ef7abaebe5e0207cd52ae52721ea3da3b5ba Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 22 Sep 2023 10:52:29 -0700 Subject: [PATCH 72/73] reset: Annotate struct reset_control_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 reset_control_array. Additionally, since the element count member must be set before accessing the annotated flexible array member, move its initialization earlier. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Philipp Zabel Reviewed-by: "Gustavo A. R. Silva" Link: https://lore.kernel.org/r/20230922175229.work.838-kees@kernel.org Signed-off-by: Kees Cook --- drivers/reset/core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/reset/core.c b/drivers/reset/core.c index f0a076e94118..7ece6a8e9858 100644 --- a/drivers/reset/core.c +++ b/drivers/reset/core.c @@ -60,7 +60,7 @@ struct reset_control { struct reset_control_array { struct reset_control base; unsigned int num_rstcs; - struct reset_control *rstc[]; + struct reset_control *rstc[] __counted_by(num_rstcs); }; static const char *rcdev_name(struct reset_controller_dev *rcdev) @@ -1185,6 +1185,7 @@ of_reset_control_array_get(struct device_node *np, bool shared, bool optional, resets = kzalloc(struct_size(resets, rstc, num), GFP_KERNEL); if (!resets) return ERR_PTR(-ENOMEM); + resets->num_rstcs = num; for (i = 0; i < num; i++) { rstc = __of_reset_control_get(np, NULL, i, shared, optional, @@ -1193,7 +1194,6 @@ of_reset_control_array_get(struct device_node *np, bool shared, bool optional, goto err_rst; resets->rstc[i] = rstc; } - resets->num_rstcs = num; resets->base.array = true; return &resets->base; From 9cca73d7b4bfec75b2fcef751015f31691afa792 Mon Sep 17 00:00:00 2001 From: Justin Stitt Date: Tue, 26 Sep 2023 06:59:15 +0000 Subject: [PATCH 73/73] hwmon: (acpi_power_meter) replace open-coded kmemdup_nul `strncpy` is deprecated for use on NUL-terminated destination strings [1]. Let's refactor this kcalloc() + strncpy() into a kmemdup_nul() which has more obvious behavior and is less error prone. 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-hardening@vger.kernel.org Signed-off-by: Justin Stitt Reviewed-by: Kees Cook Link: https://lore.kernel.org/r/20230926-strncpy-drivers-hwmon-acpi_power_meter-c-v5-1-3fc31a9daf99@google.com Signed-off-by: Kees Cook --- drivers/hwmon/acpi_power_meter.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c index fa28d447f0df..8db740214ffd 100644 --- a/drivers/hwmon/acpi_power_meter.c +++ b/drivers/hwmon/acpi_power_meter.c @@ -796,14 +796,13 @@ static int read_capabilities(struct acpi_power_meter_resource *resource) goto error; } - *str = kcalloc(element->string.length + 1, sizeof(u8), - GFP_KERNEL); + *str = kmemdup_nul(element->string.pointer, element->string.length, + GFP_KERNEL); if (!*str) { res = -ENOMEM; goto error; } - strncpy(*str, element->string.pointer, element->string.length); str++; }