[v2] ceph: Fix use-after-free in __ceph_remove_cap
diff mbox series

Message ID 20191025130524.31755-1-lhenriques@suse.com
State New
Headers show
Series
  • [v2] ceph: Fix use-after-free in __ceph_remove_cap
Related show

Commit Message

Luis Henriques Oct. 25, 2019, 1:05 p.m. UTC
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@suse.com>
---
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(-)

Comments

Jeff Layton Oct. 27, 2019, 12:31 p.m. UTC | #1
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@suse.com>
> ---
> 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!
Luis Henriques Oct. 28, 2019, 9:18 a.m. UTC | #2
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@suse.com>
> > ---
> > 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@kernel.org>
>

Patch
diff mbox series

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);