On Sun, Oct 27, 2019 at 08:31:49AM -0400, Jeff Layton wrote:
> 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 */
>
Awesome, thanks. Regarding CC: stable@, feel free to add that too. I
guess it makes sense, even if this issue has been there for a while
apparently without any reports. (Although UAFs may have non-obvious
side effects, I guess...)
Cheers,
--
Luís
> Thanks!
> --
> Jeff Layton <jlayton@xxxxxxxxxx>
>
