Received-SPF: pass (google.com: domain of linux-kernel+bounces-116605-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Subject: Re: [PATCH] ubifs: fix incorrect UBIFS_DFS_DIR_LEN macro definition
To: ZhaoLong Wang <wangzhaolong1@huawei.com>, <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>,
	<yi.zhang@huawei.com>, <yangerkun@huawei.com>
References: <20240324120333.3837837-1-wangzhaolong1@huawei.com>
From: Zhihao Cheng <chengzhihao1@huawei.com>
Message-ID: <f14013a4-80f1-4796-9f68-92a032dfdb13@huawei.com>
Date: Mon, 25 Mar 2024 14:49:02 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.5.0
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <20240324120333.3837837-1-wangzhaolong1@huawei.com>
Content-Type: text/plain; charset="gbk"; format=flowed
Content-Transfer-Encoding: 8bit

?? 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@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;

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.
>    */
>