| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <f14013a4-80f1-4796-9f68-92a032dfdb13@huawei.com>
Date: Mon, 25 Mar 2024 14:49:02 +0800
From: Zhihao Cheng <chengzhihao1@...wei.com>
To: ZhaoLong Wang <wangzhaolong1@...wei.com>, <richard@....at>,
<miquel.raynal@...tlin.com>, <vigneshr@...com>, <ada@...rsis.com>,
<error27@...il.com>, <Artem.Bityutskiy@...ia.com>
CC: <linux-mtd@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
<yi.zhang@...wei.com>, <yangerkun@...wei.com>
Subject: Re: [PATCH] ubifs: fix incorrect UBIFS_DFS_DIR_LEN macro definition
在 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@...wei.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;
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.
> */
>
Powered by blists - more mailing lists