Received-SPF: pass (google.com: domain of linux-kernel+bounces-116574-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 25 Mar 2024 09:34:20 +0300
From: Dan Carpenter <dan.carpenter@linaro.org>
To: ZhaoLong Wang <wangzhaolong1@huawei.com>
Cc: richard@nod.at, miquel.raynal@bootlin.com, vigneshr@ti.com,
	ada@thorsis.com, error27@gmail.com, Artem.Bityutskiy@nokia.com,
	linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org,
	chengzhihao1@huawei.com, yi.zhang@huawei.com, yangerkun@huawei.com
Subject: Re: [PATCH] ubifs: fix incorrect UBIFS_DFS_DIR_LEN macro definition
Message-ID: <bade7524-2b8f-449a-a29e-cafd11359504@moroto.mountain>
References: <20240324120333.3837837-1-wangzhaolong1@huawei.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240324120333.3837837-1-wangzhaolong1@huawei.com>

On Sun, Mar 24, 2024 at 08:03:33PM +0800, ZhaoLong Wang wrote:
> 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.

This commit message is very confusing because you are talking about
5.10 and the current kernel.  Only talk about the issues in the current
kernel.  Later when we're backporting patches then we can discuss
issues in the old kernels.  (You will need to re-write the commit
message and resend).

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

Heh, my commit message said that my patch did not affect runtime but I
don't know what I was thinking when I wrote that. :P  I guess I saw
UBI_DFS_DIR_NAME but didn't notice it had some %d format strings in it.

I think the calculations were wrong, yes, and the comments that explain
them were wrong, yes.  However, I think that the original code worked.

-	n = snprintf(d->dfs_dir_name, UBI_DFS_DIR_LEN + 1, UBI_DFS_DIR_NAME,
                                      ^^^^^^^^^^^^^^^^^^^
The + 1 was added by mistake, however, 9 + 1 = 10, so in the end the
math errors cancel each other out.

+	n = snprintf(d->dfs_dir_name, UBI_DFS_DIR_LEN, UBI_DFS_DIR_NAME,
                                      ^^^^^^^^^^^^^^^
This is also 10.

 		     ubi->ubi_num);
-	if (n > UBI_DFS_DIR_LEN) {
+	if (n >= UBI_DFS_DIR_LEN) {

n > 9 and n >= 10 are the same.

So I think this is a nice clean up, but I don't think it changes
runtime.  We should backport my patch to 5.10.

regards,
dan carpenter