| Message ID | 20240324120333.3837837-1-wangzhaolong1@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | ubifs: fix incorrect UBIFS_DFS_DIR_LEN macro definition | expand |
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
在 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. > */ >
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
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
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; 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. */
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. 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(-)