Received-SPF: pass (google.com: domain of linux-kernel+bounces-26423-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 15 Jan 2024 20:08:25 +0100
From: David Sterba <dsterba@suse.cz>
To: Edward Adam Davis <eadavis@qq.com>
Cc: David Sterba <dsterba@suse.cz>,
	syzbot+33f23b49ac24f986c9e8@syzkaller.appspotmail.com, clm@fb.com,
	daniel@iogearbox.net, dsterba@suse.com, john.fastabend@gmail.com,
	josef@toxicpanda.com, linux-btrfs@vger.kernel.org,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	liujian56@huawei.com, syzkaller-bugs@googlegroups.com
Subject: Re: [PATCH] btrfs: fix oob Read in getname_kernel
Message-ID: <20240115190824.GV31555@twin.jikos.cz>
Reply-To: dsterba@suse.cz
References: <tencent_44CA0665C9836EF9EEC80CB9E7E206DF5206@qq.com>
 <20240110155545.GW28693@twin.jikos.cz>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240110155545.GW28693@twin.jikos.cz>
User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12)

On Wed, Jan 10, 2024 at 04:55:46PM +0100, David Sterba wrote:
> On Tue, Dec 19, 2023 at 06:19:10PM +0800, Edward Adam Davis wrote:
> > If ioctl does not pass in the correct tgtdev_name string, oob will occur because
> > "\0" cannot be found.
> > 
> > Reported-and-tested-by: syzbot+33f23b49ac24f986c9e8@syzkaller.appspotmail.com
> > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > ---
> >  fs/btrfs/dev-replace.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> > index f9544fda38e9..e7e96e57f682 100644
> > --- a/fs/btrfs/dev-replace.c
> > +++ b/fs/btrfs/dev-replace.c
> > @@ -730,7 +730,7 @@ static int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
> >  int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
> >  			    struct btrfs_ioctl_dev_replace_args *args)
> >  {
> > -	int ret;
> > +	int ret, len;
> >  
> >  	switch (args->start.cont_reading_from_srcdev_mode) {
> >  	case BTRFS_IOCTL_DEV_REPLACE_CONT_READING_FROM_SRCDEV_MODE_ALWAYS:
> > @@ -740,8 +740,10 @@ int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
> >  		return -EINVAL;
> >  	}
> >  
> > +	len = strnlen(args->start.tgtdev_name, BTRFS_DEVICE_PATH_NAME_MAX + 1);
> >  	if ((args->start.srcdevid == 0 && args->start.srcdev_name[0] == '\0') ||
> > -	    args->start.tgtdev_name[0] == '\0')
> > +	    args->start.tgtdev_name[0] == '\0' ||
> > +	    len == BTRFS_DEVICE_PATH_NAME_MAX + 1)
> 
> I think srcdev_name would have to be checked the same way, but instead
> of strnlen I'd do memchr(name, 0, BTRFS_DEVICE_PATH_NAME_MAX). The check
> for 0 in [0] is probably pointless, it's just a shortcut for an empty
> buffer. We expect a valid 0-terminated string, which could be an invalid
> path but that will be found out later when opening the block device.

Please let me know if you're going to send an updated fix. I'd like to
get this fixed to close the syzbot report but also want to give you the
credit for debugging and fix.

The preferred fix is something like that:

--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -741,6 +741,8 @@ int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
        if ((args->start.srcdevid == 0 && args->start.srcdev_name[0] == '\0') ||
            args->start.tgtdev_name[0] == '\0')
                return -EINVAL;
+       args->start.srcdev_name[BTRFS_PATH_NAME_MAX] = 0;
+       args->start.tgtdev_name[BTRFS_PATH_NAME_MAX] = 0;
 
        ret = btrfs_dev_replace_start(fs_info, args->start.tgtdev_name,
                                        args->start.srcdevid,