btrfs: fix oob Read in getname_kernel

  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(-)
  

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.
  

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,
  

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@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;
This is not correct,
1. The maximum length of tgtdev_name is BTRFS_DEVICE_PATH_NAME_MAX + 1
2. strnlen should be used to confirm the presence of \0 in tgtdev_name
3. Input values should not be subjectively updated
4. The current issue only involves tgtdev_name
> 
>         ret = btrfs_dev_replace_start(fs_info, args->start.tgtdev_name,
>                                         args->start.srcdevid,
  

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@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;
> 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.
  

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@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;
> This is not correct,
> 1. The maximum length of tgtdev_name is BTRFS_DEVICE_PATH_NAME_MAX + 1
> 2. strnlen should be used to confirm the presence of \0 in tgtdev_name
> 3. Input values should not be subjectively updated

Regarding that point I agree it's not the best handling and could be
confusing. There are multiple instances of that in the ioctl callbacks
so the proper fix is to add a helper doing the validity check (either
strnlen or memchr) and then use it.

The pattern to look for is "vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';"
in ioctl.c (at least).

Let me know if you'd want to implement that.