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

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Nov 21, 2023 at 03:49:55AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Tuesday, November 21, 2023 9:28 AM
> > +
> > +/*
> > + * The caller holds a users refcount and wants to destroy the object. In all
> > + * cases the caller no longer has a reference on obj.
> 
> It's unclear which refcnt is being talked here given we have two now.

It says: "users refcount"

At this point the shortterm_users refcount is held by the
xarray. Let's add a note about that

> Actually none of them matches this description e.g. if wait shortterm timeouts
> then both users and shortterm_users are left intact.

shortterm_users was decremented and the object is leaked in that
case. It is a should-never-happen flow.
 
> >  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 also needs more clarification. obj->users is not recovered upon error.
> only obj->shortterm_users is put back.

And it doesn't always happen anyhow.

I'll work on this some more, it is really tricky.

> > +	 */
> > +	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);
> 
> since there is a flag parameter now can we introduce a new flag bit
> to represent extra_put? having both obj and obj->id in one invocation
> as the hint for extra_put is a bit vague.

If the user has a pointer to the object then the user must also have a
ref to the object, basic ref counting rules.

Jason