On Tue, Jan 16, 2024 at 09:09:47AM +0800, Edward Adam Davis wrote:
> On Mon, 15 Jan 2024 20:08:25 +0100, David Sterba 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > > Signed-off-by: Edward Adam Davis <eadavis@xxxxxx>
> > > > ---
> > > > 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;
> This is not correct,
> 1. The maximum length of tgtdev_name is BTRFS_DEVICE_PATH_NAME_MAX + 1
Yes, so if the array size is N + 1 then writing to offset N is valid.
It's a bit confusing that the paths using BTRFS_PATH_NAME_MAX are +1
bigger so it's not folling the patterns using N as the limit.
> 2. strnlen should be used to confirm the presence of \0 in tgtdev_name
Yes, this would work, with the slight difference that memchr looks for
the 0 character regardless of any other, while strnlen also looks for
any intermediate 0. memchr is an optimization, for input parameter
validation it does not matter.
> 3. Input values should not be subjectively updated
Yeah, this is indeed subjective, I propsed that because we already do
that for subvolume ioctls. This probably would never show up in
practice, the paths are not that long and even if the real linux limit
is PATH_MAX (4096) and BTRFS_PATH_NAME_MAX was originally set to a lower
value it's still enough for everybody.
>From practiacal perspective I don't see any difference between
overwriting the last place of NUL or checking it by strnlen/memchr.
> 4. The current issue only involves tgtdev_name
Right, that needs to be fixed. With bugs like that it's always good to
look around for similar cases or audit everything of similar pattern,
here it's an ioctl taking a user-specified path. If the target path is
fixed, we need the source path fixed too. It can be a separate patch, no
problem.
