From ff44df77a86f5082d42eec2969780e7fce52d57b Mon Sep 17 00:00:00 2001
From: Pranjal Prasad <prasadpranjal213@gmail.com>
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 <prasadpranjal213@gmail.com>
---
fs/hfsplus/btree.c | 176 +++++++++++++++++++++++--------------------
fs/hfsplus/options.c | 120 ++++++++++++++---------------
2 files changed, 154 insertions(+), 142 deletions(-)
@@ -6,6 +6,9 @@
* Brad Boyer (flar@allandria.com)
* (C) 2003 Ardis Technologies <roman@ardistech.com>
*
+ * 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;
}
@@ -6,6 +6,9 @@
* Brad Boyer (flar@allandria.com)
* (C) 2003 Ardis Technologies <roman@ardistech.com>
*
+ * 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
Hi all, This patch improves the robustness and correctness of the HFS+ filesystem implementation by addressing issues in |btree.c| and |options.c|. *Changes in fs/hfsplus/btree.c:* * 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. *Changes in fs/hfsplus/options.c:* * 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. *Summary of Changes* **fs/hfsplus/btree.c | 176 +++++++++++++++++++++++-------------------- fs/hfsplus/options.c | 120 ++++++++++++++--------------- 2 files changed, 154 insertions(+), 142 deletions(-) Please find the patch attached Best Regards, Pranjal Prasad * *