From b2ba00c2a51793d916b662cb049b8f01c55d9e82 Mon Sep 17 00:00:00 2001 From: Stephen Rothwell Date: Thu, 5 Jan 2023 12:05:50 -0800 Subject: [PATCH 01/19] rxrpc: replace zero-lenth array with DECLARE_FLEX_ARRAY() helper 0-length arrays are deprecated, and cause problems with bounds checking. Replace with a flexible array: In file included from include/linux/string.h:253, from include/linux/bitmap.h:11, from include/linux/cpumask.h:12, from arch/x86/include/asm/paravirt.h:17, from arch/x86/include/asm/cpuid.h:62, from arch/x86/include/asm/processor.h:19, from arch/x86/include/asm/cpufeature.h:5, from arch/x86/include/asm/thread_info.h:53, from include/linux/thread_info.h:60, from arch/x86/include/asm/preempt.h:9, from include/linux/preempt.h:78, from include/linux/percpu.h:6, from include/linux/prandom.h:13, from include/linux/random.h:153, from include/linux/net.h:18, from net/rxrpc/output.c:10: In function 'fortify_memcpy_chk', inlined from 'rxrpc_fill_out_ack' at net/rxrpc/output.c:158:2: include/linux/fortify-string.h:520:25: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning] 520 | __write_overflow_field(p_size_field, size); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Link: https://lore.kernel.org/linux-next/20230105132535.0d65378f@canb.auug.org.au/ Cc: David Howells Cc: Marc Dionne Cc: "David S. Miller" Cc: Eric Dumazet Cc: Jakub Kicinski Cc: Paolo Abeni Cc: linux-afs@lists.infradead.org Cc: netdev@vger.kernel.org Signed-off-by: Stephen Rothwell Signed-off-by: Kees Cook --- net/rxrpc/ar-internal.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h index 18092526d3c8..c7186484fc5f 100644 --- a/net/rxrpc/ar-internal.h +++ b/net/rxrpc/ar-internal.h @@ -783,7 +783,7 @@ struct rxrpc_txbuf { u8 data[RXRPC_JUMBO_DATALEN]; /* Data packet */ struct { struct rxrpc_ackpacket ack; - u8 acks[0]; + DECLARE_FLEX_ARRAY(u8, acks); }; }; } __aligned(64); From 439a1bcac648fe9b59210cde8991fb2acf37bdab Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Mon, 19 Sep 2022 15:53:13 -0700 Subject: [PATCH 02/19] fortify: Use __builtin_dynamic_object_size() when available Since the commits starting with c37495d6254c ("slab: add __alloc_size attributes for better bounds checking"), the compilers have runtime allocation size hints available in some places. This was immediately available to CONFIG_UBSAN_BOUNDS, but CONFIG_FORTIFY_SOURCE needed updating to explicitly make use of the hints via the associated __builtin_dynamic_object_size() helper. Detect and use the builtin when it is available, increasing the accuracy of the mitigation. When runtime sizes are not available, __builtin_dynamic_object_size() falls back to __builtin_object_size(), leaving the existing bounds checking unchanged. Additionally update the VMALLOC_LINEAR_OVERFLOW LKDTM test to make the hint invisible, otherwise the architectural defense is not exercised (the buffer overflow is detected in the memset() rather than when it crosses the edge of the allocation). Cc: Arnd Bergmann Cc: Greg Kroah-Hartman Cc: Nick Desaulniers Cc: Nathan Chancellor Cc: Tom Rix Cc: linux-hardening@vger.kernel.org Cc: llvm@lists.linux.dev Reviewed-by: Miguel Ojeda # include/linux/compiler_attributes.h Signed-off-by: Kees Cook --- drivers/misc/lkdtm/heap.c | 1 + include/linux/compiler_attributes.h | 5 +++++ include/linux/fortify-string.h | 7 +++++++ 3 files changed, 13 insertions(+) diff --git a/drivers/misc/lkdtm/heap.c b/drivers/misc/lkdtm/heap.c index 62516078a619..0ce4cbf6abda 100644 --- a/drivers/misc/lkdtm/heap.c +++ b/drivers/misc/lkdtm/heap.c @@ -31,6 +31,7 @@ static void lkdtm_VMALLOC_LINEAR_OVERFLOW(void) char *one, *two; one = vzalloc(PAGE_SIZE); + OPTIMIZER_HIDE_VAR(one); two = vzalloc(PAGE_SIZE); pr_info("Attempting vmalloc linear overflow ...\n"); diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h index 898b3458b24a..56467f86a27c 100644 --- a/include/linux/compiler_attributes.h +++ b/include/linux/compiler_attributes.h @@ -297,6 +297,11 @@ * * clang: https://clang.llvm.org/docs/AttributeReference.html#pass-object-size-pass-dynamic-object-size */ +#if __has_attribute(__pass_dynamic_object_size__) +# define __pass_dynamic_object_size(type) __attribute__((__pass_dynamic_object_size__(type))) +#else +# define __pass_dynamic_object_size(type) +#endif #if __has_attribute(__pass_object_size__) # define __pass_object_size(type) __attribute__((__pass_object_size__(type))) #else diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h index 7cad8bb031e9..c9de1f59ee80 100644 --- a/include/linux/fortify-string.h +++ b/include/linux/fortify-string.h @@ -90,10 +90,17 @@ extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size) * size, rather than struct size), but there remain some stragglers using * type 0 that will be converted in the future. */ +#if __has_builtin(__builtin_dynamic_object_size) +#define POS __pass_dynamic_object_size(1) +#define POS0 __pass_dynamic_object_size(0) +#define __struct_size(p) __builtin_dynamic_object_size(p, 0) +#define __member_size(p) __builtin_dynamic_object_size(p, 1) +#else #define POS __pass_object_size(1) #define POS0 __pass_object_size(0) #define __struct_size(p) __builtin_object_size(p, 0) #define __member_size(p) __builtin_object_size(p, 1) +#endif #define __compiletime_lessthan(bounds, length) ( \ __builtin_constant_p((bounds) < (length)) && \ From b5fc3ca3954fd0382bb576a5a295c03a089ac7e3 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Thu, 5 Jan 2023 13:57:10 -0800 Subject: [PATCH 03/19] ARM: ixp4xx: Replace 0-length arrays with flexible arrays Zero-length arrays are deprecated[1]. Replace npe_load_firmware's union of 0-length arrays with flexible arrays. Detected with GCC 13, using -fstrict-flex-arrays=3: drivers/soc/ixp4xx/ixp4xx-npe.c: In function 'npe_load_firmware': drivers/soc/ixp4xx/ixp4xx-npe.c:570:60: warning: array subscript i is outside array bounds of 'u32[0]' {aka 'unsigned int[]'} [-Warray-bounds=] 570 | image->data[i] = swab32(image->data[i]); include/uapi/linux/swab.h:115:54: note: in definition of macro '__swab32' 115 | #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x)) | ^ drivers/soc/ixp4xx/ixp4xx-npe.c:570:42: note: in expansion of macro 'swab32' 570 | image->data[i] = swab32(image->data[i]); | ^~~~~~ drivers/soc/ixp4xx/ixp4xx-npe.c:522:29: note: while referencing 'data' 522 | u32 data[0]; | ^~~~ [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays Cc: Krzysztof Halasa Cc: Linus Walleij Cc: Arnd Bergmann Cc: "Gustavo A. R. Silva" Cc: linux-arm-kernel@lists.infradead.org Signed-off-by: Kees Cook Reviewed-by: Linus Walleij Link: https://lore.kernel.org/r/20230105215706.never.027-kees@kernel.org --- drivers/soc/ixp4xx/ixp4xx-npe.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/soc/ixp4xx/ixp4xx-npe.c b/drivers/soc/ixp4xx/ixp4xx-npe.c index 58240e320c13..5be9988f30ce 100644 --- a/drivers/soc/ixp4xx/ixp4xx-npe.c +++ b/drivers/soc/ixp4xx/ixp4xx-npe.c @@ -519,15 +519,15 @@ int npe_load_firmware(struct npe *npe, const char *name, struct device *dev) u32 id; u32 size; union { - u32 data[0]; - struct dl_block blocks[0]; + DECLARE_FLEX_ARRAY(u32, data); + DECLARE_FLEX_ARRAY(struct dl_block, blocks); }; } *image; struct dl_codeblock { u32 npe_addr; u32 size; - u32 data[0]; + u32 data[]; } *cb; int i, j, err, data_size, instr_size, blocks, table_end; From b76ded214633cf5067ff51642a360eb87242c411 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 9 Dec 2022 11:57:42 -0800 Subject: [PATCH 04/19] LoadPin: Refactor read-only check into a helper In preparation for allowing mounts to shift when not enforced, move read-only checking into a separate helper. Cc: Paul Moore Cc: James Morris Cc: "Serge E. Hallyn" Cc: linux-security-module@vger.kernel.org Signed-off-by: Kees Cook Reviewed-by: Serge Hallyn Link: https://lore.kernel.org/r/20221209195746.1366607-1-keescook@chromium.org --- security/loadpin/loadpin.c | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c index 110a5ab2b46b..ca0eff3ce9d0 100644 --- a/security/loadpin/loadpin.c +++ b/security/loadpin/loadpin.c @@ -72,28 +72,21 @@ static struct ctl_table loadpin_sysctl_table[] = { { } }; -/* - * This must be called after early kernel init, since then the rootdev - * is available. - */ -static void check_pinning_enforcement(struct super_block *mnt_sb) +static void report_writable(struct super_block *mnt_sb, bool writable) { - bool ro = false; - /* * If load pinning is not enforced via a read-only block * device, allow sysctl to change modes for testing. */ if (mnt_sb->s_bdev) { - ro = bdev_read_only(mnt_sb->s_bdev); pr_info("%pg (%u:%u): %s\n", mnt_sb->s_bdev, MAJOR(mnt_sb->s_bdev->bd_dev), MINOR(mnt_sb->s_bdev->bd_dev), - ro ? "read-only" : "writable"); + writable ? "writable" : "read-only"); } else pr_info("mnt_sb lacks block device, treating as: writable\n"); - if (!ro) { + if (writable) { if (!register_sysctl_paths(loadpin_sysctl_path, loadpin_sysctl_table)) pr_notice("sysctl registration failed!\n"); @@ -103,12 +96,26 @@ static void check_pinning_enforcement(struct super_block *mnt_sb) pr_info("load pinning engaged.\n"); } #else -static void check_pinning_enforcement(struct super_block *mnt_sb) +static void report_writable(struct super_block *mnt_sb, bool writable) { pr_info("load pinning engaged.\n"); } #endif +/* + * This must be called after early kernel init, since then the rootdev + * is available. + */ +static bool sb_is_writable(struct super_block *mnt_sb) +{ + bool writable = true; + + if (mnt_sb->s_bdev) + writable = !bdev_read_only(mnt_sb->s_bdev); + + return writable; +} + static void loadpin_sb_free_security(struct super_block *mnt_sb) { /* @@ -126,6 +133,7 @@ static int loadpin_check(struct file *file, enum kernel_read_file_id id) { struct super_block *load_root; const char *origin = kernel_read_file_id_str(id); + bool load_root_writable; /* If the file id is excluded, ignore the pinning. */ if ((unsigned int)id < ARRAY_SIZE(ignore_read_file_id) && @@ -146,6 +154,7 @@ static int loadpin_check(struct file *file, enum kernel_read_file_id id) } load_root = file->f_path.mnt->mnt_sb; + load_root_writable = sb_is_writable(load_root); /* First loaded module/firmware defines the root for all others. */ spin_lock(&pinned_root_spinlock); @@ -162,7 +171,7 @@ static int loadpin_check(struct file *file, enum kernel_read_file_id id) * enforcing. This would be purely cosmetic. */ spin_unlock(&pinned_root_spinlock); - check_pinning_enforcement(pinned_root); + report_writable(pinned_root, load_root_writable); report_load(origin, file, "pinned"); } else { spin_unlock(&pinned_root_spinlock); From 60ba1028fc7b73e3cfbcfe7087a2e87e8b1fd208 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 9 Dec 2022 11:57:43 -0800 Subject: [PATCH 05/19] LoadPin: Refactor sysctl initialization In preparation for shifting root mount when not enforcing, split sysctl logic out into a separate helper, and unconditionally register the sysctl, but only make it writable when the device is writable. Cc: Paul Moore Cc: James Morris Cc: "Serge E. Hallyn" Cc: linux-security-module@vger.kernel.org Signed-off-by: Kees Cook Reviewed-by: Serge Hallyn Link: https://lore.kernel.org/r/20221209195746.1366607-2-keescook@chromium.org --- security/loadpin/loadpin.c | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c index ca0eff3ce9d0..5b15f8f7268d 100644 --- a/security/loadpin/loadpin.c +++ b/security/loadpin/loadpin.c @@ -52,7 +52,6 @@ static bool deny_reading_verity_digests; #endif #ifdef CONFIG_SYSCTL - static struct ctl_path loadpin_sysctl_path[] = { { .procname = "kernel", }, { .procname = "loadpin", }, @@ -66,18 +65,29 @@ static struct ctl_table loadpin_sysctl_table[] = { .maxlen = sizeof(int), .mode = 0644, .proc_handler = proc_dointvec_minmax, - .extra1 = SYSCTL_ZERO, + .extra1 = SYSCTL_ONE, .extra2 = SYSCTL_ONE, }, { } }; -static void report_writable(struct super_block *mnt_sb, bool writable) +static void set_sysctl(bool is_writable) { /* * If load pinning is not enforced via a read-only block * device, allow sysctl to change modes for testing. */ + if (is_writable) + loadpin_sysctl_table[0].extra1 = SYSCTL_ZERO; + else + loadpin_sysctl_table[0].extra1 = SYSCTL_ONE; +} +#else +static inline void set_sysctl(bool is_writable) { } +#endif + +static void report_writable(struct super_block *mnt_sb, bool writable) +{ if (mnt_sb->s_bdev) { pr_info("%pg (%u:%u): %s\n", mnt_sb->s_bdev, MAJOR(mnt_sb->s_bdev->bd_dev), @@ -86,21 +96,9 @@ static void report_writable(struct super_block *mnt_sb, bool writable) } else pr_info("mnt_sb lacks block device, treating as: writable\n"); - if (writable) { - if (!register_sysctl_paths(loadpin_sysctl_path, - loadpin_sysctl_table)) - pr_notice("sysctl registration failed!\n"); - else - pr_info("enforcement can be disabled.\n"); - } else + if (!writable) pr_info("load pinning engaged.\n"); } -#else -static void report_writable(struct super_block *mnt_sb, bool writable) -{ - pr_info("load pinning engaged.\n"); -} -#endif /* * This must be called after early kernel init, since then the rootdev @@ -172,6 +170,7 @@ static int loadpin_check(struct file *file, enum kernel_read_file_id id) */ spin_unlock(&pinned_root_spinlock); report_writable(pinned_root, load_root_writable); + set_sysctl(load_root_writable); report_load(origin, file, "pinned"); } else { spin_unlock(&pinned_root_spinlock); @@ -259,6 +258,10 @@ static int __init loadpin_init(void) pr_info("ready to pin (currently %senforcing)\n", enforce ? "" : "not "); parse_exclude(); +#ifdef CONFIG_SYSCTL + if (!register_sysctl_paths(loadpin_sysctl_path, loadpin_sysctl_table)) + pr_notice("sysctl registration failed!\n"); +#endif security_add_hooks(loadpin_hooks, ARRAY_SIZE(loadpin_hooks), "loadpin"); return 0; From 2cfaa84efc25e52f116507a2e69781a40c4dda41 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 9 Dec 2022 11:57:44 -0800 Subject: [PATCH 06/19] LoadPin: Move pin reporting cleanly out of locking Refactor the pin reporting to be more cleanly outside the locking. It was already, but moving it around helps clear the path for the root to switch when not enforcing. Cc: Paul Moore Cc: James Morris Cc: "Serge E. Hallyn" Cc: linux-security-module@vger.kernel.org Signed-off-by: Kees Cook Reviewed-by: Serge Hallyn Link: https://lore.kernel.org/r/20221209195746.1366607-3-keescook@chromium.org --- security/loadpin/loadpin.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c index 5b15f8f7268d..ef12d77548ae 100644 --- a/security/loadpin/loadpin.c +++ b/security/loadpin/loadpin.c @@ -131,6 +131,7 @@ static int loadpin_check(struct file *file, enum kernel_read_file_id id) { struct super_block *load_root; const char *origin = kernel_read_file_id_str(id); + bool first_root_pin = false; bool load_root_writable; /* If the file id is excluded, ignore the pinning. */ @@ -162,18 +163,14 @@ static int loadpin_check(struct file *file, enum kernel_read_file_id id) */ if (!pinned_root) { pinned_root = load_root; - /* - * Unlock now since it's only pinned_root we care about. - * In the worst case, we will (correctly) report pinning - * failures before we have announced that pinning is - * enforcing. This would be purely cosmetic. - */ - spin_unlock(&pinned_root_spinlock); + first_root_pin = true; + } + spin_unlock(&pinned_root_spinlock); + + if (first_root_pin) { report_writable(pinned_root, load_root_writable); set_sysctl(load_root_writable); report_load(origin, file, "pinned"); - } else { - spin_unlock(&pinned_root_spinlock); } if (IS_ERR_OR_NULL(pinned_root) || From eba773596be9c21a8e979d7e653f721d1d0341a9 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 9 Dec 2022 11:57:45 -0800 Subject: [PATCH 07/19] LoadPin: Allow filesystem switch when not enforcing For LoadPin to be used at all in a classic distro environment, it needs to allow for switching filesystems (from the initramfs to the "real" root filesystem). To allow for this, if the "enforce" mode is not set at boot, reset the pinned filesystem tracking when the pinned filesystem gets unmounted instead of invalidating further loads. Once enforcement is set, it cannot be unset, and the pinning will stick. This means that distros can build with CONFIG_SECURITY_LOADPIN=y, but with CONFIG_SECURITY_LOADPIN_ENFORCE disabled, but after boot is running, the system can enable enforcement: $ sysctl -w kernel.loadpin.enforced=1 Cc: Paul Moore Cc: James Morris Cc: "Serge E. Hallyn" Cc: linux-security-module@vger.kernel.org Signed-off-by: Kees Cook Reviewed-by: Serge Hallyn Link: https://lore.kernel.org/r/20221209195746.1366607-4-keescook@chromium.org --- security/loadpin/loadpin.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c index ef12d77548ae..d73a281adf86 100644 --- a/security/loadpin/loadpin.c +++ b/security/loadpin/loadpin.c @@ -119,11 +119,16 @@ static void loadpin_sb_free_security(struct super_block *mnt_sb) /* * When unmounting the filesystem we were using for load * pinning, we acknowledge the superblock release, but make sure - * no other modules or firmware can be loaded. + * no other modules or firmware can be loaded when we are in + * enforcing mode. Otherwise, allow the root to be reestablished. */ if (!IS_ERR_OR_NULL(pinned_root) && mnt_sb == pinned_root) { - pinned_root = ERR_PTR(-EIO); - pr_info("umount pinned fs: refusing further loads\n"); + if (enforce) { + pinned_root = ERR_PTR(-EIO); + pr_info("umount pinned fs: refusing further loads\n"); + } else { + pinned_root = NULL; + } } } @@ -158,8 +163,9 @@ static int loadpin_check(struct file *file, enum kernel_read_file_id id) /* First loaded module/firmware defines the root for all others. */ spin_lock(&pinned_root_spinlock); /* - * pinned_root is only NULL at startup. Otherwise, it is either - * a valid reference, or an ERR_PTR. + * pinned_root is only NULL at startup or when the pinned root has + * been unmounted while we are not in enforcing mode. Otherwise, it + * is either a valid reference, or an ERR_PTR. */ if (!pinned_root) { pinned_root = load_root; From 4076ea2419cf15bc1e1580f8b24ddf675fbdb02c Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Sun, 27 Nov 2022 10:30:41 -0800 Subject: [PATCH 08/19] drm/nouveau/disp: Fix nvif_outp_acquire_dp() argument size Both Coverity and GCC with -Wstringop-overflow noticed that nvif_outp_acquire_dp() accidentally defined its second argument with 1 additional element: drivers/gpu/drm/nouveau/dispnv50/disp.c: In function 'nv50_pior_atomic_enable': drivers/gpu/drm/nouveau/dispnv50/disp.c:1813:17: error: 'nvif_outp_acquire_dp' accessing 16 bytes in a region of size 15 [-Werror=stringop-overflow=] 1813 | nvif_outp_acquire_dp(&nv_encoder->outp, nv_encoder->dp.dpcd, 0, 0, false, false); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/gpu/drm/nouveau/dispnv50/disp.c:1813:17: note: referencing argument 2 of type 'u8[16]' {aka 'unsigned char[16]'} drivers/gpu/drm/nouveau/include/nvif/outp.h:24:5: note: in a call to function 'nvif_outp_acquire_dp' 24 | int nvif_outp_acquire_dp(struct nvif_outp *, u8 dpcd[16], | ^~~~~~~~~~~~~~~~~~~~ Avoid these warnings by defining the argument size using the matching define (DP_RECEIVER_CAP_SIZE, 15) instead of having it be a literal (and incorrect) value (16). Reported-by: coverity-bot Addresses-Coverity-ID: 1527269 ("Memory - corruptions") Addresses-Coverity-ID: 1527268 ("Memory - corruptions") Link: https://lore.kernel.org/lkml/202211100848.FFBA2432@keescook/ Link: https://lore.kernel.org/lkml/202211100848.F4C2819BB@keescook/ Fixes: 813443721331 ("drm/nouveau/disp: move DP link config into acquire") Reviewed-by: Lyude Paul Cc: Ben Skeggs Cc: Karol Herbst Cc: David Airlie Cc: Daniel Vetter Cc: Dave Airlie Cc: "Gustavo A. R. Silva" Cc: dri-devel@lists.freedesktop.org Cc: nouveau@lists.freedesktop.org Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20221127183036.never.139-kees@kernel.org --- drivers/gpu/drm/nouveau/include/nvif/outp.h | 3 ++- drivers/gpu/drm/nouveau/nvif/outp.c | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/nouveau/include/nvif/outp.h b/drivers/gpu/drm/nouveau/include/nvif/outp.h index 45daadec3c0c..fa76a7b5e4b3 100644 --- a/drivers/gpu/drm/nouveau/include/nvif/outp.h +++ b/drivers/gpu/drm/nouveau/include/nvif/outp.h @@ -3,6 +3,7 @@ #define __NVIF_OUTP_H__ #include #include +#include struct nvif_disp; struct nvif_outp { @@ -21,7 +22,7 @@ int nvif_outp_acquire_rgb_crt(struct nvif_outp *); int nvif_outp_acquire_tmds(struct nvif_outp *, int head, bool hdmi, u8 max_ac_packet, u8 rekey, u8 scdc, bool hda); int nvif_outp_acquire_lvds(struct nvif_outp *, bool dual, bool bpc8); -int nvif_outp_acquire_dp(struct nvif_outp *, u8 dpcd[16], +int nvif_outp_acquire_dp(struct nvif_outp *outp, u8 dpcd[DP_RECEIVER_CAP_SIZE], int link_nr, int link_bw, bool hda, bool mst); void nvif_outp_release(struct nvif_outp *); int nvif_outp_infoframe(struct nvif_outp *, u8 type, struct nvif_outp_infoframe_v0 *, u32 size); diff --git a/drivers/gpu/drm/nouveau/nvif/outp.c b/drivers/gpu/drm/nouveau/nvif/outp.c index 7da39f1eae9f..c24bc5eae3ec 100644 --- a/drivers/gpu/drm/nouveau/nvif/outp.c +++ b/drivers/gpu/drm/nouveau/nvif/outp.c @@ -127,7 +127,7 @@ nvif_outp_acquire(struct nvif_outp *outp, u8 proto, struct nvif_outp_acquire_v0 } int -nvif_outp_acquire_dp(struct nvif_outp *outp, u8 dpcd[16], +nvif_outp_acquire_dp(struct nvif_outp *outp, u8 dpcd[DP_RECEIVER_CAP_SIZE], int link_nr, int link_bw, bool hda, bool mst) { struct nvif_outp_acquire_v0 args; From 16a738f2f6b31f9edf0691e0f357539f7ac66662 Mon Sep 17 00:00:00 2001 From: Paulo Miguel Almeida Date: Tue, 20 Dec 2022 16:41:15 +1300 Subject: [PATCH 09/19] i915/gvt: Replace one-element array with flexible-array member One-element arrays are deprecated, and we are replacing them with flexible array members instead. So, replace one-element array with flexible-array member in struct gvt_firmware_header and refactor the rest of the code accordingly. Additionally, previous implementation was allocating 8 bytes more than required to represent firmware_header + cfg_space data + mmio data. This helps with the ongoing efforts to tighten the FORTIFY_SOURCE routines on memcpy() and help us make progress towards globally enabling -fstrict-flex-arrays=3 [1]. To make reviewing this patch easier, I'm pasting before/after struct sizes. pahole -C gvt_firmware_header before/drivers/gpu/drm/i915/gvt/firmware.o struct gvt_firmware_header { u64 magic; /* 0 8 */ u32 crc32; /* 8 4 */ u32 version; /* 12 4 */ u64 cfg_space_size; /* 16 8 */ u64 cfg_space_offset; /* 24 8 */ u64 mmio_size; /* 32 8 */ u64 mmio_offset; /* 40 8 */ unsigned char data[1]; /* 48 1 */ /* size: 56, cachelines: 1, members: 8 */ /* padding: 7 */ /* last cacheline: 56 bytes */ }; pahole -C gvt_firmware_header after/drivers/gpu/drm/i915/gvt/firmware.o struct gvt_firmware_header { u64 magic; /* 0 8 */ u32 crc32; /* 8 4 */ u32 version; /* 12 4 */ u64 cfg_space_size; /* 16 8 */ u64 cfg_space_offset; /* 24 8 */ u64 mmio_size; /* 32 8 */ u64 mmio_offset; /* 40 8 */ unsigned char data[]; /* 48 0 */ /* size: 48, cachelines: 1, members: 8 */ /* last cacheline: 48 bytes */ }; As you can see the additional byte of the fake-flexible array (data[1]) forced the compiler to pad the struct but those bytes aren't actually used as first & last bytes (of both cfg_space and mmio) are controlled by the <>_size and <>_offset members present in the gvt_firmware_header struct. Link: https://github.com/KSPP/linux/issues/79 Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836 [1] Signed-off-by: Paulo Miguel Almeida Reviewed-by: Zhenyu Wang Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/Y6Eu2604cqtryP4g@mail.google.com --- drivers/gpu/drm/i915/gvt/firmware.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/gvt/firmware.c b/drivers/gpu/drm/i915/gvt/firmware.c index a683c22d5b64..dce93738e98a 100644 --- a/drivers/gpu/drm/i915/gvt/firmware.c +++ b/drivers/gpu/drm/i915/gvt/firmware.c @@ -45,7 +45,7 @@ struct gvt_firmware_header { u64 cfg_space_offset; /* offset in the file */ u64 mmio_size; u64 mmio_offset; /* offset in the file */ - unsigned char data[1]; + unsigned char data[]; }; #define dev_to_drm_minor(d) dev_get_drvdata((d)) @@ -77,7 +77,7 @@ static int expose_firmware_sysfs(struct intel_gvt *gvt) unsigned long size, crc32_start; int ret; - size = sizeof(*h) + info->mmio_size + info->cfg_space_size; + size = offsetof(struct gvt_firmware_header, data) + info->mmio_size + info->cfg_space_size; firmware = vzalloc(size); if (!firmware) return -ENOMEM; From 118901ad1f25d2334255b3d50512fa20591531cd Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Wed, 4 Jan 2023 13:09:12 -0800 Subject: [PATCH 10/19] ext4: Fix function prototype mismatch for ext4_feat_ktype With clang's kernel control flow integrity (kCFI, CONFIG_CFI_CLANG), indirect call targets are validated against the expected function pointer prototype to make sure the call target is valid to help mitigate ROP attacks. If they are not identical, there is a failure at run time, which manifests as either a kernel panic or thread getting killed. ext4_feat_ktype was setting the "release" handler to "kfree", which doesn't have a matching function prototype. Add a simple wrapper with the correct prototype. This was found as a result of Clang's new -Wcast-function-type-strict flag, which is more sensitive than the simpler -Wcast-function-type, which only checks for type width mismatches. Note that this code is only reached when ext4 is a loadable module and it is being unloaded: CFI failure at kobject_put+0xbb/0x1b0 (target: kfree+0x0/0x180; expected type: 0x7c4aa698) ... RIP: 0010:kobject_put+0xbb/0x1b0 ... Call Trace: ext4_exit_sysfs+0x14/0x60 [ext4] cleanup_module+0x67/0xedb [ext4] Fixes: b99fee58a20a ("ext4: create ext4_feat kobject dynamically") Cc: Theodore Ts'o Cc: Eric Biggers Cc: stable@vger.kernel.org Build-tested-by: Gustavo A. R. Silva Reviewed-by: Gustavo A. R. Silva Reviewed-by: Nathan Chancellor Link: https://lore.kernel.org/r/20230103234616.never.915-kees@kernel.org Signed-off-by: Kees Cook Reviewed-by: Eric Biggers Link: https://lore.kernel.org/r/20230104210908.gonna.388-kees@kernel.org --- fs/ext4/sysfs.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c index d233c24ea342..e2b8b3437c58 100644 --- a/fs/ext4/sysfs.c +++ b/fs/ext4/sysfs.c @@ -491,6 +491,11 @@ static void ext4_sb_release(struct kobject *kobj) complete(&sbi->s_kobj_unregister); } +static void ext4_feat_release(struct kobject *kobj) +{ + kfree(kobj); +} + static const struct sysfs_ops ext4_attr_ops = { .show = ext4_attr_show, .store = ext4_attr_store, @@ -505,7 +510,7 @@ static struct kobj_type ext4_sb_ktype = { static struct kobj_type ext4_feat_ktype = { .default_groups = ext4_feat_groups, .sysfs_ops = &ext4_attr_ops, - .release = (void (*)(struct kobject *))kfree, + .release = ext4_feat_release, }; void ext4_notify_error_sysfs(struct ext4_sb_info *sbi) From 36632d062975a9ff4410c90dd6d37922b68d0920 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Thu, 5 Jan 2023 11:05:11 -0800 Subject: [PATCH 11/19] io_uring: Replace 0-length array with flexible array Zero-length arrays are deprecated[1]. Replace struct io_uring_buf_ring's "bufs" with a flexible array member. (How is the size of this array verified?) Detected with GCC 13, using -fstrict-flex-arrays=3: In function 'io_ring_buffer_select', inlined from 'io_buffer_select' at io_uring/kbuf.c:183:10: io_uring/kbuf.c:141:23: warning: array subscript 255 is outside the bounds of an interior zero-length array 'struct io_uring_buf[0]' [-Wzero-length-bounds] 141 | buf = &br->bufs[head]; | ^~~~~~~~~~~~~~~ In file included from include/linux/io_uring.h:7, from io_uring/kbuf.c:10: include/uapi/linux/io_uring.h: In function 'io_buffer_select': include/uapi/linux/io_uring.h:628:41: note: while referencing 'bufs' 628 | struct io_uring_buf bufs[0]; | ^~~~ [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays Fixes: c7fb19428d67 ("io_uring: add support for ring mapped supplied buffers") Cc: Jens Axboe Cc: Pavel Begunkov Cc: "Gustavo A. R. Silva" Cc: stable@vger.kernel.org Cc: io-uring@vger.kernel.org Signed-off-by: Kees Cook Reviewed-by: Gustavo A. R. Silva Link: https://lore.kernel.org/r/20230105190507.gonna.131-kees@kernel.org --- include/uapi/linux/io_uring.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h index 2780bce62faf..434f62e0fb72 100644 --- a/include/uapi/linux/io_uring.h +++ b/include/uapi/linux/io_uring.h @@ -625,7 +625,7 @@ struct io_uring_buf_ring { __u16 resv3; __u16 tail; }; - struct io_uring_buf bufs[0]; + __DECLARE_FLEX_ARRAY(struct io_uring_buf, bufs); }; }; From 8500689095a39a8c245ba62709f5e8735668e535 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Thu, 5 Jan 2023 15:46:01 -0800 Subject: [PATCH 12/19] net/i40e: Replace 0-length array with flexible array Zero-length arrays are deprecated[1]. Replace struct i40e_lump_tracking's "list" 0-length array with a flexible array. Detected with GCC 13, using -fstrict-flex-arrays=3: In function 'i40e_put_lump', inlined from 'i40e_clear_interrupt_scheme' at drivers/net/ethernet/intel/i40e/i40e_main.c:5145:2: drivers/net/ethernet/intel/i40e/i40e_main.c:278:27: warning: array subscript is outside array bounds of 'u16[0]' {aka 'short unsigned int[]'} [-Warray-bounds=] 278 | pile->list[i] = 0; | ~~~~~~~~~~^~~ drivers/net/ethernet/intel/i40e/i40e.h: In function 'i40e_clear_interrupt_scheme': drivers/net/ethernet/intel/i40e/i40e.h:179:13: note: while referencing 'list' 179 | u16 list[0]; | ^~~~ [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays Cc: Jesse Brandeburg Cc: Tony Nguyen Cc: "David S. Miller" Cc: Eric Dumazet Cc: Jakub Kicinski Cc: Paolo Abeni Cc: "Gustavo A. R. Silva" Cc: intel-wired-lan@lists.osuosl.org Cc: netdev@vger.kernel.org Signed-off-by: Kees Cook Reviewed-by: Jiri Pirko Tested-by: Gurucharan G (A Contingent worker at Intel) Reviewed-by: Gustavo A. R. Silva Link: https://lore.kernel.org/r/20230105234557.never.799-kees@kernel.org --- drivers/net/ethernet/intel/i40e/i40e.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h index 60e351665c70..3a1c28ca5bb4 100644 --- a/drivers/net/ethernet/intel/i40e/i40e.h +++ b/drivers/net/ethernet/intel/i40e/i40e.h @@ -176,7 +176,7 @@ enum i40e_interrupt_policy { struct i40e_lump_tracking { u16 num_entries; - u16 list[0]; + u16 list[]; #define I40E_PILE_VALID_BIT 0x8000 #define I40E_IWARP_IRQ_PILE_ID (I40E_PILE_VALID_BIT - 2) }; From aa85923a954e7704bc9d3847dabeb8540aa98d13 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Thu, 5 Jan 2023 20:19:48 -0800 Subject: [PATCH 13/19] crypto: hisilicon: Wipe entire pool on error To work around a Clang __builtin_object_size bug that shows up under CONFIG_FORTIFY_SOURCE and UBSAN_BOUNDS, move the per-loop-iteration mem_block wipe into a single wipe of the entire pool structure after the loop. Reported-by: Nathan Chancellor Link: https://github.com/ClangBuiltLinux/linux/issues/1780 Cc: Weili Qian Cc: Zhou Wang Cc: Herbert Xu Cc: "David S. Miller" Cc: linux-crypto@vger.kernel.org Signed-off-by: Kees Cook Tested-by: Nathan Chancellor # build Link: https://lore.kernel.org/r/20230106041945.never.831-kees@kernel.org --- drivers/crypto/hisilicon/sgl.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/crypto/hisilicon/sgl.c b/drivers/crypto/hisilicon/sgl.c index 2b6f2281cfd6..0974b0041405 100644 --- a/drivers/crypto/hisilicon/sgl.c +++ b/drivers/crypto/hisilicon/sgl.c @@ -124,9 +124,8 @@ err_free_mem: for (j = 0; j < i; j++) { dma_free_coherent(dev, block_size, block[j].sgl, block[j].sgl_dma); - memset(block + j, 0, sizeof(*block)); } - kfree(pool); + kfree_sensitive(pool); return ERR_PTR(-ENOMEM); } EXPORT_SYMBOL_GPL(hisi_acc_create_sgl_pool); From a8c55407a7230798eb157ed2cf5398a6a2b123b6 Mon Sep 17 00:00:00 2001 From: Andy Shevchenko Date: Fri, 27 Jan 2023 17:51:35 +0200 Subject: [PATCH 14/19] lib/string: Use strchr() in strpbrk() Use strchr() instead of open coding it as it's done elsewhere in the same file. Either we will have similar to what it was or possibly better performance in case architecture implements its own strchr(). Memory wise on x86_64 bloat-o-meter shows the following Function old new delta strsep 111 102 -9 Total: Before=2763, After=2754, chg -0.33% Signed-off-by: Andy Shevchenko Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20230127155135.27153-1-andriy.shevchenko@linux.intel.com --- lib/string.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/lib/string.c b/lib/string.c index 4fb566ea610f..3d55ef890106 100644 --- a/lib/string.c +++ b/lib/string.c @@ -480,13 +480,11 @@ EXPORT_SYMBOL(strcspn); */ char *strpbrk(const char *cs, const char *ct) { - const char *sc1, *sc2; + const char *sc; - for (sc1 = cs; *sc1 != '\0'; ++sc1) { - for (sc2 = ct; *sc2 != '\0'; ++sc2) { - if (*sc1 == *sc2) - return (char *)sc1; - } + for (sc = cs; *sc != '\0'; ++sc) { + if (strchr(ct, *sc)) + return (char *)sc; } return NULL; } From 5a6b64adc18d9adfb497a529ff004d59b6df151f Mon Sep 17 00:00:00 2001 From: Sam James Date: Wed, 1 Feb 2023 23:00:09 +0000 Subject: [PATCH 15/19] gcc-plugins: drop -std=gnu++11 to fix GCC 13 build The latest GCC 13 snapshot (13.0.1 20230129) gives the following: ``` cc1: error: cannot load plugin ./scripts/gcc-plugins/randomize_layout_plugin.so :./scripts/gcc-plugins/randomize_layout_plugin.so: undefined symbol: tree_code_type ``` This ends up being because of https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=b0241ce6e37031 upstream in GCC which changes the visibility of some types used by the kernel's plugin infrastructure like tree_code_type. After discussion with the GCC folks, we found that the kernel needs to be building plugins with the same flags used to build GCC - and GCC defaults to gnu++17 right now. The minimum GCC version needed to build the kernel is GCC 5.1 and GCC 5.1 already defaults to gnu++14 anyway, so just drop the flag, as all GCCs that could be used to build GCC already default to an acceptable version which was >= the version we forced via flags until now. Bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108634 Signed-off-by: Sam James Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20230201230009.2252783-1-sam@gentoo.org --- scripts/gcc-plugins/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/gcc-plugins/Makefile b/scripts/gcc-plugins/Makefile index b34d11e22636..320afd3cf8e8 100644 --- a/scripts/gcc-plugins/Makefile +++ b/scripts/gcc-plugins/Makefile @@ -29,7 +29,7 @@ GCC_PLUGINS_DIR = $(shell $(CC) -print-file-name=plugin) plugin_cxxflags = -Wp,-MMD,$(depfile) $(KBUILD_HOSTCXXFLAGS) -fPIC \ -include $(srctree)/include/linux/compiler-version.h \ -DPLUGIN_VERSION=$(call stringify,$(KERNELVERSION)) \ - -I $(GCC_PLUGINS_DIR)/include -I $(obj) -std=gnu++11 \ + -I $(GCC_PLUGINS_DIR)/include -I $(obj) \ -fno-rtti -fno-exceptions -fasynchronous-unwind-tables \ -ggdb -Wno-narrowing -Wno-unused-variable \ -Wno-format-diag From 48df133578c70185a95a49390d42df1996ddba2a Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 27 Jan 2023 14:39:21 -0800 Subject: [PATCH 16/19] coda: Avoid partial allocation of sig_inputArgs GCC does not like having a partially allocated object, since it cannot reason about it for bounds checking when it is passed to other code. Instead, fully allocate sig_inputArgs. (Alternatively, sig_inputArgs should be defined as a struct coda_in_hdr, if it is actually not using any other part of the union.) Seen under GCC 13: ../fs/coda/upcall.c: In function 'coda_upcall': ../fs/coda/upcall.c:801:22: warning: array subscript 'union inputArgs[0]' is partly outside array bounds of 'unsigned char[20]' [-Warray-bounds=] 801 | sig_inputArgs->ih.opcode = CODA_SIGNAL; | ^~ Cc: Jan Harkes Cc: coda@cs.cmu.edu Cc: codalist@coda.cs.cmu.edu Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20230127223921.never.882-kees@kernel.org --- fs/coda/upcall.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/coda/upcall.c b/fs/coda/upcall.c index 59f6cfd06f96..cd6a3721f6f6 100644 --- a/fs/coda/upcall.c +++ b/fs/coda/upcall.c @@ -791,7 +791,7 @@ static int coda_upcall(struct venus_comm *vcp, sig_req = kmalloc(sizeof(struct upc_req), GFP_KERNEL); if (!sig_req) goto exit; - sig_inputArgs = kvzalloc(sizeof(struct coda_in_hdr), GFP_KERNEL); + sig_inputArgs = kvzalloc(sizeof(*sig_inputArgs), GFP_KERNEL); if (!sig_inputArgs) { kfree(sig_req); goto exit; From 25b84002afb9dc9a91a7ea67166879c13ad82422 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Thu, 2 Feb 2023 22:07:49 +0000 Subject: [PATCH 17/19] arm64: Support Clang UBSAN trap codes for better reporting When building with CONFIG_UBSAN_TRAP=y on arm64, Clang encodes the UBSAN check (handler) type in the esr. Extract this and actually report these traps as coming from the specific UBSAN check that tripped. Before: Internal error: BRK handler: 00000000f20003e8 [#1] PREEMPT SMP After: Internal error: UBSAN: shift out of bounds: 00000000f2005514 [#1] PREEMPT SMP Acked-by: Mark Rutland Reviewed-by: Ard Biesheuvel Acked-by: Mukesh Ojha Reviewed-by: Fangrui Song Cc: Catalin Marinas Cc: Will Deacon Cc: John Stultz Cc: Yongqin Liu Cc: Sami Tolvanen Cc: Yury Norov Cc: Andrey Konovalov Cc: Marco Elver Cc: linux-arm-kernel@lists.infradead.org Cc: llvm@lists.linux.dev Signed-off-by: Kees Cook --- arch/arm64/include/asm/brk-imm.h | 3 ++ arch/arm64/kernel/traps.c | 21 ++++++++++ include/linux/ubsan.h | 9 +++++ lib/Makefile | 2 - lib/ubsan.c | 68 ++++++++++++++++++++++++++++++++ lib/ubsan.h | 32 +++++++++++++++ 6 files changed, 133 insertions(+), 2 deletions(-) create mode 100644 include/linux/ubsan.h diff --git a/arch/arm64/include/asm/brk-imm.h b/arch/arm64/include/asm/brk-imm.h index 6e000113e508..1abdcd508a11 100644 --- a/arch/arm64/include/asm/brk-imm.h +++ b/arch/arm64/include/asm/brk-imm.h @@ -17,6 +17,7 @@ * 0x401: for compile time BRK instruction * 0x800: kernel-mode BUG() and WARN() traps * 0x9xx: tag-based KASAN trap (allowed values 0x900 - 0x9ff) + * 0x55xx: Undefined Behavior Sanitizer traps ('U' << 8) * 0x8xxx: Control-Flow Integrity traps */ #define KPROBES_BRK_IMM 0x004 @@ -28,6 +29,8 @@ #define BUG_BRK_IMM 0x800 #define KASAN_BRK_IMM 0x900 #define KASAN_BRK_MASK 0x0ff +#define UBSAN_BRK_IMM 0x5500 +#define UBSAN_BRK_MASK 0x00ff #define CFI_BRK_IMM_TARGET GENMASK(4, 0) #define CFI_BRK_IMM_TYPE GENMASK(9, 5) diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c index 4c0caa589e12..87f42eb1c950 100644 --- a/arch/arm64/kernel/traps.c +++ b/arch/arm64/kernel/traps.c @@ -26,6 +26,7 @@ #include #include #include +#include #include #include @@ -1074,6 +1075,19 @@ static struct break_hook kasan_break_hook = { }; #endif +#ifdef CONFIG_UBSAN_TRAP +static int ubsan_handler(struct pt_regs *regs, unsigned long esr) +{ + die(report_ubsan_failure(regs, esr & UBSAN_BRK_MASK), regs, esr); + return DBG_HOOK_HANDLED; +} + +static struct break_hook ubsan_break_hook = { + .fn = ubsan_handler, + .imm = UBSAN_BRK_IMM, + .mask = UBSAN_BRK_MASK, +}; +#endif #define esr_comment(esr) ((esr) & ESR_ELx_BRK64_ISS_COMMENT_MASK) @@ -1091,6 +1105,10 @@ int __init early_brk64(unsigned long addr, unsigned long esr, #ifdef CONFIG_KASAN_SW_TAGS if ((esr_comment(esr) & ~KASAN_BRK_MASK) == KASAN_BRK_IMM) return kasan_handler(regs, esr) != DBG_HOOK_HANDLED; +#endif +#ifdef CONFIG_UBSAN_TRAP + if ((esr_comment(esr) & ~UBSAN_BRK_MASK) == UBSAN_BRK_IMM) + return ubsan_handler(regs, esr) != DBG_HOOK_HANDLED; #endif return bug_handler(regs, esr) != DBG_HOOK_HANDLED; } @@ -1104,6 +1122,9 @@ void __init trap_init(void) register_kernel_break_hook(&fault_break_hook); #ifdef CONFIG_KASAN_SW_TAGS register_kernel_break_hook(&kasan_break_hook); +#endif +#ifdef CONFIG_UBSAN_TRAP + register_kernel_break_hook(&ubsan_break_hook); #endif debug_traps_init(); } diff --git a/include/linux/ubsan.h b/include/linux/ubsan.h new file mode 100644 index 000000000000..bff7445498de --- /dev/null +++ b/include/linux/ubsan.h @@ -0,0 +1,9 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _LINUX_UBSAN_H +#define _LINUX_UBSAN_H + +#ifdef CONFIG_UBSAN_TRAP +const char *report_ubsan_failure(struct pt_regs *regs, u32 check_type); +#endif + +#endif diff --git a/lib/Makefile b/lib/Makefile index 4d9461bfea42..81b988bf9448 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -340,9 +340,7 @@ quiet_cmd_build_OID_registry = GEN $@ clean-files += oid_registry_data.c obj-$(CONFIG_UCS2_STRING) += ucs2_string.o -ifneq ($(CONFIG_UBSAN_TRAP),y) obj-$(CONFIG_UBSAN) += ubsan.o -endif UBSAN_SANITIZE_ubsan.o := n KASAN_SANITIZE_ubsan.o := n diff --git a/lib/ubsan.c b/lib/ubsan.c index 60c7099857a0..6620e0719c9d 100644 --- a/lib/ubsan.c +++ b/lib/ubsan.c @@ -14,10 +14,76 @@ #include #include #include +#include #include #include "ubsan.h" +#ifdef CONFIG_UBSAN_TRAP +/* + * Only include matches for UBSAN checks that are actually compiled in. + * The mappings of struct SanitizerKind (the -fsanitize=xxx args) to + * enum SanitizerHandler (the traps) in Clang is in clang/lib/CodeGen/. + */ +const char *report_ubsan_failure(struct pt_regs *regs, u32 check_type) +{ + switch (check_type) { +#ifdef CONFIG_UBSAN_BOUNDS + /* + * SanitizerKind::ArrayBounds and SanitizerKind::LocalBounds + * emit SanitizerHandler::OutOfBounds. + */ + case ubsan_out_of_bounds: + return "UBSAN: array index out of bounds"; +#endif +#ifdef CONFIG_UBSAN_SHIFT + /* + * SanitizerKind::ShiftBase and SanitizerKind::ShiftExponent + * emit SanitizerHandler::ShiftOutOfBounds. + */ + case ubsan_shift_out_of_bounds: + return "UBSAN: shift out of bounds"; +#endif +#ifdef CONFIG_UBSAN_DIV_ZERO + /* + * SanitizerKind::IntegerDivideByZero emits + * SanitizerHandler::DivremOverflow. + */ + case ubsan_divrem_overflow: + return "UBSAN: divide/remainder overflow"; +#endif +#ifdef CONFIG_UBSAN_UNREACHABLE + /* + * SanitizerKind::Unreachable emits + * SanitizerHandler::BuiltinUnreachable. + */ + case ubsan_builtin_unreachable: + return "UBSAN: unreachable code"; +#endif +#if defined(CONFIG_UBSAN_BOOL) || defined(CONFIG_UBSAN_ENUM) + /* + * SanitizerKind::Bool and SanitizerKind::Enum emit + * SanitizerHandler::LoadInvalidValue. + */ + case ubsan_load_invalid_value: + return "UBSAN: loading invalid value"; +#endif +#ifdef CONFIG_UBSAN_ALIGNMENT + /* + * SanitizerKind::Alignment emits SanitizerHandler::TypeMismatch + * or SanitizerHandler::AlignmentAssumption. + */ + case ubsan_alignment_assumption: + return "UBSAN: alignment assumption"; + case ubsan_type_mismatch: + return "UBSAN: type mismatch"; +#endif + default: + return "UBSAN: unrecognized failure code"; + } +} + +#else static const char * const type_check_kinds[] = { "load of", "store to", @@ -384,3 +450,5 @@ void __ubsan_handle_alignment_assumption(void *_data, unsigned long ptr, ubsan_epilogue(); } EXPORT_SYMBOL(__ubsan_handle_alignment_assumption); + +#endif /* !CONFIG_UBSAN_TRAP */ diff --git a/lib/ubsan.h b/lib/ubsan.h index 9a0b71c5ff9f..cc5cb94895a6 100644 --- a/lib/ubsan.h +++ b/lib/ubsan.h @@ -2,6 +2,38 @@ #ifndef _LIB_UBSAN_H #define _LIB_UBSAN_H +/* + * ABI defined by Clang's UBSAN enum SanitizerHandler: + * https://github.com/llvm/llvm-project/blob/release/16.x/clang/lib/CodeGen/CodeGenFunction.h#L113 + */ +enum ubsan_checks { + ubsan_add_overflow, + ubsan_builtin_unreachable, + ubsan_cfi_check_fail, + ubsan_divrem_overflow, + ubsan_dynamic_type_cache_miss, + ubsan_float_cast_overflow, + ubsan_function_type_mismatch, + ubsan_implicit_conversion, + ubsan_invalid_builtin, + ubsan_invalid_objc_cast, + ubsan_load_invalid_value, + ubsan_missing_return, + ubsan_mul_overflow, + ubsan_negate_overflow, + ubsan_nullability_arg, + ubsan_nullability_return, + ubsan_nonnull_arg, + ubsan_nonnull_return, + ubsan_out_of_bounds, + ubsan_pointer_overflow, + ubsan_shift_out_of_bounds, + ubsan_sub_overflow, + ubsan_type_mismatch, + ubsan_alignment_assumption, + ubsan_vla_bound_not_positive, +}; + enum { type_kind_int = 0, type_kind_float = 1, From 04ffde1319a715bd0550ded3580d4ea3bc003776 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Tue, 31 Jan 2023 17:37:59 -0800 Subject: [PATCH 18/19] uaccess: Add minimum bounds check on kernel buffer size While there is logic about the difference between ksize and usize, copy_struct_from_user() didn't check the size of the destination buffer (when it was known) against ksize. Add this check so there is an upper bounds check on the possible memset() call, otherwise lower bounds checks made by callers will trigger bounds warnings under -Warray-bounds. Seen under GCC 13: In function 'copy_struct_from_user', inlined from 'iommufd_fops_ioctl' at ../drivers/iommu/iommufd/main.c:333:8: ../include/linux/fortify-string.h:59:33: warning: '__builtin_memset' offset [57, 4294967294] is out of the bounds [0, 56] of object 'buf' with type 'union ucmd_buffer' [-Warray-bounds=] 59 | #define __underlying_memset __builtin_memset | ^ ../include/linux/fortify-string.h:453:9: note: in expansion of macro '__underlying_memset' 453 | __underlying_memset(p, c, __fortify_size); \ | ^~~~~~~~~~~~~~~~~~~ ../include/linux/fortify-string.h:461:25: note: in expansion of macro '__fortify_memset_chk' 461 | #define memset(p, c, s) __fortify_memset_chk(p, c, s, \ | ^~~~~~~~~~~~~~~~~~~~ ../include/linux/uaccess.h:334:17: note: in expansion of macro 'memset' 334 | memset(dst + size, 0, rest); | ^~~~~~ ../drivers/iommu/iommufd/main.c: In function 'iommufd_fops_ioctl': ../drivers/iommu/iommufd/main.c:311:27: note: 'buf' declared here 311 | union ucmd_buffer buf; | ^~~ Cc: Christian Brauner Cc: Rasmus Villemoes Cc: Arnd Bergmann Cc: Dinh Nguyen Cc: Catalin Marinas Cc: Andrew Morton Cc: Geert Uytterhoeven Cc: Alexander Potapenko Acked-by: Aleksa Sarai Signed-off-by: Kees Cook Link: https://lore.kernel.org/lkml/20230203193523.never.667-kees@kernel.org/ --- include/linux/uaccess.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h index afb18f198843..ab9728138ad6 100644 --- a/include/linux/uaccess.h +++ b/include/linux/uaccess.h @@ -329,6 +329,10 @@ copy_struct_from_user(void *dst, size_t ksize, const void __user *src, size_t size = min(ksize, usize); size_t rest = max(ksize, usize) - size; + /* Double check if ksize is larger than a known object size. */ + if (WARN_ON_ONCE(ksize > __builtin_object_size(dst, 1))) + return -E2BIG; + /* Deal with trailing bytes. */ if (usize < ksize) { memset(dst + size, 0, rest); From 78f7a3fd6dc66cb788c21d7705977ed13c879351 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Tue, 7 Feb 2023 22:51:33 -0800 Subject: [PATCH 19/19] randstruct: disable Clang 15 support The randstruct support released in Clang 15 is unsafe to use due to a bug that can cause miscompilations: "-frandomize-layout-seed inconsistently randomizes all-function-pointers structs" (https://github.com/llvm/llvm-project/issues/60349). It has been fixed on the Clang 16 release branch, so add a Clang version check. Fixes: 035f7f87b729 ("randstruct: Enable Clang support") Cc: stable@vger.kernel.org Signed-off-by: Eric Biggers Acked-by: Nick Desaulniers Reviewed-by: Nathan Chancellor Reviewed-by: Bill Wendling Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20230208065133.220589-1-ebiggers@kernel.org --- security/Kconfig.hardening | 3 +++ 1 file changed, 3 insertions(+) diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening index 53baa95cb644..0f295961e773 100644 --- a/security/Kconfig.hardening +++ b/security/Kconfig.hardening @@ -281,6 +281,9 @@ endmenu config CC_HAS_RANDSTRUCT def_bool $(cc-option,-frandomize-layout-seed-file=/dev/null) + # Randstruct was first added in Clang 15, but it isn't safe to use until + # Clang 16 due to https://github.com/llvm/llvm-project/issues/60349 + depends on !CC_IS_CLANG || CLANG_VERSION >= 160000 choice prompt "Randomize layout of sensitive kernel structures"