From: ZhaoLong Wang <wangzhaolong1@huawei.com>
To: <richard@nod.at>, <miquel.raynal@bootlin.com>, <vigneshr@ti.com>,
<ada@thorsis.com>, <error27@gmail.com>,
<Artem.Bityutskiy@nokia.com>
Cc: <linux-mtd@lists.infradead.org>, <linux-kernel@vger.kernel.org>,
<chengzhihao1@huawei.com>, <wangzhaolong1@huawei.com>,
<yi.zhang@huawei.com>, <yangerkun@huawei.com>
Subject: [PATCH] ubifs: fix incorrect UBIFS_DFS_DIR_LEN macro definition
Date: Sun, 24 Mar 2024 20:03:33 +0800 [thread overview]
Message-ID: <20240324120333.3837837-1-wangzhaolong1@huawei.com> (raw)
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.
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@huawei.com>
---
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;
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.
*/
--
2.31.1
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
next reply other threads:[~2024-03-24 12:03 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-24 12:03 ZhaoLong Wang [this message]
2024-03-25 6:34 ` [PATCH] ubifs: fix incorrect UBIFS_DFS_DIR_LEN macro definition Dan Carpenter
2024-03-25 8:42 ` ZhaoLong Wang
2024-03-25 6:49 ` Zhihao Cheng
2024-03-25 8:43 ` ZhaoLong Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240324120333.3837837-1-wangzhaolong1@huawei.com \
--to=wangzhaolong1@huawei.com \
--cc=Artem.Bityutskiy@nokia.com \
--cc=ada@thorsis.com \
--cc=chengzhihao1@huawei.com \
--cc=error27@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=miquel.raynal@bootlin.com \
--cc=richard@nod.at \
--cc=vigneshr@ti.com \
--cc=yangerkun@huawei.com \
--cc=yi.zhang@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).