Re: [RFC][PATCH 2/4][cr]: Define __f_setown_uid()

["Serge E. Hallyn" <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]

Quoting Serge E. Hallyn (serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> Quoting Sukadev Bhattiprolu (sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> 
> [From patch 2]
> 
> > @@ -222,9 +222,16 @@ int __f_setown(struct file *filp, struct pid *pid, enum pid_type type,
> >  	if (err)
> >  		return err;
> > 
> > -	f_modown(filp, pid, type, current_uid(), current_euid(), force);
> > +	f_modown(filp, pid, type, uid, euid, force);
> >  	return 0;
> >  }
> > +
> > +int __f_setown(struct file *filp, struct pid *pid, enum pid_type type,
> > +		int force)
> > +{
> > +	return __f_setown_uid(filp, pid, type, current_uid(), current_euid(),
> > +			force);
> > +}
> >  EXPORT_SYMBOL(__f_setown);
> 
> [From patch 4]
> 
> > +	/*
> > +	 * TODO: Do we need to force==1 or can it be 0 ? 'force' is used to
> > +	 * 	 modify the owner, if one is already set. Can it be set when
> > +	 * 	 we restart an application ?
> > +	 */
> > +	ret = __f_setown_uid(file, pid, h->f_owner_pid_type, h->f_owner_uid,
> > +			h->f_owner_euid, 1);
> > +	rcu_read_unlock();
> 
> I think you need to modify how __f_setown() is calling
> security_file_set_fowner().  Though I guess noone looks at the
> current_uid(), so maybe it's not so important at this point.
> 
> (I do wonder whether converting fowner to using a struct cred
> is the way to go)

Well you can probably skip LSM implications at this point.

But I'm worried about the fact that you do no check on uid here.
Note that now if a signal is to be sent, fown->pid will
get signal fow->signum sent by fown->uid.  So this looks like
a way for an unprivileged task to use root privs to kill a
task he shouldn't be able to.

-serge