linux-kernel - Re: [PATCH 03/20] ext4: fix double brelse() the buffer of the extents path

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: <ZqsqZP2Hgx8xTa2i@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com>
Date: Thu, 1 Aug 2024 11:56:07 +0530
From: Ojaswin Mujoo <ojaswin@...ux.ibm.com>
To: Baokun Li <libaokun@...weicloud.com>
Cc: linux-ext4@...r.kernel.org, tytso@....edu, adilger.kernel@...ger.ca,
        jack@...e.cz, ritesh.list@...il.com, linux-kernel@...r.kernel.org,
        yi.zhang@...wei.com, yangerkun@...wei.com,
        Baokun Li <libaokun1@...wei.com>, stable@...nel.org
Subject: Re: [PATCH 03/20] ext4: fix double brelse() the buffer of the
 extents path

On Tue, Jul 30, 2024 at 04:47:56PM +0800, Baokun Li wrote:
> On 2024/7/26 19:45, Ojaswin Mujoo wrote:
> > On Wed, Jul 10, 2024 at 12:06:37PM +0800, libaokun@...weicloud.com wrote:
> > > From: Baokun Li <libaokun1@...wei.com>
> > >  snip
> > Hi Baokun,
> > 
> > If i'm not wrong, in this trace, we'll enter ext4_ext_insert_extent() with
> > gb_flags having EXT4_GET_BLOCKS_PRE_IO so we won't actually go for a
> > merge_up.
> > 
> > That being said, there seems to be a few places where we follow the
> > split-insert pattern and it might be possible that one of those call
> > sites might not be passing EXT4_GET_BLOCKS_PRE_IO and we'll the double
> > free issue you mentioned. I'll check and update if I see anything.
> Hi Ojaswin,
> 
> You're right. I am very sorry for the confusion.
> 
> The trace here is wrong, this patch should actually be placed after the two
> UAF patches. Here ext4_ext_try_to_merge() is called when trying zeroout in
> ext4_split_extent_at(). It is called when trying zeroout with or without
> EXT4_GET_BLOCKS_PRE_IO.The correct trace is as follows:
> 
>   split2    map    split1
> |--------|-------|--------|
> 
> ext4_ext_map_blocks
>  ext4_ext_handle_unwritten_extents
>   ext4_split_convert_extents
>    // path->p_depth == 0
>    ext4_split_extent
>      // 1. do split1
>      ext4_split_extent_at
>        |ext4_ext_insert_extent
>        |  ext4_ext_create_new_leaf
>        |    ext4_ext_grow_indepth
>        |      le16_add_cpu(&neh->eh_depth, 1)
>        |    ext4_find_extent
>        |      // return -ENOMEM
>        |// get error and try zeroout
>        |path = ext4_find_extent
>        |  path->p_depth = 1
>        |ext4_ext_try_to_merge
>        |  ext4_ext_try_to_merge_up
>        |    path->p_depth = 0
>        |    brelse(path[1].p_bh)  ---> not set to NULL here
>        |// zeroout success
>      // 2. update path
>      ext4_find_extent
>      // 3. do split2
>      ext4_split_extent_at
>        ext4_ext_insert_extent
>          ext4_ext_create_new_leaf
>            ext4_ext_grow_indepth
>              le16_add_cpu(&neh->eh_depth, 1)
>            ext4_find_extent
>              path[0].p_bh = NULL;
>              path->p_depth = 1
>              read_extent_tree_block  ---> return err
>              // path[1].p_bh is still the old value
>  ext4_free_ext_path
>   ext4_ext_drop_refs
>    // path->p_depth == 1
>    brelse(path[1].p_bh)  ---> brelse a buffer twice
> 
> I'll adjust the order of the patches and correct the trace in the next
> version.

Hey Baokun,

The corrected trace looks good, thanks.

Regards,
Ojaswin