On Fri, 2019-10-25 at 14:05 +0100, Luis Henriques wrote:
> KASAN reports a use-after-free when running xfstest generic/531, with the
> following trace:
>
> [ 293.903362] kasan_report+0xe/0x20
> [ 293.903365] rb_erase+0x1f/0x790
> [ 293.903370] __ceph_remove_cap+0x201/0x370
> [ 293.903375] __ceph_remove_caps+0x4b/0x70
> [ 293.903380] ceph_evict_inode+0x4e/0x360
> [ 293.903386] evict+0x169/0x290
> [ 293.903390] __dentry_kill+0x16f/0x250
> [ 293.903394] dput+0x1c6/0x440
> [ 293.903398] __fput+0x184/0x330
> [ 293.903404] task_work_run+0xb9/0xe0
> [ 293.903410] exit_to_usermode_loop+0xd3/0xe0
> [ 293.903413] do_syscall_64+0x1a0/0x1c0
> [ 293.903417] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> This happens because __ceph_remove_cap() may queue a cap release
> (__ceph_queue_cap_release) which can be scheduled before that cap is
> removed from the inode list with
>
> rb_erase(&cap->ci_node, &ci->i_caps);
>
> And, when this finally happens, the use-after-free will occur.
>
> This can be fixed by removing the cap from the inode list before being
> removed from the session list, and thus eliminating the risk of an UAF.
>
> Signed-off-by: Luis Henriques <lhenriques@xxxxxxxx>
> ---
> Hi!
>
> So, after spending some time trying to find possible races throught code
> review and testing, I modified the fix according to Jeff's suggestion.
>
> Cheers,
> Luis
>
> fs/ceph/caps.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index d3b9c9d5c1bd..a9ce858c37d0 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -1058,6 +1058,11 @@ void __ceph_remove_cap(struct ceph_cap *cap, bool queue_release)
>
> dout("__ceph_remove_cap %p from %p\n", cap, &ci->vfs_inode);
>
> + /* remove from inode list */
> + rb_erase(&cap->ci_node, &ci->i_caps);
> + if (ci->i_auth_cap == cap)
> + ci->i_auth_cap = NULL;
> +
> /* remove from session list */
> spin_lock(&session->s_cap_lock);
> if (session->s_cap_iterator == cap) {
> @@ -1091,11 +1096,6 @@ void __ceph_remove_cap(struct ceph_cap *cap, bool queue_release)
>
> spin_unlock(&session->s_cap_lock);
>
> - /* remove from inode list */
> - rb_erase(&cap->ci_node, &ci->i_caps);
> - if (ci->i_auth_cap == cap)
> - ci->i_auth_cap = NULL;
> -
> if (removed)
> ceph_put_cap(mdsc, cap);
>
Looks good. Merged with a slight modification to the comment:
+ /* remove from inode's cap rbtree, and clear auth cap */
Thanks!
--
Jeff Layton <jlayton@xxxxxxxxxx>
