| Message ID | 20190314075041.28966-1-kjlu@umn.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix a NULL pointer dereference | expand |
On 14.03.19 г. 9:50 ч., Kangjie Lu wrote: > btrfs_lookup_block_group may fail and return NULL. The fix goes > to out when it fails to avoid NULL pointer dereference. Actually no, in this case btrfs_lookup_block_group must never fail because if we have an allocated eb then it must have been allocated from a bg. > > Signed-off-by: Kangjie Lu <kjlu@umn.edu> > --- > fs/btrfs/extent-tree.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c > index 994f0cc41799..b1e7985bcb9d 100644 > --- a/fs/btrfs/extent-tree.c > +++ b/fs/btrfs/extent-tree.c > @@ -7303,6 +7303,8 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans, > > pin = 0; > cache = btrfs_lookup_block_group(fs_info, buf->start); > + if (!cache) > + goto out; > > if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) { > pin_down_extent(fs_info, cache, buf->start, >
On 2019/3/14 下午3:50, Kangjie Lu wrote: > btrfs_lookup_block_group may fail and return NULL. The fix goes > to out when it fails to avoid NULL pointer dereference. > > Signed-off-by: Kangjie Lu <kjlu@umn.edu> > --- > fs/btrfs/extent-tree.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c > index 994f0cc41799..b1e7985bcb9d 100644 > --- a/fs/btrfs/extent-tree.c > +++ b/fs/btrfs/extent-tree.c > @@ -7303,6 +7303,8 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans, > > pin = 0; > cache = btrfs_lookup_block_group(fs_info, buf->start); > + if (!cache) > + goto out; The check itself is OK. Reviewed-by: Qu Wenruo <wqu@suse.com> The problem is, here we're freeing a tree block, if there is no block group for it, we shouldn't be able to read the extent buffer out. So it's near impossible to hit. (Unless there is some other things wrong) Thanks, Qu > > if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) { > pin_down_extent(fs_info, cache, buf->start, >
On 2019/3/14 下午3:54, Nikolay Borisov wrote: > > > On 14.03.19 г. 9:50 ч., Kangjie Lu wrote: >> btrfs_lookup_block_group may fail and return NULL. The fix goes >> to out when it fails to avoid NULL pointer dereference. > > Actually no, in this case btrfs_lookup_block_group must never fail > because if we have an allocated eb then it must have been allocated from > a bg. Yep, that's the normal case. However I'm wondering if it's possible to get a bad eb which is cached. Then we could hit such situation. So I still believe being safe here still makes sense, especially who knows future fuzzed image will be. Thanks, Qu > >> >> Signed-off-by: Kangjie Lu <kjlu@umn.edu> >> --- >> fs/btrfs/extent-tree.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c >> index 994f0cc41799..b1e7985bcb9d 100644 >> --- a/fs/btrfs/extent-tree.c >> +++ b/fs/btrfs/extent-tree.c >> @@ -7303,6 +7303,8 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans, >> >> pin = 0; >> cache = btrfs_lookup_block_group(fs_info, buf->start); >> + if (!cache) >> + goto out; >> >> if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) { >> pin_down_extent(fs_info, cache, buf->start, >>
On 14.03.19 г. 10:02 ч., Qu Wenruo wrote: > > > On 2019/3/14 下午3:54, Nikolay Borisov wrote: >> >> >> On 14.03.19 г. 9:50 ч., Kangjie Lu wrote: >>> btrfs_lookup_block_group may fail and return NULL. The fix goes >>> to out when it fails to avoid NULL pointer dereference. >> >> Actually no, in this case btrfs_lookup_block_group must never fail >> because if we have an allocated eb then it must have been allocated from >> a bg. > > Yep, that's the normal case. > > However I'm wondering if it's possible to get a bad eb which is cached. > > Then we could hit such situation. > > So I still believe being safe here still makes sense, especially who > knows future fuzzed image will be. Then I'd rather have ASSERT(cache) > > Thanks, > Qu > >> >>> >>> Signed-off-by: Kangjie Lu <kjlu@umn.edu> >>> --- >>> fs/btrfs/extent-tree.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c >>> index 994f0cc41799..b1e7985bcb9d 100644 >>> --- a/fs/btrfs/extent-tree.c >>> +++ b/fs/btrfs/extent-tree.c >>> @@ -7303,6 +7303,8 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans, >>> >>> pin = 0; >>> cache = btrfs_lookup_block_group(fs_info, buf->start); >>> + if (!cache) >>> + goto out; >>> >>> if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) { >>> pin_down_extent(fs_info, cache, buf->start, >>> >
On 2019/3/14 下午4:03, Nikolay Borisov wrote: > > > On 14.03.19 г. 10:02 ч., Qu Wenruo wrote: >> >> >> On 2019/3/14 下午3:54, Nikolay Borisov wrote: >>> >>> >>> On 14.03.19 г. 9:50 ч., Kangjie Lu wrote: >>>> btrfs_lookup_block_group may fail and return NULL. The fix goes >>>> to out when it fails to avoid NULL pointer dereference. >>> >>> Actually no, in this case btrfs_lookup_block_group must never fail >>> because if we have an allocated eb then it must have been allocated from >>> a bg. >> >> Yep, that's the normal case. >> >> However I'm wondering if it's possible to get a bad eb which is cached. >> >> Then we could hit such situation. >> >> So I still believe being safe here still makes sense, especially who >> knows future fuzzed image will be. > > Then I'd rather have ASSERT(cache) Isn't assert() a bad idea for production build without assert() support? Thanks, Qu > >> >> Thanks, >> Qu >> >>> >>>> >>>> Signed-off-by: Kangjie Lu <kjlu@umn.edu> >>>> --- >>>> fs/btrfs/extent-tree.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c >>>> index 994f0cc41799..b1e7985bcb9d 100644 >>>> --- a/fs/btrfs/extent-tree.c >>>> +++ b/fs/btrfs/extent-tree.c >>>> @@ -7303,6 +7303,8 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans, >>>> >>>> pin = 0; >>>> cache = btrfs_lookup_block_group(fs_info, buf->start); >>>> + if (!cache) >>>> + goto out; >>>> >>>> if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) { >>>> pin_down_extent(fs_info, cache, buf->start, >>>> >>
On 14.03.19 г. 11:15 ч., Qu Wenruo wrote:
> Isn't assert() a bad idea for production build without assert() support?
As already discussed this is invariant of the code, if this invariant is
broken then in production builds we'd likely crash (which is good since
we want to understand why and not silently handle it). In debug builds
the assert will be triggered.
On 2019/3/14 4:02 PM, Qu Wenruo wrote: > > > On 2019/3/14 下午3:54, Nikolay Borisov wrote: >> >> >> On 14.03.19 г. 9:50 ч., Kangjie Lu wrote: >>> btrfs_lookup_block_group may fail and return NULL. The fix goes >>> to out when it fails to avoid NULL pointer dereference. >> >> Actually no, in this case btrfs_lookup_block_group must never fail >> because if we have an allocated eb then it must have been allocated from >> a bg. > > Yep, that's the normal case. > > However I'm wondering if it's possible to get a bad eb which is cached. > > Then we could hit such situation. > > So I still believe being safe here still makes sense, especially who > knows future fuzzed image will be. Plus one. Personally, I'd rather like the version 1. Thanks, Su > > Thanks, > Qu > >> >>> >>> Signed-off-by: Kangjie Lu <kjlu@umn.edu> >>> --- >>> fs/btrfs/extent-tree.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c >>> index 994f0cc41799..b1e7985bcb9d 100644 >>> --- a/fs/btrfs/extent-tree.c >>> +++ b/fs/btrfs/extent-tree.c >>> @@ -7303,6 +7303,8 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans, >>> >>> pin = 0; >>> cache = btrfs_lookup_block_group(fs_info, buf->start); >>> + if (!cache) >>> + goto out; >>> >>> if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) { >>> pin_down_extent(fs_info, cache, buf->start, >>>
On 3/14/19 4:15 AM, Qu Wenruo wrote: > > On 2019/3/14 下午4:03, Nikolay Borisov wrote: >> >> On 14.03.19 г. 10:02 ч., Qu Wenruo wrote: >>> >>> On 2019/3/14 下午3:54, Nikolay Borisov wrote: >>>> >>>> On 14.03.19 г. 9:50 ч., Kangjie Lu wrote: >>>>> btrfs_lookup_block_group may fail and return NULL. The fix goes >>>>> to out when it fails to avoid NULL pointer dereference. >>>> Actually no, in this case btrfs_lookup_block_group must never fail >>>> because if we have an allocated eb then it must have been allocated from >>>> a bg. >>> Yep, that's the normal case. >>> >>> However I'm wondering if it's possible to get a bad eb which is cached. >>> >>> Then we could hit such situation. >>> >>> So I still believe being safe here still makes sense, especially who >>> knows future fuzzed image will be. >> Then I'd rather have ASSERT(cache) > Isn't assert() a bad idea for production build without assert() support? I also agree with that, in general, assert should not be used in production runs. The first patch might be better. > > Thanks, > Qu > >>> Thanks, >>> Qu >>> >>>>> Signed-off-by: Kangjie Lu <kjlu@umn.edu> >>>>> --- >>>>> fs/btrfs/extent-tree.c | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>>> >>>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c >>>>> index 994f0cc41799..b1e7985bcb9d 100644 >>>>> --- a/fs/btrfs/extent-tree.c >>>>> +++ b/fs/btrfs/extent-tree.c >>>>> @@ -7303,6 +7303,8 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans, >>>>> >>>>> pin = 0; >>>>> cache = btrfs_lookup_block_group(fs_info, buf->start); >>>>> + if (!cache) >>>>> + goto out; >>>>> >>>>> if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) { >>>>> pin_down_extent(fs_info, cache, buf->start, >>>>>
On Thu, Mar 14, 2019 at 09:54:07AM +0200, Nikolay Borisov wrote: > > > On 14.03.19 г. 9:50 ч., Kangjie Lu wrote: > > btrfs_lookup_block_group may fail and return NULL. The fix goes > > to out when it fails to avoid NULL pointer dereference. > > Actually no, in this case btrfs_lookup_block_group must never fail > because if we have an allocated eb then it must have been allocated from > a bg. > Agreed, we only get to btrfs_free_tree_block() if we are actually deleting the extent buffer. We would have had to read in the extent buffer first to get here, which would have failed if there was no block group. We can't get into this situation with a specifically crafted file system to exploit this as we'd bail out well before we could get to btrfs_free_tree_block(). Adding an ASSERT() makes sure developers aren't doing anything stupid, but again we'd have to be doing something _super_ stupid to hit it. Thanks, Josef
On Thu, Mar 14, 2019 at 02:50:40AM -0500, Kangjie Lu wrote: > btrfs_lookup_block_group may fail and return NULL. The fix goes > to out when it fails to avoid NULL pointer dereference. The subject, changelog and code change are not valid anymore after the discussion. Please update them accordingly and resend. Thanks.
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index 994f0cc41799..b1e7985bcb9d 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -7303,6 +7303,8 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans, pin = 0; cache = btrfs_lookup_block_group(fs_info, buf->start); + if (!cache) + goto out; if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) { pin_down_extent(fs_info, cache, buf->start,
btrfs_lookup_block_group may fail and return NULL. The fix goes to out when it fails to avoid NULL pointer dereference. Signed-off-by: Kangjie Lu <kjlu@umn.edu> --- fs/btrfs/extent-tree.c | 2 ++ 1 file changed, 2 insertions(+)