Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Paulo Alcantara <pc@cjr.nz>
To:     ChenXiaoSong <chenxiaosong2@huawei.com>, sfrench@samba.org,
        lsahlber@redhat.com
Cc:     linux-cifs@vger.kernel.org, samba-technical@lists.samba.org,
        linux-kernel@vger.kernel.org, chenxiaosong2@huawei.com,
        yi.zhang@huawei.com, zhangxiaoxu5@huawei.com
Subject: Re: [PATCH] cifs: fix use-after-free on the link name
In-Reply-To: <20221102061659.920334-1-chenxiaosong2@huawei.com>
References: <20221102061659.920334-1-chenxiaosong2@huawei.com>
Date:   Thu, 03 Nov 2022 13:35:30 -0300
Message-ID: <875yfw0ze5.fsf@cjr.nz>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

ChenXiaoSong <chenxiaosong2@huawei.com> writes:

> xfstests generic/011 reported use-after-free bug as follows:
>
>   BUG: KASAN: use-after-free in __d_alloc+0x269/0x859
>   Read of size 15 at addr ffff8880078933a0 by task dirstress/952
>
>   CPU: 1 PID: 952 Comm: dirstress Not tainted 6.1.0-rc3+ #77
>   Call Trace:
>    __dump_stack+0x23/0x29
>    dump_stack_lvl+0x51/0x73
>    print_address_description+0x67/0x27f
>    print_report+0x3e/0x5c
>    kasan_report+0x7b/0xa8
>    kasan_check_range+0x1b2/0x1c1
>    memcpy+0x22/0x5d
>    __d_alloc+0x269/0x859
>    d_alloc+0x45/0x20c
>    d_alloc_parallel+0xb2/0x8b2
>    lookup_open+0x3b8/0x9f9
>    open_last_lookups+0x63d/0xc26
>    path_openat+0x11a/0x261
>    do_filp_open+0xcc/0x168
>    do_sys_openat2+0x13b/0x3f7
>    do_sys_open+0x10f/0x146
>    __se_sys_creat+0x27/0x2e
>    __x64_sys_creat+0x55/0x6a
>    do_syscall_64+0x40/0x96
>    entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
>   Allocated by task 952:
>    kasan_save_stack+0x1f/0x42
>    kasan_set_track+0x21/0x2a
>    kasan_save_alloc_info+0x17/0x1d
>    __kasan_kmalloc+0x7e/0x87
>    __kmalloc_node_track_caller+0x59/0x155
>    kstrndup+0x60/0xe6
>    parse_mf_symlink+0x215/0x30b
>    check_mf_symlink+0x260/0x36a
>    cifs_get_inode_info+0x14e1/0x1690
>    cifs_revalidate_dentry_attr+0x70d/0x964
>    cifs_revalidate_dentry+0x36/0x62
>    cifs_d_revalidate+0x162/0x446
>    lookup_open+0x36f/0x9f9
>    open_last_lookups+0x63d/0xc26
>    path_openat+0x11a/0x261
>    do_filp_open+0xcc/0x168
>    do_sys_openat2+0x13b/0x3f7
>    do_sys_open+0x10f/0x146
>    __se_sys_creat+0x27/0x2e
>    __x64_sys_creat+0x55/0x6a
>    do_syscall_64+0x40/0x96
>    entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
>   Freed by task 950:
>    kasan_save_stack+0x1f/0x42
>    kasan_set_track+0x21/0x2a
>    kasan_save_free_info+0x1c/0x34
>    ____kasan_slab_free+0x1c1/0x1d5
>    __kasan_slab_free+0xe/0x13
>    __kmem_cache_free+0x29a/0x387
>    kfree+0xd3/0x10e
>    cifs_fattr_to_inode+0xb6a/0xc8c
>    cifs_get_inode_info+0x3cb/0x1690
>    cifs_revalidate_dentry_attr+0x70d/0x964
>    cifs_revalidate_dentry+0x36/0x62
>    cifs_d_revalidate+0x162/0x446
>    lookup_open+0x36f/0x9f9
>    open_last_lookups+0x63d/0xc26
>    path_openat+0x11a/0x261
>    do_filp_open+0xcc/0x168
>    do_sys_openat2+0x13b/0x3f7
>    do_sys_open+0x10f/0x146
>    __se_sys_creat+0x27/0x2e
>    __x64_sys_creat+0x55/0x6a
>    do_syscall_64+0x40/0x96
>    entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
> When opened a symlink, link name is from 'inode->i_link', but it may be
> reset to a new value when revalidate the dentry. If some processes get the
> link name on the race scenario, then UAF will happen on link name.
>
> Fix this by implementing 'get_link' interface to duplicate the link name.
>
> Fixes: 76894f3e2f71 ("cifs: improve symlink handling for smb2+")
> Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com>
> ---
>  fs/cifs/cifsfs.c | 21 ++++++++++++++++++++-
>  fs/cifs/inode.c  |  5 -----
>  2 files changed, 20 insertions(+), 6 deletions(-)

Good catch.  I was also able to reproduce it with CONFIG_KASAN=y when
running xfstests generic/011 multiple times.

> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index d0b9fec111aa..bb9592594fcc 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -1143,8 +1143,27 @@ const struct inode_operations cifs_file_inode_ops = {
>  	.fiemap = cifs_fiemap,
>  };
>  
> +const char *cifs_get_link(struct dentry *dentry, struct inode *inode,
> +			    struct delayed_call *done)
> +{

Aren't you supposed to return ERR_PTR(-ECHILD) when !dentry
(e.g. rcu-walk mode)?

> +	char *target_path = NULL;
> +
> +	spin_lock(&inode->i_lock);
> +	if (likely(CIFS_I(inode)->symlink_target))
> +		target_path = kstrdup(CIFS_I(inode)->symlink_target,
> +				      GFP_ATOMIC);
> +	spin_unlock(&inode->i_lock);

Can't you get rid of above kmalloc() inside spin lock and do something
like

        target_path = kmalloc(PATH_MAX, GFP_KERNEL);
        if (!target_path)
                return ERR_PTR(-ENOMEM);

        spin_lock(&inode->i_lock);
        if (likely(CIFS_I(inode)->symlink_target))
                strscpy(target_path, CIFS_I(inode)->symlink_target, PATH_MAX);
        else {
                kfree(target_path);
                target_path = ERR_PTR(-EOPNOTSUPP);
        }
        spin_unlock(&inode->i_lock);

        if (!IS_ERR(target_path))
                set_delayed_call(done, kfree_link, target_path);

        return target_path;