On Thu, May 23, 2024 at 09:04:56AM -0400, Theodore Ts'o wrote: > On Wed, May 22, 2024 at 11:29:25PM -0700, syzbot wrote: > > Hello, > > > > syzbot found the following issue on: > > > > dashboard link: https://syzkaller.appspot.com/bug?extid=50835f73143cc2905b9e > > > ... > > strnlen: detected buffer overflow: 17 byte read of buffer size 16 > > [<8080fe10>] (__fortify_report) from [<818e9a40>] (__fortify_panic+0x10/0x14 lib/string_helpers.c:1036) > > [<818e9a30>] (__fortify_panic) from [<8062a3b0>] (strnlen include/linux/fortify-string.h:221 [inline]) > > [<818e9a30>] (__fortify_panic) from [<8062a3b0>] (sized_strscpy include/linux/fortify-string.h:295 [inline]) > > [<818e9a30>] (__fortify_panic) from [<8062a3b0>] (ext4_ioctl_getlabel fs/ext4/ioctl.c:1154 [inline]) > > > [<818e9a30>] (__fortify_panic) from [<8062a3b0>] (ext4_fileattr_get+0x0/0x78 fs/ext4/ioctl.c:1609) > > [<8062829c>] (__ext4_ioctl) from [<8062aaac>] (ext4_ioctl+0x10/0x14 fs/ext4/ioctl.c:1626) > > r10:836e6c00 r9:00000005 r8:845e7900 r7:00000000 r6:845e7900 r5:00000000 > > This is caused by commit 744a56389f73 ("ext4: replace deprecated > strncpy with alternatives") and it's unclear whether this is being > caused by a buggy implementation of strscpy_pad(), or a buggy fortify, > but a simple way to fix is to go back to the good-old strncpy(), which > is perfectly safe, and perfectly secure. > > (And this is a great example of "security initiatives" being an > exercise in pain alocation tradeoffs between overworked maintainers > and security teams... regardless of whether the bug is in fortify, > syzkaller, or an effort to completely convert away from strncpy() > because it makes security analysis easier.) It looks like this is another case of a non-terminated string being made terminated by strncpy into a string with 1 extra byte at the end: char label[EXT4_LABEL_MAX + 1]; ... - memset(label, 0, sizeof(label)); lock_buffer(sbi->s_sbh); - strncpy(label, sbi->s_es->s_volume_name, EXT4_LABEL_MAX); + strscpy_pad(label, sbi->s_es->s_volume_name); unlock_buffer(sbi->s_sbh); This should be using memtostr_pad() as: memtostr_pad(label, sbi->s_es->s_volume_name); I'll send a patch. It looks like __nonstring markings from commit 072ebb3bffe6 ("ext4: add nonstring annotations to ext4.h") were incomplete. -Kees -- Kees Cook
