Re: [syzbot] [ext4?] WARNING in __fortify_report

[Kees Cook]

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