Re: [PATCH 6/8] unicode: free the map after a grace period

[Gabriel Krisman Bertazi <krisman@suse.de>]

Christian Brauner <brauner@kernel.org> writes:

> generic_ci_d_hash() and generic_ci_d_compare() read sb->s_encoding and
> dereference the unicode map while a pathwalk in rcu mode holds nothing
> but rcu_read_lock(). The filesystems that use them, ext4, f2fs and
> tmpfs, drop the map with utf8_unload() from ->put_super(), which runs
> when the last reference to the mount goes away. A lookup that is still
> inside such a filesystem at that point reads a freed map.
>
> With a pathwalk held up in utf8_casefold_hash() while the last reference
> to a casefolded tmpfs goes away:
>
>   BUG: KASAN: slab-use-after-free in utf8nlookup+0x74c/0x950
>   Read of size 8 at addr ff110001030fe658 by task walker/98
>   Call Trace:
>    <TASK>
>    utf8nlookup+0x74c/0x950
>    utf8byte+0x160/0x1560
>    utf8_casefold_hash+0x167/0x230
>    generic_ci_d_hash+0xdd/0x1c0
>    link_path_walk+0x11ad/0x1d30
>    path_lookupat+0x63/0xb00
>    filename_lookup+0x1f1/0x5b0
>    vfs_statx+0xd7/0x3b0
>   [...]
>   Allocated by task 94:
>    __kmalloc_cache_noprof+0x249/0x560
>    utf8_load+0x4e/0x570
>    shmem_parse_opt_casefold.isra.0+0x92/0x270
>   [...]
>   Freed by task 97:
>    kfree+0x1db/0x5b0
>    shmem_put_super+0x72/0x170
>    generic_shutdown_super+0x14c/0x530
>    kill_anon_super+0x3a/0x60
>    deactivate_locked_super+0x9c/0xf0
>    cleanup_mnt+0x9f/0x130
>
> So far that has been hard to hit. umount(2) waits for a grace period
> in namespace_unlock() before it drops the mount, so only a mount whose
> last reference goes away elsewhere, like the close of a file or a chdir
> out of a lazily unmounted directory, can race a walker. So it is
> reachable.
>
> Delay the free until a grace period has passed. The map and the data
> table it holds a reference to then outlive any pathwalk that found the
> superblock before the mount went away. The reference on the data table
> module is dropped from the rcu callback as well. utf8-core is built in
> whenever CONFIG_UNICODE is set, so there is no module exit that would
> have to wait for the callbacks.
>
> Overlayfs points the s_encoding of an overlay at the map of its first
> layer without taking a reference of its own. That stays correct. The
> layer is held by the overlay, so its map is dropped only after the
> overlay is gone.
>
> Fixes: b886ee3e778e ("ext4: Support case-insensitive file name lookups")
> Cc: stable@vger.kernel.org
> Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>

Hey, Christian!

Interesting find and looks good to me.  Feel free to add:

Reviewed-by: Gabriel Krisman Bertazi <krisman@suse.de>

thanks,

-- 
Gabriel Krisman Bertazi