Re: [PATCH] ubifs: fix incorrect UBIFS_DFS_DIR_LEN macro definition
From: Zhihao Cheng
Date: Mon Mar 25 2024 - 09:09:51 EST
在 2024/3/24 20:03, ZhaoLong Wang 写道:
The UBIFS_DFS_DIR_LEN macro, which defines the maximum length of the UBIFS
debugfs directory name, is incorrectly calculated. The current formula is
(3 + 1 + 2*2 + 1), which assumes that both UBI device number and volume ID
are limited to 2 characters. However, UBI device number ranges from 0 to
37 (2 characters), and volume ID ranges from 0 to 127 (up to 3 characters).
This incorrect definition leads to a buffer overflow issue when the device
number is 31 and volume ID is 127, causing the dbg_debugfs_init_fs() function
to return prematurely without creating debugfs directory in the stable branch
linux-5.10.y.
A previous attempt to fix this issue in commit be076fdf8369 ("ubifs: fix
snprintf() checking") by modifying the snprintf return value check range is
insufficient. It avoids the premature function return but does not address
the root cause of the problem. If the buffer length is inadequate, snprintf
will truncate the output string, resulting in incorrect directory names
during filesystem debugging.
I don't think 'snprintf' ever truncated the output string in
dbg_debugfs_init_fs(), even before be076fdf8369 ("ubifs: fix snprintf()
checking"). The 'UBIFS_DFS_DIR_LEN' contains trailing zero byte
according to the comments, but actually all callers treat it as real
string length without '\0' terminated(eg. dbg_debugfs_init_fs,
ubifs_sysfs_register).
So there are no actual problems here. The only problem is that the
comment of 'UBIFS_DFS_DIR_LEN' is not consistent with its' usage, the
simpliest way is modifying comments. If you still want to cleanup the
code, please remove the wrong fixing tags.
The proper solution is to correct the UBIFS_DFS_DIR_LEN macro definition to
(3 + 1 + 2 + 3 + 1), accommodating the maximum lengths of both UBI device
number and volume ID, plus the separators and null terminator.
Additionally, the original equality check for snprintf return value strictly
adheres to the function definition. Although it may seem rigid, it is a good
programming practice to prevent introducing subtle bugs.
This patch makes the following changes:
1. Corrects the UBIFS_DFS_DIR_LEN macro definition to (3 + 1 + 2 + 3 + 1).
2. Updates the snprintf calls to use UBIFS_DFS_DIR_LEN instead of
UBIFS_DFS_DIR_LEN + 1.
3. Modifies the error checks to compare against UBIFS_DFS_DIR_LEN using >=
instead of >.
4. Removes the redundant UBIFS_DFS_DIR_LEN definition in ubifs.h.
5. Updates the relevant comments to reflect the correct maximum length
calculation.
With these changes, the buffer overflow issue is thoroughly resolved, and the
code is made more robust and maintainable.
Fixes: be076fdf8369 ("ubifs: fix snprintf() checking")
Fixes: ae380ce04731 ("UBIFS: lessen the size of debugging info data structure")
Fixes: 2a734bb8d502 ("UBI: use debugfs for the extra checks knobs")
Signed-off-by: ZhaoLong Wang <wangzhaolong1@xxxxxxxxxx>
---
drivers/mtd/ubi/debug.c | 4 ++--
drivers/mtd/ubi/ubi.h | 2 +-
fs/ubifs/debug.c | 4 ++--
fs/ubifs/debug.h | 9 +++++----
fs/ubifs/ubifs.h | 7 -------
5 files changed, 10 insertions(+), 16 deletions(-)
diff --git a/drivers/mtd/ubi/debug.c b/drivers/mtd/ubi/debug.c
index d57f52bd2ff3..9ec3b8b6a0aa 100644
--- a/drivers/mtd/ubi/debug.c
+++ b/drivers/mtd/ubi/debug.c
@@ -598,9 +598,9 @@ int ubi_debugfs_init_dev(struct ubi_device *ubi)
if (!IS_ENABLED(CONFIG_DEBUG_FS))
return 0;
- n = snprintf(d->dfs_dir_name, UBI_DFS_DIR_LEN + 1, UBI_DFS_DIR_NAME,
+ n = snprintf(d->dfs_dir_name, UBI_DFS_DIR_LEN, UBI_DFS_DIR_NAME,
ubi->ubi_num);
- if (n > UBI_DFS_DIR_LEN) {
+ if (n >= UBI_DFS_DIR_LEN) {
/* The array size is too small */
return -EINVAL;
}
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 32009a24869e..da4e53ef5b0a 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -420,7 +420,7 @@ struct ubi_debug_info {
unsigned int power_cut_min;
unsigned int power_cut_max;
unsigned int emulate_failures;
- char dfs_dir_name[UBI_DFS_DIR_LEN + 1];
+ char dfs_dir_name[UBI_DFS_DIR_LEN];
struct dentry *dfs_dir;
struct dentry *dfs_chk_gen;
struct dentry *dfs_chk_io;
diff --git a/fs/ubifs/debug.c b/fs/ubifs/debug.c
index ac77ac1fd73e..d91cec93d968 100644
--- a/fs/ubifs/debug.c
+++ b/fs/ubifs/debug.c
@@ -2827,9 +2827,9 @@ void dbg_debugfs_init_fs(struct ubifs_info *c)
const char *fname;
struct ubifs_debug_info *d = c->dbg;
- n = snprintf(d->dfs_dir_name, UBIFS_DFS_DIR_LEN + 1, UBIFS_DFS_DIR_NAME,
+ n = snprintf(d->dfs_dir_name, UBIFS_DFS_DIR_LEN, UBIFS_DFS_DIR_NAME,
c->vi.ubi_num, c->vi.vol_id);
- if (n > UBIFS_DFS_DIR_LEN) {
+ if (n >= UBIFS_DFS_DIR_LEN) {
/* The array size is too small */
return;
}
diff --git a/fs/ubifs/debug.h b/fs/ubifs/debug.h
index ed966108da80..bbcb2bf41f83 100644
--- a/fs/ubifs/debug.h
+++ b/fs/ubifs/debug.h
@@ -18,11 +18,12 @@ typedef int (*dbg_znode_callback)(struct ubifs_info *c,
struct ubifs_znode *znode, void *priv);
/*
- * The UBIFS debugfs directory name pattern and maximum name length (3 for "ubi"
- * + 1 for "_" and plus 2x2 for 2 UBI numbers and 1 for the trailing zero byte.
+ * The UBIFS sysfs directory name pattern and maximum name length (3 for "ubi"
+ * + 1 for "_" and 2 for UBI device numbers and 3 for volume number and 1 for
+ * the trailing zero byte.
*/
#define UBIFS_DFS_DIR_NAME "ubi%d_%d"
-#define UBIFS_DFS_DIR_LEN (3 + 1 + 2*2 + 1)
+#define UBIFS_DFS_DIR_LEN (3 + 1 + 2 + 3 + 1)
/**
* ubifs_debug_info - per-FS debugging information.
@@ -103,7 +104,7 @@ struct ubifs_debug_info {
unsigned int chk_fs:1;
unsigned int tst_rcvry:1;
- char dfs_dir_name[UBIFS_DFS_DIR_LEN + 1];
+ char dfs_dir_name[UBIFS_DFS_DIR_LEN];
struct dentry *dfs_dir;
struct dentry *dfs_dump_lprops;
struct dentry *dfs_dump_budg;
If you want to clean up code, modifying sysfs related
code(ubifs_sysfs_register) is needed too.
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index 1f3ea879d93a..7b6be3fb4f62 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -157,13 +157,6 @@
#define UBIFS_HMAC_ARR_SZ 0
#endif
-/*
- * The UBIFS sysfs directory name pattern and maximum name length (3 for "ubi"
- * + 1 for "_" and plus 2x2 for 2 UBI numbers and 1 for the trailing zero byte.
- */
-#define UBIFS_DFS_DIR_NAME "ubi%d_%d"
-#define UBIFS_DFS_DIR_LEN (3 + 1 + 2*2 + 1)
-
/*
* Lockdep classes for UBIFS inode @ui_mutex.
*/