Received-SPF: pass (google.com: domain of linux-kernel+bounces-188087-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Thu, 23 May 2024 15:48:01 -0700
From: Kees Cook <keescook@chromium.org>
To: Theodore Ts'o <tytso@mit.edu>
Cc: syzbot <syzbot+50835f73143cc2905b9e@syzkaller.appspotmail.com>,
	adilger.kernel@dilger.ca, linux-ext4@vger.kernel.org,
	linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com,
	Justin Stitt <justinstitt@google.com>
Subject: Re: [syzbot] [ext4?] WARNING in __fortify_report
Message-ID: <202405231540.84B3DBE841@keescook>
References: <00000000000019f4c00619192c05@google.com>
 <20240523130456.GH65648@mit.edu>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240523130456.GH65648@mit.edu>

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