From df86e81f0a0fdcf958160e6fe3044f69a78df638 Mon Sep 17 00:00:00 2001 From: Alexander Dahl Date: Wed, 3 Jul 2024 12:12:55 +0200 Subject: fs: ubifs: Fix memleak and double free in u-boot wrapper functions When mounting ubifs e.g. through command 'ubifsmount' one global static superblock 'ubifs_sb' is used _and_ the requested volume is opened (like in Linux). The pointer returned by 'ubifs_open_volume()' is stored in that superblock struct and freed later on cmd 'ubifsumount' or another call to 'ubifsmount' with a different volume, through ubifs_umount() and ubi_close_volume(). In ubifs_ls(), ubifs_exists(), ubifs_size(), and ubifs_read() the volume was opened again, which is technically no problem with regard to refcounting, but here the still valid pointer in sb was overwritten, leading to a memory leak. Even worse, when using one of those functions and calling ubifsumount later, ubi_close_volume() was called again but now on an already freed pointer, leading to a double free. This actually crashed with different invalid memory accesses on a board using the old distro boot and a rather long script handling RAUC updates. Example: > ubi part UBI > ubifsmount ubi0:boot > test -e ubi ubi0:boot /boot.scr.uimg > ubifsumount The ubifs specific commands 'ubifsls' and 'ubifsload' check for a mounted volume by themselves, for the generic fs variants 'ls', 'load', (and 'size', and 'test -e') this is covered by special ubifs handling in fs_set_blk_dev() and deeper down blk_get_device_part_str() then. So for ubifs_ls(), ubifs_exists(), ubifs_size(), and ubifs_read() we can be sure the volume is opened and the necessary struct pointer in sb is valid, so it is not needed to open volume again. Fixes: 9eefe2a2b37 ("UBIFS: Implement read-only UBIFS support in U-Boot") Fixes: 29cc5bcadfc ("ubifs: Add functions for generic fs use") Signed-off-by: Alexander Dahl --- fs/ubifs/ubifs.c | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c index 048730db7ff..61ae5580e62 100644 --- a/fs/ubifs/ubifs.c +++ b/fs/ubifs/ubifs.c @@ -614,7 +614,6 @@ int ubifs_set_blk_dev(struct blk_desc *rbdd, struct disk_partition *info) int ubifs_ls(const char *filename) { - struct ubifs_info *c = ubifs_sb->s_fs_info; struct file *file; struct dentry *dentry; struct inode *dir; @@ -622,7 +621,6 @@ int ubifs_ls(const char *filename) unsigned long inum; int ret = 0; - c->ubi = ubi_open_volume(c->vi.ubi_num, c->vi.vol_id, UBI_READONLY); inum = ubifs_findfile(ubifs_sb, (char *)filename); if (!inum) { ret = -1; @@ -656,31 +654,24 @@ out_mem: free(dir); out: - ubi_close_volume(c->ubi); return ret; } int ubifs_exists(const char *filename) { - struct ubifs_info *c = ubifs_sb->s_fs_info; unsigned long inum; - c->ubi = ubi_open_volume(c->vi.ubi_num, c->vi.vol_id, UBI_READONLY); inum = ubifs_findfile(ubifs_sb, (char *)filename); - ubi_close_volume(c->ubi); return inum != 0; } int ubifs_size(const char *filename, loff_t *size) { - struct ubifs_info *c = ubifs_sb->s_fs_info; unsigned long inum; struct inode *inode; int err = 0; - c->ubi = ubi_open_volume(c->vi.ubi_num, c->vi.vol_id, UBI_READONLY); - inum = ubifs_findfile(ubifs_sb, (char *)filename); if (!inum) { err = -1; @@ -698,7 +689,6 @@ int ubifs_size(const char *filename, loff_t *size) ubifs_iput(inode); out: - ubi_close_volume(c->ubi); return err; } @@ -893,7 +883,6 @@ int ubifs_read(const char *filename, void *buf, loff_t offset, return -1; } - c->ubi = ubi_open_volume(c->vi.ubi_num, c->vi.vol_id, UBI_READONLY); /* ubifs_findfile will resolve symlinks, so we know that we get * the real file here */ inum = ubifs_findfile(ubifs_sb, (char *)filename); @@ -957,7 +946,6 @@ put_inode: ubifs_iput(inode); out: - ubi_close_volume(c->ubi); return err; } -- cgit v1.3.1 From 573dae50f5fe2c84ff8329bd8dbf54d234952579 Mon Sep 17 00:00:00 2001 From: Alexander Dahl Date: Wed, 3 Jul 2024 12:12:56 +0200 Subject: fs: ubifs: Set pointers to NULL after free Global superblock pointer 'ubifs_sb' and volume pointer 'ubi' of type struct ubi_volume_desc in private member sb->s_fs_info of type struct ubifs_info, can be allocated and freed at runtime, and allocated and freed again, depending which console or script commands are run. In some cases ubifs_sb is even tested to determine if the filesystem is mounted. Reset those pointers to NULL after free to clearly mark them as not valid. This avoids potential double free on invalid pointers. (The ubifs_sb pointer was already reset, but that statement was moved now to directly after the free() to make it easier to understand.) Signed-off-by: Alexander Dahl --- fs/ubifs/super.c | 4 ++++ fs/ubifs/ubifs.c | 1 - 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c index d8d78a2d3d6..bbbbeb5ee17 100644 --- a/fs/ubifs/super.c +++ b/fs/ubifs/super.c @@ -1758,11 +1758,13 @@ void ubifs_umount(struct ubifs_info *c) ubifs_debugging_exit(c); #ifdef __UBOOT__ ubi_close_volume(c->ubi); + c->ubi = NULL; mutex_unlock(&c->umount_mutex); /* Finally free U-Boot's global copy of superblock */ if (ubifs_sb != NULL) { free(ubifs_sb->s_fs_info); free(ubifs_sb); + ubifs_sb = NULL; } #endif } @@ -2061,6 +2063,7 @@ static void ubifs_put_super(struct super_block *sb) #ifndef __UBOOT__ bdi_destroy(&c->bdi); ubi_close_volume(c->ubi); + c->ubi = NULL; mutex_unlock(&c->umount_mutex); #endif } @@ -2340,6 +2343,7 @@ out_bdi: out_close: #endif ubi_close_volume(c->ubi); + c->ubi = NULL; out: return err; } diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c index 61ae5580e62..6ed9318f739 100644 --- a/fs/ubifs/ubifs.c +++ b/fs/ubifs/ubifs.c @@ -976,6 +976,5 @@ void uboot_ubifs_umount(void) printf("Unmounting UBIFS volume %s!\n", ((struct ubifs_info *)(ubifs_sb->s_fs_info))->vi.name); ubifs_umount(ubifs_sb->s_fs_info); - ubifs_sb = NULL; } } -- cgit v1.3.1 From 0989033d0968878bd8f5d42d4f507dc9a806cfe4 Mon Sep 17 00:00:00 2001 From: Alexander Dahl Date: Wed, 3 Jul 2024 12:12:57 +0200 Subject: fs: ubifs: Make k(z)alloc/kfree symmetric Although kfree() is in fact only a slim wrapper to free() in U-Boot, use kfree() here, because those structs where allocated with kalloc() or kzalloc(). Signed-off-by: Alexander Dahl --- fs/ubifs/super.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c index bbbbeb5ee17..03ed603d0ea 100644 --- a/fs/ubifs/super.c +++ b/fs/ubifs/super.c @@ -1762,8 +1762,8 @@ void ubifs_umount(struct ubifs_info *c) mutex_unlock(&c->umount_mutex); /* Finally free U-Boot's global copy of superblock */ if (ubifs_sb != NULL) { - free(ubifs_sb->s_fs_info); - free(ubifs_sb); + kfree(ubifs_sb->s_fs_info); + kfree(ubifs_sb); ubifs_sb = NULL; } #endif -- cgit v1.3.1 From ca1f11d8c11dacd91c3bfd8e39d41db349e83f8b Mon Sep 17 00:00:00 2001 From: Alexander Dahl Date: Wed, 3 Jul 2024 12:12:58 +0200 Subject: fs: ubifs: Add volume mounted check Safety guard in the U-Boot filesystem glue code, because these functions are called from different parts of the codebase. For generic filesystem handling this should have been checked in blk_get_device_part_str() already. Commands from cmd/ubifs.c should also check this before calling those functions, but you never know?! Signed-off-by: Alexander Dahl --- fs/ubifs/ubifs.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c index 6ed9318f739..8b9bf125ab9 100644 --- a/fs/ubifs/ubifs.c +++ b/fs/ubifs/ubifs.c @@ -621,6 +621,11 @@ int ubifs_ls(const char *filename) unsigned long inum; int ret = 0; + if (!ubifs_is_mounted()) { + debug("UBIFS not mounted, use ubifsmount to mount volume first!\n"); + return -1; + } + inum = ubifs_findfile(ubifs_sb, (char *)filename); if (!inum) { ret = -1; @@ -661,6 +666,11 @@ int ubifs_exists(const char *filename) { unsigned long inum; + if (!ubifs_is_mounted()) { + debug("UBIFS not mounted, use ubifsmount to mount volume first!\n"); + return -1; + } + inum = ubifs_findfile(ubifs_sb, (char *)filename); return inum != 0; @@ -672,6 +682,11 @@ int ubifs_size(const char *filename, loff_t *size) struct inode *inode; int err = 0; + if (!ubifs_is_mounted()) { + debug("UBIFS not mounted, use ubifsmount to mount volume first!\n"); + return -1; + } + inum = ubifs_findfile(ubifs_sb, (char *)filename); if (!inum) { err = -1; @@ -875,6 +890,11 @@ int ubifs_read(const char *filename, void *buf, loff_t offset, int count; int last_block_size = 0; + if (!ubifs_is_mounted()) { + debug("UBIFS not mounted, use ubifsmount to mount volume first!\n"); + return -1; + } + *actread = 0; if (offset & (PAGE_SIZE - 1)) { -- cgit v1.3.1 From cf7ea719ce60cd885cce5023dfaf7833d9fc1442 Mon Sep 17 00:00:00 2001 From: Ravi Minnikanti Date: Tue, 30 Jul 2024 02:14:57 -0700 Subject: ubifs: mount fails after power cycle When kernel uses file system encryption, fscrypt on UBIFS v5, after a hard power cycle UBIFS journal replay fails which results in mount failure. Failure logs: UBIFS: recovery needed UBIFS error (pid 0): ubifs_validate_entry: bad directory entry node UBIFS error (pid 0): replay_bud: bad node is at LEB 890:24576 UBIFS error (pid 0): ubifs_mount: Error reading superblock on volume 'ubi0:rootfs' errno=-22! This change is ported from kernel: commit id: 304790c038bc4af4f19774705409db27eafb09fc Kernel commit description: Kernel commit description: ubifs: Relax checks in ubifs_validate_entry() With encrypted filenames we store raw binary data, doing string tests is no longer possible. Signed-off-by: rminnikanti Reviewed-by: Heiko Schocher --- fs/ubifs/replay.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/ubifs/replay.c b/fs/ubifs/replay.c index aa7f281ef6b..b6e03b76d41 100644 --- a/fs/ubifs/replay.c +++ b/fs/ubifs/replay.c @@ -451,7 +451,7 @@ int ubifs_validate_entry(struct ubifs_info *c, if (le32_to_cpu(dent->ch.len) != nlen + UBIFS_DENT_NODE_SZ + 1 || dent->type >= UBIFS_ITYPES_CNT || nlen > UBIFS_MAX_NLEN || dent->name[nlen] != 0 || - strnlen(dent->name, nlen) != nlen || + (key_type == UBIFS_XENT_KEY && strnlen(dent->name, nlen) != nlen) || le64_to_cpu(dent->inum) > MAX_INUM) { ubifs_err(c, "bad %s node", key_type == UBIFS_DENT_KEY ? "directory entry" : "extended attribute entry"); -- cgit v1.3.1 From d16bda85ff0762aa96e36fa65d2e3b5342d73d7a Mon Sep 17 00:00:00 2001 From: Michael Trimarchi Date: Sat, 10 Aug 2024 14:57:44 +0200 Subject: ubifs: Call ubifs_iput when ubifs_iget is used The inode should be freed after a reference is get to avoid memory leak Tested-by: Alexander Dahl Link: https://lore.kernel.org/u-boot/b698ec3e-d857-6512-8cc9-4edcab0a41b9@denx.de/T/#t Link: https://lore.kernel.org/all/8f3a7059-6330-f332-8e9f-729b853e001e@denx.de/T/ Co-developed-by: Heiko Schocher Signed-off-by: Michael Trimarchi --- fs/ubifs/super.c | 1 + fs/ubifs/ubifs.c | 9 ++++++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c index 03ed603d0ea..7718081f093 100644 --- a/fs/ubifs/super.c +++ b/fs/ubifs/super.c @@ -2324,6 +2324,7 @@ static int ubifs_fill_super(struct super_block *sb, void *data, int silent) goto out_umount; } #else + ubifs_iput(root); sb->s_root = NULL; #endif diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c index 8b9bf125ab9..398b076d783 100644 --- a/fs/ubifs/ubifs.c +++ b/fs/ubifs/ubifs.c @@ -319,9 +319,7 @@ static int filldir(struct ubifs_info *c, const char *name, int namlen, } ctime_r((time_t *)&inode->i_mtime, filetime); printf("%9lld %24.24s ", inode->i_size, filetime); -#ifndef __UBOOT__ ubifs_iput(inode); -#endif printf("%s\n", name); @@ -557,6 +555,7 @@ static unsigned long ubifs_findfile(struct super_block *sb, char *filename) /* We have some sort of symlink recursion, bail out */ if (symlink_count++ > 8) { + ubifs_iput(inode); printf("Symlink recursion, aborting\n"); return 0; } @@ -568,6 +567,7 @@ static unsigned long ubifs_findfile(struct super_block *sb, char *filename) * the leading slash */ next = name = link_name + 1; root_inum = 1; + ubifs_iput(inode); continue; } /* Relative to cur dir */ @@ -575,6 +575,7 @@ static unsigned long ubifs_findfile(struct super_block *sb, char *filename) link_name, next == NULL ? "" : next); memcpy(symlinkpath, buf, sizeof(buf)); next = name = symlinkpath; + ubifs_iput(inode); continue; } @@ -583,8 +584,10 @@ static unsigned long ubifs_findfile(struct super_block *sb, char *filename) */ /* Found the node! */ - if (!next || *next == '\0') + if (!next || *next == '\0') { + ubifs_iput(inode); return inum; + } root_inum = inum; name = next; -- cgit v1.3.1