linux-kernel - Re: [PATCH] ubifs: fix incorrect UBIFS_DFS_DIR

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Mon, 25 Mar 2024 16:43:10 +0800
From: ZhaoLong Wang <wangzhaolong1@...wei.com>
To: Zhihao Cheng <chengzhihao1@...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

Thank you very much for your comments and suggestions.

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

Regarding my original commit message, I realize that the statement "If 
the buffer length is inadequate, snprintf will truncate the output 
string, resulting in incorrect directory names during filesystem 
debugging." is inaccurate.

`snprintf` does indeed stop writing when it reaches the specified buffer 
size and appends a null character (`'\0'`) after the last character. 
However, since the buffer size passed to `snprintf` is sufficiently 
large, the directory names are not actually truncated in the buffer.

 > If you want to clean up code, modifying sysfs related 
code(ubifs_sysfs_register) is needed too.

That's a good suggestion, I'll go through that part of the code and make
the necessary changes for consistency.

Thanks again for your valuable feedback. I'll take all your suggestions
into consideration and adjust my patch accordingly. I'll resend the
patch once I have an updated version ready.

Best regards,
ZhaoLong Wang