From a43e0fc5e9134a46515de2f2f8d4100b74e50de3 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Thu, 22 Feb 2024 09:48:46 -0800 Subject: [PATCH 1/6] pstore: inode: Only d_invalidate() is needed Unloading a modular pstore backend with records in pstorefs would trigger the dput() double-drop warning: WARNING: CPU: 0 PID: 2569 at fs/dcache.c:762 dput.part.0+0x3f3/0x410 Using the combo of d_drop()/dput() (as mentioned in Documentation/filesystems/vfs.rst) isn't the right approach here, and leads to the reference counting problem seen above. Use d_invalidate() and update the code to not bother checking for error codes that can never happen. Suggested-by: Alexander Viro Fixes: 609e28bb139e ("pstore: Remove filesystem records when backend is unregistered") Signed-off-by: Kees Cook --- Cc: "Guilherme G. Piccoli" Cc: Tony Luck Cc: linux-hardening@vger.kernel.org --- fs/pstore/inode.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c index d0d9bfdad30c..56815799ce79 100644 --- a/fs/pstore/inode.c +++ b/fs/pstore/inode.c @@ -307,7 +307,6 @@ int pstore_put_backend_records(struct pstore_info *psi) { struct pstore_private *pos, *tmp; struct dentry *root; - int rc = 0; root = psinfo_lock_root(); if (!root) @@ -317,11 +316,8 @@ int pstore_put_backend_records(struct pstore_info *psi) list_for_each_entry_safe(pos, tmp, &records_list, list) { if (pos->record->psi == psi) { list_del_init(&pos->list); - rc = simple_unlink(d_inode(root), pos->dentry); - if (WARN_ON(rc)) - break; - d_drop(pos->dentry); - dput(pos->dentry); + d_invalidate(pos->dentry); + simple_unlink(d_inode(root), pos->dentry); pos->dentry = NULL; } } @@ -329,7 +325,7 @@ int pstore_put_backend_records(struct pstore_info *psi) inode_unlock(d_inode(root)); - return rc; + return 0; } /* From 12dc54f568d4f589fd49d7c143cca0cc5fa221fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?N=C3=ADcolas=20F=2E=20R=2E=20A=2E=20Prado?= Date: Wed, 10 Jan 2024 18:05:02 -0300 Subject: [PATCH 2/6] pstore/ram: Register to module device table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Register the compatible for this module on the module device table so it can be automatically loaded when a matching DT node is present, allowing logging of panics and oopses without any intervention. Signed-off-by: Nícolas F. R. A. Prado Reviewed-by: Kees Cook Reviewed-by: AngeloGioacchino Del Regno Link: https://lore.kernel.org/r/20240110210600.787703-2-nfraprado@collabora.com Signed-off-by: Kees Cook --- fs/pstore/ram.c | 1 + 1 file changed, 1 insertion(+) diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c index 88b34fdbf759..b1a455f42e93 100644 --- a/fs/pstore/ram.c +++ b/fs/pstore/ram.c @@ -893,6 +893,7 @@ static const struct of_device_id dt_match[] = { { .compatible = "ramoops" }, {} }; +MODULE_DEVICE_TABLE(of, dt_match); static struct platform_driver ramoops_driver = { .probe = ramoops_probe, From 77a6557d2a58ad9abea0537509d6dfc946cddfd3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?N=C3=ADcolas=20F=2E=20R=2E=20A=2E=20Prado?= Date: Wed, 10 Jan 2024 18:05:03 -0300 Subject: [PATCH 3/6] arm64: defconfig: Enable PSTORE_RAM MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Enable PSTORE_RAM, that is the ramoops driver, in the defconfig, to allow logging and retrieving panics and oopses to/from RAM automatically for platforms that have a ramoops reserved memory node in DT. Signed-off-by: Nícolas F. R. A. Prado Reviewed-by: David Heidelberg Link: https://lore.kernel.org/r/20240110210600.787703-3-nfraprado@collabora.com Signed-off-by: Kees Cook --- arch/arm64/configs/defconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig index e6cf3e5d63c3..b0652c52bf3a 100644 --- a/arch/arm64/configs/defconfig +++ b/arch/arm64/configs/defconfig @@ -1590,6 +1590,7 @@ CONFIG_CONFIGFS_FS=y CONFIG_EFIVAR_FS=y CONFIG_UBIFS_FS=m CONFIG_SQUASHFS=y +CONFIG_PSTORE_RAM=m CONFIG_NFS_FS=y CONFIG_NFS_V4=y CONFIG_NFS_V4_1=y From a28655c330ab294862cabe66deadb0f85cd4f191 Mon Sep 17 00:00:00 2001 From: "Guilherme G. Piccoli" Date: Wed, 3 Jan 2024 15:40:32 -0300 Subject: [PATCH 4/6] efi: pstore: Allow dynamic initialization based on module parameter The efi-pstore module parameter "pstore_disable" warrants that users are able to deactivate such backend. There is also a Kconfig option for the default value of this parameter. It was originally added due to some bad UEFI FW implementations that could break with many variables written. Some distros (such as Arch Linux) set this in their config file still nowadays. And once it is set, even being a writable module parameter, there is effectively no way to make use of efi-pstore anymore. If "pstore_disable" is set to true, the init function of the module exits early and is never called again after the initcall processing. Let's switch this module parameter to have a callback and perform the pstore backend registration again each time it's set from Y->N (and vice-versa). With this, the writable nature of the parameter starts to make sense, given that users now can switch back to using efi-pstore or not during runtime by writing into it. Signed-off-by: Guilherme G. Piccoli Link: https://lore.kernel.org/r/20240103184053.226203-1-gpiccoli@igalia.com Signed-off-by: Kees Cook --- drivers/firmware/efi/efi-pstore.c | 43 +++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 8 deletions(-) diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c index e7b9ec6f8a86..833cbb995dd3 100644 --- a/drivers/firmware/efi/efi-pstore.c +++ b/drivers/firmware/efi/efi-pstore.c @@ -14,16 +14,43 @@ static unsigned int record_size = 1024; module_param(record_size, uint, 0444); MODULE_PARM_DESC(record_size, "size of each pstore UEFI var (in bytes, min/default=1024)"); -static bool efivars_pstore_disable = - IS_ENABLED(CONFIG_EFI_VARS_PSTORE_DEFAULT_DISABLE); - -module_param_named(pstore_disable, efivars_pstore_disable, bool, 0644); - #define PSTORE_EFI_ATTRIBUTES \ (EFI_VARIABLE_NON_VOLATILE | \ EFI_VARIABLE_BOOTSERVICE_ACCESS | \ EFI_VARIABLE_RUNTIME_ACCESS) +static bool pstore_disable = IS_ENABLED(CONFIG_EFI_VARS_PSTORE_DEFAULT_DISABLE); + +static int efivars_pstore_init(void); +static void efivars_pstore_exit(void); + +static int efi_pstore_disable_set(const char *val, const struct kernel_param *kp) +{ + int err; + bool old_pstore_disable = pstore_disable; + + err = param_set_bool(val, kp); + if (err) + return err; + + if (old_pstore_disable != pstore_disable) { + if (pstore_disable) + efivars_pstore_exit(); + else + efivars_pstore_init(); + } + + return 0; +} + +static const struct kernel_param_ops pstore_disable_ops = { + .set = efi_pstore_disable_set, + .get = param_get_bool, +}; + +module_param_cb(pstore_disable, &pstore_disable_ops, &pstore_disable, 0644); +__MODULE_PARM_TYPE(pstore_disable, "bool"); + static int efi_pstore_open(struct pstore_info *psi) { int err; @@ -218,12 +245,12 @@ static struct pstore_info efi_pstore_info = { .erase = efi_pstore_erase, }; -static __init int efivars_pstore_init(void) +static int efivars_pstore_init(void) { if (!efivar_supports_writes()) return 0; - if (efivars_pstore_disable) + if (pstore_disable) return 0; /* @@ -250,7 +277,7 @@ static __init int efivars_pstore_init(void) return 0; } -static __exit void efivars_pstore_exit(void) +static void efivars_pstore_exit(void) { if (!efi_pstore_info.bufsize) return; From 98bc7e26e14fbb26a6abf97603d59532475e97f8 Mon Sep 17 00:00:00 2001 From: Kunwu Chan Date: Thu, 18 Jan 2024 18:02:06 +0800 Subject: [PATCH 5/6] pstore/zone: Add a null pointer check to the psz_kmsg_read kasprintf() returns a pointer to dynamically allocated memory which can be NULL upon failure. Ensure the allocation was successful by checking the pointer validity. Signed-off-by: Kunwu Chan Link: https://lore.kernel.org/r/20240118100206.213928-1-chentao@kylinos.cn Signed-off-by: Kees Cook --- fs/pstore/zone.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/fs/pstore/zone.c b/fs/pstore/zone.c index 2770746bb7aa..abca117725c8 100644 --- a/fs/pstore/zone.c +++ b/fs/pstore/zone.c @@ -973,6 +973,8 @@ static ssize_t psz_kmsg_read(struct pstore_zone *zone, char *buf = kasprintf(GFP_KERNEL, "%s: Total %d times\n", kmsg_dump_reason_str(record->reason), record->count); + if (!buf) + return -ENOMEM; hlen = strlen(buf); record->buf = krealloc(buf, hlen + size, GFP_KERNEL); if (!record->buf) { From c8d25d696f526a42ad8cf615dc1131c0b00c662e Mon Sep 17 00:00:00 2001 From: Christophe JAILLET Date: Sat, 9 Mar 2024 09:24:27 +0100 Subject: [PATCH 6/6] pstore/zone: Don't clear memory twice There is no need to call memset(..., 0, ...) on memory allocated by kcalloc(). It is already zeroed. Remove the redundant call. Signed-off-by: Christophe JAILLET Link: https://lore.kernel.org/r/fa2597400051c18c6ca11187b0e4b906729991b2.1709972649.git.christophe.jaillet@wanadoo.fr Signed-off-by: Kees Cook --- fs/pstore/zone.c | 1 - 1 file changed, 1 deletion(-) diff --git a/fs/pstore/zone.c b/fs/pstore/zone.c index abca117725c8..694db616663f 100644 --- a/fs/pstore/zone.c +++ b/fs/pstore/zone.c @@ -1217,7 +1217,6 @@ static struct pstore_zone **psz_init_zones(enum pstore_type_id type, pr_err("allocate for zones %s failed\n", name); return ERR_PTR(-ENOMEM); } - memset(zones, 0, c * sizeof(*zones)); for (i = 0; i < c; i++) { zone = psz_init_zone(type, off, record_size);