Re: [PATCH] ubifs: fix incorrect UBIFS_DFS_DIR_LEN macro definition

[ZhaoLong Wang]

Thank you for your detailed comments and suggestions.

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

You're right, I shouldn't have mentioned the 5.10 kernel issue in the
commit message. I'll rewrite the commit message to focus only on code
improvements for the current kernel.

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

I also agree with you that the original code, while having incorrect
calculations and comments, still works due to the mathematical errors
canceling out. I'll acknowledge this in the revised commit message.

As for the suggestion to backport your patch to 5.10, I'm all for it.
This will help fix the problem in the old kernel.

Thanks again for the feedback. I'll prepare an updated patch and resend
it as soon as possible.

Best regards,
ZhaoLong Wang