linux-kernel - Re: [PATCH] btrfs: add a sanity check for btrfs root

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241025222205.GN31418@twin.jikos.cz>
Date: Sat, 26 Oct 2024 00:22:05 +0200
From: David Sterba <dsterba@...e.cz>
To: Qu Wenruo <quwenruo.btrfs@....com>
Cc: dsterba@...e.cz, Lizhi Xu <lizhi.xu@...driver.com>,
	syzbot+3030e17bd57a73d39bd7@...kaller.appspotmail.com, clm@...com,
	dsterba@...e.com, josef@...icpanda.com, linux-btrfs@...r.kernel.org,
	linux-kernel@...r.kernel.org, syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH] btrfs: add a sanity check for btrfs root

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@...kaller.appspotmail.com
> >>> Closes: https://syzkaller.appspot.com/bug?extid=3030e17bd57a73d39bd7
> >>> Signed-off-by: Lizhi Xu <lizhi.xu@...driver.com>
> >>
> >> Reviewed-by: Qu Wenruo <wqu@...e.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.