Re: [PATCH v2] ceph: fix potential use-after-free bug when trimming caps

[Xiubo Li <xiubli@xxxxxxxxxx>]

On 4/17/23 23:55, Luís Henriques wrote:
> xiubli@xxxxxxxxxx writes:
>
> > From: Xiubo Li <xiubli@xxxxxxxxxx>
> >
> > When trimming the caps and just after the 'session->s_cap_lock' is
> > released in ceph_iterate_session_caps() the cap maybe removed by
> > another thread, and when using the stale cap memory in the callbacks
> > it will trigger use-after-free crash.
> >
> > We need to check the existence of the cap just after the 'ci->i_ceph_lock'
> > being acquired. And do nothing if it's already removed.
> >
> > Cc: stable@xxxxxxxxxxxxxxx
> > URL: https://bugzilla.redhat.com/show_bug.cgi?id=2186264
> I didn't had time to look closer at what this patch is fixing but the
> above URL requires a account to access it.  So I guess it should be
> dropped or replaced by another one from the tracker...?

Make sense.


> Also, just skimming through the patch, there are at least 2 obvious issues
> with it.  See below.
>
> > Signed-off-by: Xiubo Li <xiubli@xxxxxxxxxx>
> > ---
> >
> > V2:
> > - Fix this in ceph_iterate_session_caps instead.
> >
> >
> >   fs/ceph/debugfs.c    |  7 +++++-
> >   fs/ceph/mds_client.c | 56 ++++++++++++++++++++++++++++++--------------
> >   fs/ceph/mds_client.h |  2 +-
> >   3 files changed, 46 insertions(+), 19 deletions(-)
> >
> > diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c
> > index bec3c4549c07..5c0f07df5b02 100644
> > --- a/fs/ceph/debugfs.c
> > +++ b/fs/ceph/debugfs.c
> > @@ -248,14 +248,19 @@ static int metrics_caps_show(struct seq_file *s, void *p)
> >   	return 0;
> >   }
> >   
> > -static int caps_show_cb(struct inode *inode, struct ceph_cap *cap, void *p)
> > +static int caps_show_cb(struct inode *inode, struct rb_node *ci_node, void *p)
> >   {
> > +	struct ceph_inode_info *ci = ceph_inode(inode);
> >   	struct seq_file *s = p;
> > +	struct ceph_cap *cap;
> >   
> > +	spin_lock(&ci->i_ceph_lock);
> > +	cap = rb_entry(ci_node, struct ceph_cap, ci_node);
> >   	seq_printf(s, "0x%-17llx%-3d%-17s%-17s\n", ceph_ino(inode),
> >   		   cap->session->s_mds,
> >   		   ceph_cap_string(cap->issued),
> >   		   ceph_cap_string(cap->implemented));
> > +	spin_unlock(&ci->i_ceph_lock);
> >   	return 0;
> >   }
> >   
> > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > index 294af79c25c9..7fcfbddd534d 100644
> > --- a/fs/ceph/mds_client.c
> > +++ b/fs/ceph/mds_client.c
> > @@ -1786,7 +1786,7 @@ static void cleanup_session_requests(struct ceph_mds_client *mdsc,
> >    * Caller must hold session s_mutex.
> >    */
> >   int ceph_iterate_session_caps(struct ceph_mds_session *session,
> > -			      int (*cb)(struct inode *, struct ceph_cap *,
> > +			      int (*cb)(struct inode *, struct rb_node *ci_node,
> >   					void *), void *arg)
> >   {
> >   	struct list_head *p;
> > @@ -1799,6 +1799,8 @@ int ceph_iterate_session_caps(struct ceph_mds_session *session,
> >   	spin_lock(&session->s_cap_lock);
> >   	p = session->s_caps.next;
> >   	while (p != &session->s_caps) {
> > +		struct rb_node *ci_node;
> > +
> >   		cap = list_entry(p, struct ceph_cap, session_caps);
> >   		inode = igrab(&cap->ci->netfs.inode);
> >   		if (!inode) {
> > @@ -1806,6 +1808,7 @@ int ceph_iterate_session_caps(struct ceph_mds_session *session,
> >   			continue;
> >   		}
> >   		session->s_cap_iterator = cap;
> > +		ci_node = &cap->ci_node;
> >   		spin_unlock(&session->s_cap_lock);
> >   
> >   		if (last_inode) {
> > @@ -1817,7 +1820,7 @@ int ceph_iterate_session_caps(struct ceph_mds_session *session,
> >   			old_cap = NULL;
> >   		}
> >   
> > -		ret = cb(inode, cap, arg);
> > +		ret = cb(inode, ci_node, arg);
> >   		last_inode = inode;
> >   
> >   		spin_lock(&session->s_cap_lock);
> > @@ -1850,17 +1853,22 @@ int ceph_iterate_session_caps(struct ceph_mds_session *session,
> >   	return ret;
> >   }
> >   
> > -static int remove_session_caps_cb(struct inode *inode, struct ceph_cap *cap,
> > +static int remove_session_caps_cb(struct inode *inode, struct rb_node *ci_node,
> >   				  void *arg)
> >   {
> >   	struct ceph_inode_info *ci = ceph_inode(inode);
> >   	bool invalidate = false;
> > +	struct ceph_cap *cap;
> >   	int iputs;
> >   
> > -	dout("removing cap %p, ci is %p, inode is %p\n",
> > -	     cap, ci, &ci->netfs.inode);
> >   	spin_lock(&ci->i_ceph_lock);
> > -	iputs = ceph_purge_inode_cap(inode, cap, &invalidate);
> This will leave iputs uninitialized if the statement below returns NULL.
> Which will cause issues later in the function.

Yeah, correct. It seems some configuration are not enabled when 
compiling the code locally, it doesn't complain about this.


> > +	cap = rb_entry(ci_node, struct ceph_cap, ci_node);
> > +	if (cap) {
> > +		dout(" removing cap %p, ci is %p, inode is %p\n",
> > +		     cap, ci, &ci->netfs.inode);
> > +
> > +		iputs = ceph_purge_inode_cap(inode, cap, &invalidate);
> > +	}
> >   	spin_unlock(&ci->i_ceph_lock);
> >   
> >   	wake_up_all(&ci->i_cap_wq);
> > @@ -1934,11 +1942,11 @@ enum {
> >    *
> >    * caller must hold s_mutex.
> >    */
> > -static int wake_up_session_cb(struct inode *inode, struct ceph_cap *cap,
> > -			      void *arg)
> > +static int wake_up_session_cb(struct inode *inode, struct rb_node *ci_node, void *arg)
> >   {
> >   	struct ceph_inode_info *ci = ceph_inode(inode);
> >   	unsigned long ev = (unsigned long)arg;
> > +	struct ceph_cap *cap;
> >   
> >   	if (ev == RECONNECT) {
> >   		spin_lock(&ci->i_ceph_lock);
> > @@ -1949,7 +1957,9 @@ static int wake_up_session_cb(struct inode *inode, struct ceph_cap *cap,
> >   		if (cap->cap_gen < atomic_read(&cap->session->s_cap_gen)) {
> Since we're replacing the 'cap' argument by the 'ci_node', the
> above statement will have garbage in 'cap'.

Yeah, should check the cap first.

Thanks

- Xiubo


> Cheers,