| Message ID | 20241025045553.2012160-1-lizhi.xu@windriver.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | btrfs: add a sanity check for btrfs root | expand |
在 2024/10/25 15:25, Lizhi Xu 写道: > Syzbot report a null-ptr-deref in btrfs_search_slot. > It use the input logical can't find the extent root in extent_from_logical, > and triger the null-ptr-deref in btrfs_search_slot. > Add sanity check for btrfs root before using it in btrfs_search_slot. Although I'd prefer to explain a little more about why the NULL root pointer can happen (caused by the rescue=all mount option), which can cause NULL root pointer for non-critical tree roots, like uuid/csum/extent or even device trees. You don't need to bother sending an update. I can add such info when pushing to the maintainer's tree. > > Reported-by: syzbot+3030e17bd57a73d39bd7@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=3030e17bd57a73d39bd7 > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu > --- > fs/btrfs/ctree.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c > index 0cc919d15b14..9c05cab473f5 100644 > --- a/fs/btrfs/ctree.c > +++ b/fs/btrfs/ctree.c > @@ -2010,7 +2010,7 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root, > const struct btrfs_key *key, struct btrfs_path *p, > int ins_len, int cow) > { > - struct btrfs_fs_info *fs_info = root->fs_info; > + struct btrfs_fs_info *fs_info; > struct extent_buffer *b; > int slot; > int ret; > @@ -2023,6 +2023,10 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root, > int min_write_lock_level; > int prev_cmp; > > + if (!root) > + return -EINVAL; > + > + fs_info = root->fs_info; > might_sleep(); > > lowest_level = p->lowest_level;
On Fri, Oct 25, 2024 at 08:23:07PM +1030, Qu Wenruo wrote: > > > 在 2024/10/25 15:25, Lizhi Xu 写道: > > Syzbot report a null-ptr-deref in btrfs_search_slot. > > It use the input logical can't find the extent root in extent_from_logical, > > and triger the null-ptr-deref in btrfs_search_slot. > > Add sanity check for btrfs root before using it in btrfs_search_slot. > > Although I'd prefer to explain a little more about why the NULL root > pointer can happen (caused by the rescue=all mount option), which can > cause NULL root pointer for non-critical tree roots, like > uuid/csum/extent or even device trees. > > You don't need to bother sending an update. > I can add such info when pushing to the maintainer's tree. > > > > > Reported-by: syzbot+3030e17bd57a73d39bd7@syzkaller.appspotmail.com > > Closes: https://syzkaller.appspot.com/bug?extid=3030e17bd57a73d39bd7 > > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com> > > Reviewed-by: Qu Wenruo <wqu@suse.com> > > @@ -2023,6 +2023,10 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root, > > int min_write_lock_level; > > int prev_cmp; > > > > + if (!root) > > + return -EINVAL; The function returns errors indirectly so it's not clear which could be ultimately returned. I did a quick search over the calls starting in btrfs_search_slot() and it seems that EINVAL is not used so we'd know if it ends up in some error report. The ones I found: EAGAIN, EIO, EUCLEAN, ENOMEM.
在 2024/10/26 04:33, David Sterba 写道: > On Fri, Oct 25, 2024 at 08:23:07PM +1030, Qu Wenruo wrote: >> >> >> 在 2024/10/25 15:25, Lizhi Xu 写道: >>> Syzbot report a null-ptr-deref in btrfs_search_slot. >>> It use the input logical can't find the extent root in extent_from_logical, >>> and triger the null-ptr-deref in btrfs_search_slot. >>> Add sanity check for btrfs root before using it in btrfs_search_slot. >> >> Although I'd prefer to explain a little more about why the NULL root >> pointer can happen (caused by the rescue=all mount option), which can >> cause NULL root pointer for non-critical tree roots, like >> uuid/csum/extent or even device trees. >> >> You don't need to bother sending an update. >> I can add such info when pushing to the maintainer's tree. >> >>> >>> Reported-by: syzbot+3030e17bd57a73d39bd7@syzkaller.appspotmail.com >>> Closes: https://syzkaller.appspot.com/bug?extid=3030e17bd57a73d39bd7 >>> Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com> >> >> Reviewed-by: Qu Wenruo <wqu@suse.com> > >>> @@ -2023,6 +2023,10 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root, >>> int min_write_lock_level; >>> int prev_cmp; >>> >>> + if (!root) >>> + return -EINVAL; > > The function returns errors indirectly so it's not clear which could be > ultimately returned. I did a quick search over the calls starting in > btrfs_search_slot() and it seems that EINVAL is not used so we'd know if > it ends up in some error report. The ones I found: EAGAIN, EIO, EUCLEAN, > ENOMEM. > If you want, I can add extra (ratelimited though) error/warning message for such cases. Considering this is only possible for rescue=all cases, extra error messages should be fine. Or do you prefer some more rare return values? Thanks, Qu
On Sat, Oct 26, 2024 at 07:33:59AM +1030, Qu Wenruo wrote: > > > 在 2024/10/26 04:33, David Sterba 写道: > > On Fri, Oct 25, 2024 at 08:23:07PM +1030, Qu Wenruo wrote: > >> > >> > >> 在 2024/10/25 15:25, Lizhi Xu 写道: > >>> Syzbot report a null-ptr-deref in btrfs_search_slot. > >>> It use the input logical can't find the extent root in extent_from_logical, > >>> and triger the null-ptr-deref in btrfs_search_slot. > >>> Add sanity check for btrfs root before using it in btrfs_search_slot. > >> > >> Although I'd prefer to explain a little more about why the NULL root > >> pointer can happen (caused by the rescue=all mount option), which can > >> cause NULL root pointer for non-critical tree roots, like > >> uuid/csum/extent or even device trees. > >> > >> You don't need to bother sending an update. > >> I can add such info when pushing to the maintainer's tree. > >> > >>> > >>> Reported-by: syzbot+3030e17bd57a73d39bd7@syzkaller.appspotmail.com > >>> Closes: https://syzkaller.appspot.com/bug?extid=3030e17bd57a73d39bd7 > >>> Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com> > >> > >> Reviewed-by: Qu Wenruo <wqu@suse.com> > > > >>> @@ -2023,6 +2023,10 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root, > >>> int min_write_lock_level; > >>> int prev_cmp; > >>> > >>> + if (!root) > >>> + return -EINVAL; > > > > The function returns errors indirectly so it's not clear which could be > > ultimately returned. I did a quick search over the calls starting in > > btrfs_search_slot() and it seems that EINVAL is not used so we'd know if > > it ends up in some error report. The ones I found: EAGAIN, EIO, EUCLEAN, > > ENOMEM. > > If you want, I can add extra (ratelimited though) error/warning message > for such cases. > > Considering this is only possible for rescue=all cases, extra error > messages should be fine. > > Or do you prefer some more rare return values? I haven't thought about printing a message, even with rate limiting it could be noisy and I don't see what information would it bring to the user, namely in combination with the rescue=all. The error from search slot can bubble up from various functions, though with the missing csum tree it's probably not that many. The only other error code that's remotely relevant is ENOENT (no such entry), but this is more suitable for entries in structures, not whole structures. Even if we'd use some random error, the string for the error code would be misleading. So let's keep EINVAL for now.
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c index 0cc919d15b14..9c05cab473f5 100644 --- a/fs/btrfs/ctree.c +++ b/fs/btrfs/ctree.c @@ -2010,7 +2010,7 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root, const struct btrfs_key *key, struct btrfs_path *p, int ins_len, int cow) { - struct btrfs_fs_info *fs_info = root->fs_info; + struct btrfs_fs_info *fs_info; struct extent_buffer *b; int slot; int ret; @@ -2023,6 +2023,10 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root, int min_write_lock_level; int prev_cmp; + if (!root) + return -EINVAL; + + fs_info = root->fs_info; might_sleep(); lowest_level = p->lowest_level;
Syzbot report a null-ptr-deref in btrfs_search_slot. It use the input logical can't find the extent root in extent_from_logical, and triger the null-ptr-deref in btrfs_search_slot. Add sanity check for btrfs root before using it in btrfs_search_slot. Reported-by: syzbot+3030e17bd57a73d39bd7@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=3030e17bd57a73d39bd7 Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com> --- fs/btrfs/ctree.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)