Re: [PATCH rc v2 2/2] iommufd: Do not UAF during iommufd_put_object()

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Nov 24, 2023 at 06:48:59AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Wednesday, November 22, 2023 9:13 PM
> > 
> >  static inline bool iommufd_lock_obj(struct iommufd_object *obj)
> >  {
> > -	if (!down_read_trylock(&obj->destroy_rwsem))
> > +	if (!refcount_inc_not_zero(&obj->users))
> >  		return false;
> > -	if (!refcount_inc_not_zero(&obj->users)) {
> > -		up_read(&obj->destroy_rwsem);
> > +	if (!refcount_inc_not_zero(&obj->shortterm_users)) {
> > +		/*
> > +		 * If the caller doesn't already have a ref on obj this must be
> > +		 * called under the xa_lock. Otherwise the caller is holding a
> > +		 * ref on users so this cannot make it zero.
> > +		 */
> > +		refcount_dec(&obj->users);
> >  		return false;
> 
> I'm not sure what the comment is explaining. This path can be reached
> only if the earlier refcount_inc_not_zero(&obj->users) succeeds and
> in that case certainly refcount_dec(&obj->users) cannot make it
> zero.

It is a refcount so a parallel thread could have decremented it
leaving us with 1 and then refcount_dec(1) will WARN_ON.

That can't happen because of the reasoning in the comment.

		/*
		 * If the caller doesn't already have a ref on obj this must be
		 * called under the xa_lock. Otherwise the caller is holding a
		 * ref on users. Thus it cannot be one before this decrement.
		 */

> > +/*
> > + * The caller holds a users refcount and wants to destroy the object. In all
> > + * cases the caller no longer has a users reference on obj. At this point
> > + * the xarray will be holding a shortterm_users reference.
> 
> iiuc shortterm_users will be held only if the destroy operation fails
> otherwise it's meaningless given the object is already freed. It's clearer
> to replace "at this point" with "if any error"

"At this point" was not intended to describe the error case but the
entry condition..

/*
 * The caller holds a users refcount and wants to destroy the object. At this
 * point the caller has no shortterm_users reference and at least the xarray
 * will be holding one.
 */

> > + */
> >  static inline void iommufd_object_destroy_user(struct iommufd_ctx *ictx,
> >  					       struct iommufd_object *obj)
> >  {
> > -	__iommufd_object_destroy_user(ictx, obj, false);
> > +	int ret;
> > +
> > +	ret = iommufd_object_remove(ictx, obj, obj->id,
> > REMOVE_WAIT_SHORTTERM);
> > +
> > +	/*
> > +	 * If there is a bug and we couldn't destroy the object then we did put
> > +	 * back the caller's refcount and will eventually try to free it again
> > +	 * during close.
> > +	 */
> 
> this is still inconsistent with last comment which says "the caller no longer
> has a users reference" but here "we did put back the caller's refcount"

"put back" means to have effectively done iommufd_put_object()

> > +	WARN_ON(ret);
> >  }
> > +
> > +/*
> > + * Used by autodomains to clean up the hwpt object if it was not used
> > manually.
> > + */
> >  static inline void iommufd_object_deref_user(struct iommufd_ctx *ictx,
> >  					     struct iommufd_object *obj)
> >  {
> > -	__iommufd_object_destroy_user(ictx, obj, true);
> > +	iommufd_object_remove(ictx, obj, obj->id, 0);
> 
> I forgot why autodomains can tolerate failure to not wait here. Can you
> take this chance to add some comment to clarify?

/*
 * The HWPT allocated by autodomains is used in possibly many devices and
 * is automatically destroyed when its refcount reaches zero.
 *
 * If userspace uses the HWPT manually, even for a short term, then it will
 * disrupt this refcounting and the auto-free in the kernel will not work.
 * Userspace that tries to use the automatically allocated HWPT must be careful
 * to ensure that it is consistently destroyed, eg by not racing accesses
 * and by not attaching an automatic HWPT to a device manually.
 */
static inline void
iommufd_object_put_and_try_destroy(struct iommufd_ctx *ictx,
				   struct iommufd_object *obj)

> > +int iommufd_object_remove(struct iommufd_ctx *ictx,
> > +			  struct iommufd_object *to_destroy, u32 id,
> > +			  unsigned int flags)
> >  {
> >  	struct iommufd_object *obj;
> >  	XA_STATE(xas, &ictx->objects, id);
> > +	bool zerod_shortterm = false;
> > +	int ret;
> > +
> > +	/*
> > +	 * The purpose of the shortterm_users is to ensure deterministic
> > +	 * destruction of objects used by external drivers and destroyed by
> > this
> > +	 * function. Any temporary increment of the refcount must
> > increment
> > +	 * shortterm_users, such as during ioctl execution.
> > +	 */
> > +	if (flags & REMOVE_WAIT_SHORTTERM) {
> > +		ret = iommufd_object_dec_wait_shortterm(ictx, to_destroy);
> 
> missed a check that to_destroy must be valid when this flag bit is set

It is an internal function I don't think we need to check the
parameters for consistency..

> > +		if (ret) {
> > +			/*
> > +			 * We have a bug. Put back the callers reference and
> > +			 * defer cleaning this object until close.
> > +			 */
> > +			refcount_dec(&to_destroy->users);
> 
> explain why refcount_dec means 'put back'? also 'put back' is
> inconsistent with the earlier comment "In all cases the caller no
> longer has a users reference "

As above, "put back" means to have effectively done iommufd_put_object()

Jason