From 2c7ccb3c362bc86aeb3e52d6fbf15f7f480ca961 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Wed, 13 Sep 2023 16:54:14 -0700 Subject: [PATCH 1/7] module: Clarify documentation of module_param_call() Commit 9bbb9e5a3310 ("param: use ops in struct kernel_param, rather than get and set fns directly") added the comment that module_param_call() was deprecated, during a large scale refactoring to bring sanity to type casting back then. In 2017 following more cleanups, it became useful again as it wraps a common pattern of creating an ops struct for a given get/set pair: b2f270e87473 ("module: Prepare to convert all module_param_call() prototypes") ece1996a21ee ("module: Do not paper over type mismatches in module_param_call()") static const struct kernel_param_ops __param_ops_##name = \ { .flags = 0, .set = _set, .get = _get }; \ __module_param_call(MODULE_PARAM_PREFIX, \ name, &__param_ops_##name, arg, perm, -1, 0) __module_param_call(MODULE_PARAM_PREFIX, name, ops, arg, perm, -1, 0) Many users of module_param_cb() appear to be almost universally open-coding the same thing that module_param_call() does now. Don't discourage[1] people from using module_param_call(): clarify the comment to show that module_param_cb() is useful if you repeatedly use the same pair of get/set functions. [1] https://lore.kernel.org/lkml/202308301546.5C789E5EC@keescook/ Cc: Luis Chamberlain Cc: Johan Hovold Cc: Jessica Yu Cc: Sagi Grimberg Cc: Nick Desaulniers Cc: Miguel Ojeda Cc: Joe Perches Cc: linux-modules@vger.kernel.org Reviewed-by: Miguel Ojeda Signed-off-by: Kees Cook Signed-off-by: Luis Chamberlain --- include/linux/moduleparam.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h index 962cd41a2cb5..d4452f93d060 100644 --- a/include/linux/moduleparam.h +++ b/include/linux/moduleparam.h @@ -293,7 +293,11 @@ struct kparam_array = { __param_str_##name, THIS_MODULE, ops, \ VERIFY_OCTAL_PERMISSIONS(perm), level, flags, { arg } } -/* Obsolete - use module_param_cb() */ +/* + * Useful for describing a set/get pair used only once (i.e. for this + * parameter). For repeated set/get pairs (i.e. the same struct + * kernel_param_ops), use module_param_cb() instead. + */ #define module_param_call(name, _set, _get, arg, perm) \ static const struct kernel_param_ops __param_ops_##name = \ { .flags = 0, .set = _set, .get = _get }; \ From 62eedac264159690b4fed78aeec4d3623ce253b7 Mon Sep 17 00:00:00 2001 From: Luis Chamberlain Date: Wed, 20 Sep 2023 14:07:58 -0700 Subject: [PATCH 2/7] MAINTAINERS: add include/linux/module*.h to modules Use glob include/linux/module*.h to capture all module changes. Suggested-by: Kees Cook Reviewed-by: Kees Cook Signed-off-by: Luis Chamberlain --- MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index ca67fd0ed5bf..8b7d80146d4a 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14519,7 +14519,7 @@ L: linux-kernel@vger.kernel.org S: Maintained T: git git://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git modules-next F: include/linux/kmod.h -F: include/linux/module.h +F: include/linux/module*.h F: kernel/module/ F: lib/test_kmod.c F: scripts/module* From 3737df782c740b944912ed93420c57344b1cf864 Mon Sep 17 00:00:00 2001 From: Andrea Righi Date: Wed, 30 Aug 2023 17:58:20 +0200 Subject: [PATCH 3/7] module/decompress: use vmalloc() for gzip decompression workspace Use a similar approach as commit a419beac4a07 ("module/decompress: use vmalloc() for zstd decompression workspace") and replace kmalloc() with vmalloc() also for the gzip module decompression workspace. In this case the workspace is represented by struct inflate_workspace that can be fairly large for kmalloc() and it can potentially lead to allocation errors on certain systems: $ pahole inflate_workspace struct inflate_workspace { struct inflate_state inflate_state; /* 0 9544 */ /* --- cacheline 149 boundary (9536 bytes) was 8 bytes ago --- */ unsigned char working_window[32768]; /* 9544 32768 */ /* size: 42312, cachelines: 662, members: 2 */ /* last cacheline: 8 bytes */ }; Considering that there is no need to use continuous physical memory, simply switch to vmalloc() to provide a more reliable in-kernel module decompression. Fixes: b1ae6dc41eaa ("module: add in-kernel support for decompressing") Signed-off-by: Andrea Righi Signed-off-by: Luis Chamberlain --- kernel/module/decompress.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/kernel/module/decompress.c b/kernel/module/decompress.c index 87440f714c0c..4156d59be440 100644 --- a/kernel/module/decompress.c +++ b/kernel/module/decompress.c @@ -100,7 +100,7 @@ static ssize_t module_gzip_decompress(struct load_info *info, s.next_in = buf + gzip_hdr_len; s.avail_in = size - gzip_hdr_len; - s.workspace = kmalloc(zlib_inflate_workspacesize(), GFP_KERNEL); + s.workspace = vmalloc(zlib_inflate_workspacesize()); if (!s.workspace) return -ENOMEM; @@ -138,7 +138,7 @@ static ssize_t module_gzip_decompress(struct load_info *info, out_inflate_end: zlib_inflateEnd(&s); out: - kfree(s.workspace); + vfree(s.workspace); return retval; } #elif defined(CONFIG_MODULE_COMPRESS_XZ) From 60da3640b07ce03706a8c77a3740ebad8b9af063 Mon Sep 17 00:00:00 2001 From: Tiezhu Yang Date: Fri, 16 Jun 2023 09:51:32 +0800 Subject: [PATCH 4/7] module: Make is_mapping_symbol() return bool The return value of is_mapping_symbol() is true or false, so change its type to reflect that. Suggested-by: Xi Zhang Signed-off-by: Tiezhu Yang Signed-off-by: Luis Chamberlain --- include/linux/module_symbol.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/module_symbol.h b/include/linux/module_symbol.h index 1269543d0634..77c9895b9ddb 100644 --- a/include/linux/module_symbol.h +++ b/include/linux/module_symbol.h @@ -3,7 +3,7 @@ #define _LINUX_MODULE_SYMBOL_H /* This ignores the intensely annoying "mapping symbols" found in ELF files. */ -static inline int is_mapping_symbol(const char *str) +static inline bool is_mapping_symbol(const char *str) { if (str[0] == '.' && str[1] == 'L') return true; From 04311b9b306388288f72cf6ebde659274b06ffd6 Mon Sep 17 00:00:00 2001 From: Tiezhu Yang Date: Fri, 16 Jun 2023 09:51:33 +0800 Subject: [PATCH 5/7] module: Make is_valid_name() return bool The return value of is_valid_name() is true or false, so change its type to reflect that. Signed-off-by: Tiezhu Yang Signed-off-by: Luis Chamberlain --- scripts/mod/modpost.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c index b3dee80497cb..ea72af229d2b 100644 --- a/scripts/mod/modpost.c +++ b/scripts/mod/modpost.c @@ -1059,12 +1059,12 @@ static int secref_whitelist(const char *fromsec, const char *fromsym, * only by merging __exit and __init sections into __text, bloating * the kernel (which is especially evil on embedded platforms). */ -static inline int is_valid_name(struct elf_info *elf, Elf_Sym *sym) +static inline bool is_valid_name(struct elf_info *elf, Elf_Sym *sym) { const char *name = elf->strtab + sym->st_name; if (!name || !strlen(name)) - return 0; + return false; return !is_mapping_symbol(name); } From fd06da776130ec2611c30272a0868f6a54cdf9d2 Mon Sep 17 00:00:00 2001 From: Zhu Mao Date: Wed, 20 Sep 2023 17:13:09 -0700 Subject: [PATCH 6/7] module: Fix comment typo Delete duplicated word in comment. Signed-off-by: Zhu Mao Signed-off-by: Luis Chamberlain --- kernel/module/stats.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/module/stats.c b/kernel/module/stats.c index 6ab2c94d6bc3..3ba0e98b3c91 100644 --- a/kernel/module/stats.c +++ b/kernel/module/stats.c @@ -126,7 +126,7 @@ static LIST_HEAD(dup_failed_modules); * These typically should not happen unless your system is under memory * pressure. * * invalid_becoming_bytes: total number of bytes allocated and freed used - * used to read the kernel module userspace wants us to read before we + * to read the kernel module userspace wants us to read before we * promote it to be processed to be added to our @modules linked list. These * failures can happen if we had a check in between a successful kernel_read_file_from_fd() * call and right before we allocate the our private memory for the module From ea0b0bcef4917a2640ecc100c768b8e785784834 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 22 Sep 2023 10:52:53 -0700 Subject: [PATCH 7/7] module: Annotate struct module_notes_attrs 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 module_notes_attrs. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Luis Chamberlain Cc: linux-modules@vger.kernel.org Signed-off-by: Kees Cook Signed-off-by: Luis Chamberlain --- kernel/module/sysfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c index c921bf044050..d964167c6658 100644 --- a/kernel/module/sysfs.c +++ b/kernel/module/sysfs.c @@ -143,7 +143,7 @@ static void remove_sect_attrs(struct module *mod) struct module_notes_attrs { struct kobject *dir; unsigned int notes; - struct bin_attribute attrs[]; + struct bin_attribute attrs[] __counted_by(notes); }; static ssize_t module_notes_read(struct file *filp, struct kobject *kobj,