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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Apr 20, 2023 at 3:22 PM Luís Henriques <lhenriques@xxxxxxx> wrote:
>
> Xiubo Li <xiubli@xxxxxxxxxx> writes:
>
> > On 4/18/23 22:20, 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.
> >> Your patch seems to be OK, but I'll be honest: the locking is *so* complex
> >> that I can say for sure it really solves any problem :-(
> >>
> >> ceph_put_cap() uses mdsc->caps_list_lock to protect the list, but I can't
> >> be sure that holding ci->i_ceph_lock will protect a race in the case
> >> you're trying to solve.
> >
> > The 'mdsc->caps_list_lock' will protect the members in mdsc:
> >
> >         /*
> >          * Cap reservations
> >          *
> >          * Maintain a global pool of preallocated struct ceph_caps, referenced
> >          * by struct ceph_caps_reservations.  This ensures that we preallocate
> >          * memory needed to successfully process an MDS response. (If an MDS
> >          * sends us cap information and we fail to process it, we will have
> >          * problems due to the client and MDS being out of sync.)
> >          *
> >          * Reservations are 'owned' by a ceph_cap_reservation context.
> >          */
> >         spinlock_t      caps_list_lock;
> >         struct          list_head caps_list; /* unused (reserved or
> >                                                 unreserved) */
> >         struct          list_head cap_wait_list;
> >         int             caps_total_count;    /* total caps allocated */
> >         int             caps_use_count;      /* in use */
> >         int             caps_use_max;        /* max used caps */
> >         int             caps_reserve_count;  /* unused, reserved */
> >         int             caps_avail_count;    /* unused, unreserved */
> >         int             caps_min_count;      /* keep at least this many
> >
> > Not protecting the cap list in session or inode.
> >
> >
> > And the racy is between the session's cap list and inode's cap rbtree and both
> > are holding the same 'cap' reference.
> >
> > So in 'ceph_iterate_session_caps()' after getting the 'cap' and releasing the
> > 'session->s_cap_lock', just before passing the 'cap' to _cb() another thread
> > could continue and release the 'cap'. Then the 'cap' should be stale now and
> > after being passed to _cb() the 'cap' when dereferencing it will crash the
> > kernel.
> >
> > And if the 'cap' is stale, it shouldn't exist in the inode's cap rbtree. Please
> > note the lock order will be:
> >
> > 1, spin_lock(&ci->i_ceph_lock)
> >
> > 2, spin_lock(&session->s_cap_lock)
> >
> >
> > Before:
> >
> > ThreadA: ThreadB:
> >
> > __ceph_remove_caps() -->
> >
> >     spin_lock(&ci->i_ceph_lock)
> >
> >     ceph_remove_cap() --> ceph_iterate_session_caps() -->
> >
> >         __ceph_remove_cap() --> spin_lock(&session->s_cap_lock);
> >
> > cap = list_entry(p, struct ceph_cap, session_caps);
> >
> > spin_unlock(&session->s_cap_lock);
> >
> >             spin_lock(&session->s_cap_lock);
> >
> >             // remove it from the session's cap list
> >
> >             list_del_init(&cap->session_caps);
> >
> >             spin_unlock(&session->s_cap_lock);
> >
> >             ceph_put_cap()
> >
> > trim_caps_cb('cap') -->   // the _cb() could be deferred after ThreadA finished
> > 'ceph_put_cap()'
> >
> > spin_unlock(&ci->i_ceph_lock) dreference cap->xxx will trigger crash
> >
> >
> >
> > With this patch:
> >
> > ThreadA: ThreadB:
> >
> > __ceph_remove_caps() -->
> >
> >     spin_lock(&ci->i_ceph_lock)
> >
> >     ceph_remove_cap() --> ceph_iterate_session_caps() -->
> >
> >         __ceph_remove_cap() --> spin_lock(&session->s_cap_lock);
> >
> > cap = list_entry(p, struct ceph_cap, session_caps);
> >
> > ci_node = &cap->ci_node;
> >
> > spin_unlock(&session->s_cap_lock);
> >
> >             spin_lock(&session->s_cap_lock);
> >
> >             // remove it from the session's cap list
> >
> >             list_del_init(&cap->session_caps);
> >
> >             spin_unlock(&session->s_cap_lock);
> >
> >             ceph_put_cap()
> >
> > trim_caps_cb('ci_node') -->
> >
> > spin_unlock(&ci->i_ceph_lock)
> >
> > spin_lock(&ci->i_ceph_lock)
> >
> > cap = rb_entry(ci_node, struct ceph_cap, ci_node);    // This is buggy in this
> > version, we should use the 'mds' instead and I will fix it.
> >
> > if (!cap)  { release the spin lock and return directly }
> >
> > spin_unlock(&ci->i_ceph_lock)
>
> Thanks a lot for taking the time to explain all of this.  Much
> appreciated.  It all seems to make sense, and, again, I don't have any
> real objection to your patch.  It's just that I still find the whole
> locking to be too complex, and every change that is made to it looks like
> walking on a mine field :-)
>
> > While we should switch to use the 'mds' of the cap instead of the 'ci_node',
> > which is buggy. I will fix it in next version.
>
> Yeah, I've took a quick look at v4 and it looks like it fixes this.

Hi Luís,

Do you mind if I put this down as a Reviewed-by? ;)

Thanks,

                Ilya




[Index of Archives]     [CEPH Users]     [Ceph Large]     [Ceph Dev]     [Information on CEPH]     [Linux BTRFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux