[PATCH][v3] btrfs: only search for left_info if there is no right_info

[PATCH][v3] btrfs: only search for left_info if there is no right_info

[Josef Bacik]

In try_to_merge_free_space we attempt to find entries to the left and
right of the entry we are adding to see if they can be merged.  We
search for an entry past our current info (saved into right_info), and
then if right_info exists and it has a rb_prev() we save the rb_prev()
into left_info.

However there's a slight problem in the case that we have a right_info,
but no entry previous to that entry.  At that point we will search for
an entry just before the info we're attempting to insert.  This will
simply find right_info again, and assign it to left_info, making them
both the same pointer.

Now if right_info _can_ be merged with the range we're inserting, we'll
add it to the info and free right_info.  However further down we'll
access left_info, which was right_info, and thus get a UAF.

Fix this by only searching for the left entry if we don't find a right
entry at all.

The CVE referenced had a specially crafted file system that could
trigger this UAF.  However with the tree checker improvements we no
longer trigger the conditions for the UAF.  But the original conditions
still apply, hence this fix.

Reference: CVE-2019-19448
Fixes: 963030817060 ("Btrfs: use hybrid extents+bitmap rb tree for free space")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
v2->v3:
- Updated the changelog.

 fs/btrfs/free-space-cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
index 6d961e11639e..37fd2fa1ac1f 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -2298,7 +2298,7 @@ static bool try_merge_free_space(struct btrfs_free_space_ctl *ctl,
 	if (right_info && rb_prev(&right_info->offset_index))
 		left_info = rb_entry(rb_prev(&right_info->offset_index),
 				     struct btrfs_free_space, offset_index);
-	else
+	else if (!right_info)
 		left_info = tree_search_offset(ctl, offset - 1, 0, 0);
 
 	/* See try_merge_free_space() comment. */
-- 
2.24.1

Re: [PATCH][v3] btrfs: only search for left_info if there is no right_info

[David Sterba]

On Mon, Jul 27, 2020 at 10:28:05AM -0400, Josef Bacik wrote:
> In try_to_merge_free_space we attempt to find entries to the left and
> right of the entry we are adding to see if they can be merged.  We
> search for an entry past our current info (saved into right_info), and
> then if right_info exists and it has a rb_prev() we save the rb_prev()
> into left_info.
> 
> However there's a slight problem in the case that we have a right_info,
> but no entry previous to that entry.  At that point we will search for
> an entry just before the info we're attempting to insert.  This will
> simply find right_info again, and assign it to left_info, making them
> both the same pointer.
> 
> Now if right_info _can_ be merged with the range we're inserting, we'll
> add it to the info and free right_info.  However further down we'll
> access left_info, which was right_info, and thus get a UAF.
> 
> Fix this by only searching for the left entry if we don't find a right
> entry at all.
> 
> The CVE referenced had a specially crafted file system that could
> trigger this UAF.  However with the tree checker improvements we no
> longer trigger the conditions for the UAF.  But the original conditions
> still apply, hence this fix.
> 
> Reference: CVE-2019-19448
> Fixes: 963030817060 ("Btrfs: use hybrid extents+bitmap rb tree for free space")
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
> v2->v3:
> - Updated the changelog.

Added to misc-next, thanks.

Re: [PATCH][v3] btrfs: only search for left_info if there is no right_info

[Sebastian Döring]

For reasons unrelated to btrfs I've been trying linux-next-20200728 today.

This patch causes Kernel Oops and call trace (with
try_merge_free_space on top of stack) on my system. Because of
immediate system lock-up I can't provide a dmesg log and there's
nothing in /var/log (probably because it immediately goes read-only),
but removing this patch and rebuilding the kernel fixed my issues. I'm
happy to help if you need more info in order to reproduce.

Am Di., 28. Juli 2020 um 16:47 Uhr schrieb David Sterba <dsterba@suse.cz>:
>
> On Mon, Jul 27, 2020 at 10:28:05AM -0400, Josef Bacik wrote:
> > In try_to_merge_free_space we attempt to find entries to the left and
> > right of the entry we are adding to see if they can be merged.  We
> > search for an entry past our current info (saved into right_info), and
> > then if right_info exists and it has a rb_prev() we save the rb_prev()
> > into left_info.
> >
> > However there's a slight problem in the case that we have a right_info,
> > but no entry previous to that entry.  At that point we will search for
> > an entry just before the info we're attempting to insert.  This will
> > simply find right_info again, and assign it to left_info, making them
> > both the same pointer.
> >
> > Now if right_info _can_ be merged with the range we're inserting, we'll
> > add it to the info and free right_info.  However further down we'll
> > access left_info, which was right_info, and thus get a UAF.
> >
> > Fix this by only searching for the left entry if we don't find a right
> > entry at all.
> >
> > The CVE referenced had a specially crafted file system that could
> > trigger this UAF.  However with the tree checker improvements we no
> > longer trigger the conditions for the UAF.  But the original conditions
> > still apply, hence this fix.
> >
> > Reference: CVE-2019-19448
> > Fixes: 963030817060 ("Btrfs: use hybrid extents+bitmap rb tree for free space")
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > ---
> > v2->v3:
> > - Updated the changelog.
>
> Added to misc-next, thanks.

Re: [PATCH][v3] btrfs: only search for left_info if there is no right_info

[Josef Bacik]

On 7/29/20 11:42 AM, Sebastian Döring wrote:
> For reasons unrelated to btrfs I've been trying linux-next-20200728 today.
> 
> This patch causes Kernel Oops and call trace (with
> try_merge_free_space on top of stack) on my system. Because of
> immediate system lock-up I can't provide a dmesg log and there's
> nothing in /var/log (probably because it immediately goes read-only),
> but removing this patch and rebuilding the kernel fixed my issues. I'm
> happy to help if you need more info in order to reproduce.
> 

Lol I literally just hit this and sent the fixup to Dave when you posted this. 
My bad, somehow it didn't hit either of us until just now.  Thanks,

Josef

Re: [PATCH][v3] btrfs: only search for left_info if there is no right_info

[David Sterba]

On Wed, Jul 29, 2020 at 11:43:40AM -0400, Josef Bacik wrote:
> On 7/29/20 11:42 AM, Sebastian Döring wrote:
> > For reasons unrelated to btrfs I've been trying linux-next-20200728 today.
> > 
> > This patch causes Kernel Oops and call trace (with
> > try_merge_free_space on top of stack) on my system. Because of
> > immediate system lock-up I can't provide a dmesg log and there's
> > nothing in /var/log (probably because it immediately goes read-only),
> > but removing this patch and rebuilding the kernel fixed my issues. I'm
> > happy to help if you need more info in order to reproduce.
> > 
> 
> Lol I literally just hit this and sent the fixup to Dave when you posted this. 
> My bad, somehow it didn't hit either of us until just now.  Thanks,

Updated misc-next pushed, for-next will follow.

Re: [PATCH][v3] btrfs: only search for left_info if there is no right_info

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1207 bytes --]



On 2020/7/30 上午12:13, David Sterba wrote:
> On Wed, Jul 29, 2020 at 11:43:40AM -0400, Josef Bacik wrote:
>> On 7/29/20 11:42 AM, Sebastian Döring wrote:
>>> For reasons unrelated to btrfs I've been trying linux-next-20200728 today.
>>>
>>> This patch causes Kernel Oops and call trace (with
>>> try_merge_free_space on top of stack) on my system. Because of
>>> immediate system lock-up I can't provide a dmesg log and there's
>>> nothing in /var/log (probably because it immediately goes read-only),
>>> but removing this patch and rebuilding the kernel fixed my issues. I'm
>>> happy to help if you need more info in order to reproduce.
>>>
>>
>> Lol I literally just hit this and sent the fixup to Dave when you posted this. 
>> My bad, somehow it didn't hit either of us until just now.  Thanks,
> 
> Updated misc-next pushed, for-next will follow.
> 
I guess it's still not working...

The latest commit 2f0cb6b46a28 ("btrfs: only search for left_info if
there is no right_info in try_merge_free_space"), shows it's now the
updated one.

But still fails at selftest:
https://paste.opensuse.org/41470779

Have to revert that commit to do my test...

Thanks,
Qu


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH][v3] btrfs: only search for left_info if there is no right_info

[Josef Bacik]

On 7/30/20 7:42 AM, Qu Wenruo wrote:
> 
> 
> On 2020/7/30 上午12:13, David Sterba wrote:
>> On Wed, Jul 29, 2020 at 11:43:40AM -0400, Josef Bacik wrote:
>>> On 7/29/20 11:42 AM, Sebastian Döring wrote:
>>>> For reasons unrelated to btrfs I've been trying linux-next-20200728 today.
>>>>
>>>> This patch causes Kernel Oops and call trace (with
>>>> try_merge_free_space on top of stack) on my system. Because of
>>>> immediate system lock-up I can't provide a dmesg log and there's
>>>> nothing in /var/log (probably because it immediately goes read-only),
>>>> but removing this patch and rebuilding the kernel fixed my issues. I'm
>>>> happy to help if you need more info in order to reproduce.
>>>>
>>>
>>> Lol I literally just hit this and sent the fixup to Dave when you posted this.
>>> My bad, somehow it didn't hit either of us until just now.  Thanks,
>>
>> Updated misc-next pushed, for-next will follow.
>>
> I guess it's still not working...
> 
> The latest commit 2f0cb6b46a28 ("btrfs: only search for left_info if
> there is no right_info in try_merge_free_space"), shows it's now the
> updated one.
> 
> But still fails at selftest:
> https://paste.opensuse.org/41470779
> 
> Have to revert that commit to do my test...
> 

I'm looking at misc-next and the commit is

c5f239232fbe749042e05cae508e1c514ed5bd3c

Do you have a

struct btrfs_free_space *left_info = NULL;

in your tree?  Thanks,

Josef

Re: [PATCH][v3] btrfs: only search for left_info if there is no right_info

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1632 bytes --]



On 2020/7/30 下午10:02, Josef Bacik wrote:
> On 7/30/20 7:42 AM, Qu Wenruo wrote:
>>
>>
>> On 2020/7/30 上午12:13, David Sterba wrote:
>>> On Wed, Jul 29, 2020 at 11:43:40AM -0400, Josef Bacik wrote:
>>>> On 7/29/20 11:42 AM, Sebastian Döring wrote:
>>>>> For reasons unrelated to btrfs I've been trying linux-next-20200728
>>>>> today.
>>>>>
>>>>> This patch causes Kernel Oops and call trace (with
>>>>> try_merge_free_space on top of stack) on my system. Because of
>>>>> immediate system lock-up I can't provide a dmesg log and there's
>>>>> nothing in /var/log (probably because it immediately goes read-only),
>>>>> but removing this patch and rebuilding the kernel fixed my issues. I'm
>>>>> happy to help if you need more info in order to reproduce.
>>>>>
>>>>
>>>> Lol I literally just hit this and sent the fixup to Dave when you
>>>> posted this.
>>>> My bad, somehow it didn't hit either of us until just now.  Thanks,
>>>
>>> Updated misc-next pushed, for-next will follow.
>>>
>> I guess it's still not working...
>>
>> The latest commit 2f0cb6b46a28 ("btrfs: only search for left_info if
>> there is no right_info in try_merge_free_space"), shows it's now the
>> updated one.
>>
>> But still fails at selftest:
>> https://paste.opensuse.org/41470779
>>
>> Have to revert that commit to do my test...
>>
> 
> I'm looking at misc-next and the commit is
> 
> c5f239232fbe749042e05cae508e1c514ed5bd3c
> 
> Do you have a
> 
> struct btrfs_free_space *left_info = NULL;
> 
> in your tree?  Thanks,
> 
> Josef

Now it's there and no longer crash.

Thanks,
Qu


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]