| Message ID | tencent_44CA0665C9836EF9EEC80CB9E7E206DF5206@qq.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix oob Read in getname_kernel | expand |
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.
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) return -EINVAL; ret = btrfs_dev_replace_start(fs_info, args->start.tgtdev_name,
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(-)