diff mbox series

[1/4] fs: ubifs: Fix memleak and double free in u-boot wrapper functions

Message ID 20240703101258.1670825-2-ada@thorsis.com
State Accepted
Commit df86e81f0a0fdcf958160e6fe3044f69a78df638
Delegated to: Heiko Schocher
Headers show
Series fs: ubifs: Fix crash and add safeguards | expand

Commit Message

Alexander Dahl July 3, 2024, 10:12 a.m. UTC 
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 <ada@thorsis.com>
---
 fs/ubifs/ubifs.c | 12 ------------
 1 file changed, 12 deletions(-)
diff mbox series

Patch

diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
index 75de01e95f7..58b3f3659c1 100644
--- a/fs/ubifs/ubifs.c
+++ b/fs/ubifs/ubifs.c
@@ -566,7 +566,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;
@@ -574,7 +573,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;
@@ -608,31 +606,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;
@@ -650,7 +641,6 @@  int ubifs_size(const char *filename, loff_t *size)
 
 	ubifs_iput(inode);
 out:
-	ubi_close_volume(c->ubi);
 	return err;
 }
 
@@ -845,7 +835,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);
@@ -909,7 +898,6 @@  put_inode:
 	ubifs_iput(inode);
 
 out:
-	ubi_close_volume(c->ubi);
 	return err;
 }