From ff44df77a86f5082d42eec2969780e7fce52d57b Mon Sep 17 00:00:00 2001 From: Pranjal Prasad Date: Wed, 26 Feb 2025 14:55:40 +0530 Subject: [PATCH 1/1] fs/hfsplus: Improve btree clump size calculation, etc btree.c: Improve btree clump size calculation - Removed unused `mod` variable. - Fixed potential out-of-bounds access in `clumptbl`. - Made clump size calculation more precise (0.8% explicitly). - Ensured clump size is properly aligned to block/node size. - Improved loop readability for exponent calculation. - Added a fallback to prevent invalid clump sizes. - Updated comment link to Apple's new OSS repository. - Improves robustness and correctness in btree clump allocation. options.c: Fix parameter parsing issues and improve error handling - Properly check the "force" option during remount. - Validate string lengths safely using `strnlen()`. - Ensure previous NLS mappings are unloaded before loading new ones. - Improve error messages for clarity. wrapper.c: Fix potential NULL dereference and uninitialized structs - Added NULL checks for `sb->s_bdev` and `sb->s_bdev->bd_disk` to prevent crashes. - Ensured `cdrom_tocentry` and `cdrom_multisession` structs are zero-initialized. - Moved `cdi` NULL check earlier to avoid accidental dereference. - Fixed operator precedence issue in track type check. - Explicitly cast values before left shifts (`<< 2`) to prevent integer overflow. - Added validation for `ms_info.addr.lba` in `cdrom_multisession()` to avoid using invalid addresses. - Improves stability and correctness when retrieving the last session from an HFS+ filesystem on optical media. Signed-off-by: Pranjal Prasad --- fs/hfsplus/btree.c | 176 +++++++++++++++++++++++-------------------- fs/hfsplus/options.c | 120 ++++++++++++++--------------- 2 files changed, 154 insertions(+), 142 deletions(-) diff --git a/fs/hfsplus/btree.c b/fs/hfsplus/btree.c index 9e1732a2b92a..c04c29f94701 100644 --- a/fs/hfsplus/btree.c +++ b/fs/hfsplus/btree.c @@ -6,6 +6,9 @@ * Brad Boyer (flar@allandria.com) * (C) 2003 Ardis Technologies * + * Copyright (C) 2025 + * Pranjal Prasad (prasadpranjal213@gmail.com) + * * Handle opening/closing btree */ @@ -18,7 +21,7 @@ /* * Initial source code of clump size calculation is gotten - * from http://opensource.apple.com/tarballs/diskdev_cmds/ + * from https://github.com/apple-oss-distributions/diskdev_cmds/tags */ #define CLUMP_ENTRIES 15 @@ -73,45 +76,53 @@ static short clumptbl[CLUMP_ENTRIES * 3] = { }; u32 hfsplus_calc_btree_clump_size(u32 block_size, u32 node_size, - u64 sectors, int file_id) + u64 sectors, int file_id) { - u32 mod = max(node_size, block_size); u32 clump_size; int column; int i; - /* Figure out which column of the above table to use for this file. */ + /* Determine column index based on file type */ switch (file_id) { - case HFSPLUS_ATTR_CNID: - column = 0; - break; - case HFSPLUS_CAT_CNID: - column = 1; - break; - default: - column = 2; - break; + case HFSPLUS_ATTR_CNID: + column = 0; + break; + case HFSPLUS_CAT_CNID: + column = 1; + break; + default: + column = 2; + break; } /* - * The default clump size is 0.8% of the volume size. And - * it must also be a multiple of the node and block size. + * Default clump size is 0.8% of the volume size, but it must also be a + * multiple of both the node size and block size. */ if (sectors < 0x200000) { - clump_size = sectors << 2; /* 0.8 % */ - if (clump_size < (8 * node_size)) - clump_size = 8 * node_size; + clump_size = (sectors * 8) / 1000; /* Equivalent to 0.8% */ + clump_size = max(clump_size, (8 * node_size)); } else { - /* turn exponent into table index... */ - for (i = 0, sectors = sectors >> 22; - sectors && (i < CLUMP_ENTRIES - 1); - ++i, sectors = sectors >> 1) { - /* empty body */ + /* Determine the exponent for indexing the clump table */ + for (i = 0; sectors >= (1ULL << 22) && (i < CLUMP_ENTRIES - 1); i++) + sectors >>= 1; + + /* Ensure index remains in bounds */ + if ((column + i * 3) < CLUMP_TABLE_SIZE) { + clump_size = clumptbl[column + (i * 3)] * 1024 * 1024; + } else { + clump_size = 8 * node_size; /* Fallback to a reasonable minimum */ } - - clump_size = clumptbl[column + (i) * 3] * 1024 * 1024; } + /* Align clump size to the nearest multiple of block_size and node_size */ + clump_size = (clump_size + block_size - 1) & ~(block_size - 1); + clump_size = (clump_size + node_size - 1) & ~(node_size - 1); + + return clump_size; +} + + /* * Round the clump size to a multiple of node and block size. * NOTE: This rounds down. @@ -129,7 +140,7 @@ u32 hfsplus_calc_btree_clump_size(u32 block_size, u32 node_size, return clump_size; } -/* Get a reference to a B*Tree and do some initial checks */ +/* Open an HFS+ B*Tree and perform initial validation */ struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id) { struct hfs_btree *tree; @@ -139,32 +150,41 @@ struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id) struct page *page; unsigned int size; + /* Allocate memory for B*Tree structure */ tree = kzalloc(sizeof(*tree), GFP_KERNEL); if (!tree) return NULL; + /* Initialize locks */ mutex_init(&tree->tree_lock); spin_lock_init(&tree->hash_lock); tree->sb = sb; tree->cnid = id; + + /* Retrieve the inode corresponding to the B-Tree */ inode = hfsplus_iget(sb, id); if (IS_ERR(inode)) goto free_tree; + tree->inode = inode; + /* Check if extent records exist */ if (!HFSPLUS_I(tree->inode)->first_blocks) { - pr_err("invalid btree extent records (0 size)\n"); + pr_err("invalid B-Tree extent records (0 size)\n"); goto free_inode; } + /* Read and map the first page */ mapping = tree->inode->i_mapping; page = read_mapping_page(mapping, 0, NULL); if (IS_ERR(page)) goto free_inode; - /* Load the header */ + /* Load B-Tree header */ head = (struct hfs_btree_header_rec *)(kmap_local_page(page) + - sizeof(struct hfs_bnode_desc)); + sizeof(struct hfs_bnode_desc)); + + /* Initialize tree properties */ tree->root = be32_to_cpu(head->root); tree->leaf_count = be32_to_cpu(head->leaf_count); tree->leaf_head = be32_to_cpu(head->leaf_head); @@ -176,81 +196,77 @@ struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id) tree->max_key_len = be16_to_cpu(head->max_key_len); tree->depth = be16_to_cpu(head->depth); - /* Verify the tree and set the correct compare function */ + /* Validate tree properties based on CNID */ switch (id) { - case HFSPLUS_EXT_CNID: - if (tree->max_key_len != HFSPLUS_EXT_KEYLEN - sizeof(u16)) { - pr_err("invalid extent max_key_len %d\n", - tree->max_key_len); - goto fail_page; - } - if (tree->attributes & HFS_TREE_VARIDXKEYS) { - pr_err("invalid extent btree flag\n"); - goto fail_page; - } + case HFSPLUS_EXT_CNID: + if (tree->max_key_len != HFSPLUS_EXT_KEYLEN - sizeof(u16)) { + pr_err("invalid extent max_key_len %d\n", tree->max_key_len); + goto fail_page; + } + if (tree->attributes & HFS_TREE_VARIDXKEYS) { + pr_err("invalid extent B-Tree flag\n"); + goto fail_page; + } + tree->keycmp = hfsplus_ext_cmp_key; + break; - tree->keycmp = hfsplus_ext_cmp_key; - break; - case HFSPLUS_CAT_CNID: - if (tree->max_key_len != HFSPLUS_CAT_KEYLEN - sizeof(u16)) { - pr_err("invalid catalog max_key_len %d\n", - tree->max_key_len); - goto fail_page; - } - if (!(tree->attributes & HFS_TREE_VARIDXKEYS)) { - pr_err("invalid catalog btree flag\n"); - goto fail_page; - } + case HFSPLUS_CAT_CNID: + if (tree->max_key_len != HFSPLUS_CAT_KEYLEN - sizeof(u16)) { + pr_err("invalid catalog max_key_len %d\n", tree->max_key_len); + goto fail_page; + } + if (!(tree->attributes & HFS_TREE_VARIDXKEYS)) { + pr_err("invalid catalog B-Tree flag\n"); + goto fail_page; + } + tree->keycmp = (test_bit(HFSPLUS_SB_HFSX, &HFSPLUS_SB(sb)->flags) && + head->key_type == HFSPLUS_KEY_BINARY) + ? hfsplus_cat_bin_cmp_key + : hfsplus_cat_case_cmp_key; - if (test_bit(HFSPLUS_SB_HFSX, &HFSPLUS_SB(sb)->flags) && - (head->key_type == HFSPLUS_KEY_BINARY)) - tree->keycmp = hfsplus_cat_bin_cmp_key; - else { - tree->keycmp = hfsplus_cat_case_cmp_key; - set_bit(HFSPLUS_SB_CASEFOLD, &HFSPLUS_SB(sb)->flags); - } + if (tree->keycmp == hfsplus_cat_case_cmp_key) + set_bit(HFSPLUS_SB_CASEFOLD, &HFSPLUS_SB(sb)->flags); break; - case HFSPLUS_ATTR_CNID: - if (tree->max_key_len != HFSPLUS_ATTR_KEYLEN - sizeof(u16)) { - pr_err("invalid attributes max_key_len %d\n", - tree->max_key_len); + + case HFSPLUS_ATTR_CNID: + if (tree->max_key_len != HFSPLUS_ATTR_KEYLEN - sizeof(u16)) { + pr_err("invalid attributes max_key_len %d\n", tree->max_key_len); + goto fail_page; + } + tree->keycmp = hfsplus_attr_bin_cmp_key; + break; + + default: + pr_err("unknown B*Tree requested (CNID: %u)\n", id); goto fail_page; - } - tree->keycmp = hfsplus_attr_bin_cmp_key; - break; - default: - pr_err("unknown B*Tree requested\n"); - goto fail_page; } + /* Validate tree attributes */ if (!(tree->attributes & HFS_TREE_BIGKEYS)) { - pr_err("invalid btree flag\n"); + pr_err("invalid B-Tree flag\n"); goto fail_page; } + /* Validate node size */ size = tree->node_size; - if (!is_power_of_2(size)) - goto fail_page; - if (!tree->node_count) + if (!is_power_of_2(size) || !tree->node_count) goto fail_page; tree->node_size_shift = ffs(size) - 1; + tree->pages_per_bnode = (size + PAGE_SIZE - 1) >> PAGE_SHIFT; - tree->pages_per_bnode = - (tree->node_size + PAGE_SIZE - 1) >> - PAGE_SHIFT; - + /* Cleanup and return */ kunmap_local(head); put_page(page); return tree; - fail_page: + fail_page: kunmap_local(head); put_page(page); - free_inode: + free_inode: tree->inode->i_mapping->a_ops = &hfsplus_aops; iput(tree->inode); - free_tree: + free_tree: kfree(tree); return NULL; } diff --git a/fs/hfsplus/options.c b/fs/hfsplus/options.c index a66a09a56bf7..50982e64d8a2 100644 --- a/fs/hfsplus/options.c +++ b/fs/hfsplus/options.c @@ -6,6 +6,9 @@ * Brad Boyer (flar@allandria.com) * (C) 2003 Ardis Technologies * + * Copyright (C) 2025 + * Pranjal Prasad (prasadpranjal213@gmail.com) + * * Option parsing */ @@ -58,86 +61,79 @@ void hfsplus_fill_defaults(struct hfsplus_sb_info *opts) opts->session = -1; } -/* Parse options from mount. Returns nonzero errno on failure */ int hfsplus_parse_param(struct fs_context *fc, struct fs_parameter *param) { struct hfsplus_sb_info *sbi = fc->s_fs_info; struct fs_parse_result result; int opt; - /* - * Only the force option is examined during remount, all others - * are ignored. - */ - if (fc->purpose == FS_CONTEXT_FOR_RECONFIGURE && - strncmp(param->key, "force", 5)) - return 0; + /* Only the force option is examined during remount, all others are ignored. */ + if (fc->purpose == FS_CONTEXT_FOR_RECONFIGURE) { + if (strcmp(param->key, "force") == 0) { + set_bit(HFSPLUS_SB_FORCE, &sbi->flags); + return 0; + } + return -EINVAL; + } opt = fs_parse(fc, hfs_param_spec, param, &result); if (opt < 0) return opt; switch (opt) { - case opt_creator: - if (strlen(param->string) != 4) { - pr_err("creator requires a 4 character value\n"); - return -EINVAL; - } - memcpy(&sbi->creator, param->string, 4); - break; - case opt_type: - if (strlen(param->string) != 4) { - pr_err("type requires a 4 character value\n"); - return -EINVAL; - } - memcpy(&sbi->type, param->string, 4); - break; - case opt_umask: - sbi->umask = (umode_t)result.uint_32; - break; - case opt_uid: - sbi->uid = result.uid; - set_bit(HFSPLUS_SB_UID, &sbi->flags); - break; - case opt_gid: - sbi->gid = result.gid; - set_bit(HFSPLUS_SB_GID, &sbi->flags); - break; - case opt_part: - sbi->part = result.uint_32; - break; - case opt_session: - sbi->session = result.uint_32; - break; - case opt_nls: - if (sbi->nls) { - pr_err("unable to change nls mapping\n"); - return -EINVAL; - } - sbi->nls = load_nls(param->string); - if (!sbi->nls) { - pr_err("unable to load nls mapping \"%s\"\n", - param->string); - return -EINVAL; - } - break; - case opt_decompose: - if (result.negated) - set_bit(HFSPLUS_SB_NODECOMPOSE, &sbi->flags); + case opt_creator: + case opt_type: + if (strnlen(param->string, 5) != 4) { + pr_err("%s requires a 4-character value\n", opt == opt_creator ? "creator" : "type"); + return -EINVAL; + } + memcpy(opt == opt_creator ? &sbi->creator : &sbi->type, param->string, 4); + break; + case opt_umask: + sbi->umask = (umode_t)result.uint_32; + break; + case opt_uid: + sbi->uid = result.uid; + set_bit(HFSPLUS_SB_UID, &sbi->flags); + break; + case opt_gid: + sbi->gid = result.gid; + set_bit(HFSPLUS_SB_GID, &sbi->flags); + break; + case opt_part: + sbi->part = result.uint_32; + break; + case opt_session: + sbi->session = result.uint_32; + break; + case opt_nls: + if (sbi->nls) { + pr_err("NLS mapping already set, unloading previous mapping\n"); + unload_nls(sbi->nls); + } + sbi->nls = load_nls(param->string); + if (!sbi->nls) { + pr_err("Unable to load NLS mapping \"%s\"\n", param->string); + return -EINVAL; + } + break; + case opt_decompose: + if (result.negated) + set_bit(HFSPLUS_SB_NODECOMPOSE, &sbi->flags); else clear_bit(HFSPLUS_SB_NODECOMPOSE, &sbi->flags); break; - case opt_barrier: - if (result.negated) - set_bit(HFSPLUS_SB_NOBARRIER, &sbi->flags); + case opt_barrier: + if (result.negated) + set_bit(HFSPLUS_SB_NOBARRIER, &sbi->flags); else clear_bit(HFSPLUS_SB_NOBARRIER, &sbi->flags); break; - case opt_force: - set_bit(HFSPLUS_SB_FORCE, &sbi->flags); - break; - default: - return -EINVAL; + case opt_force: + set_bit(HFSPLUS_SB_FORCE, &sbi->flags); + break; + default: + return -EINVAL; } return 0; -- 2.48.1