Re: [PATCH 04/17][cr][v4]: Restore file_owner info

[Oren Laadan <orenl@cs.columbia.edu>]

On 08/16/2010 03:43 PM, Sukadev Bhattiprolu wrote:
> Restore the file-owner information for each 'struct file'.  This is
> essentially is like a new fcntl(F_SETOWN) and fcntl(F_SETSIG) calls,
> except that the pid, uid, euid and signum values are read from the
> checkpoint image.
> 
> Changelog[v4]:
> 	- [Oren Laadan]: Sanitize the pid-type field read from checkpoint image.
> Changelog[v3]:
> 	- [Oren Laadan]: Ensure find_vpid() found a valid pid before
> 	  making it the file owner.
> Changelog[v2]:
> 	- [Matt Helsley, Serge Hallyn]: Don't trust uids in checkpoint image.
> 	  (added CAP_KILL check)
> 	- Check that signal number read from the checkpoint image is valid.
> 	  (not sure it is required, since its an incomplete check for tampering)
> 
> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> ---
>  fs/checkpoint.c |   78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 78 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/checkpoint.c b/fs/checkpoint.c
> index ce1b4af..be9d39a 100644
> --- a/fs/checkpoint.c
> +++ b/fs/checkpoint.c
> @@ -618,6 +618,80 @@ static int attach_file(struct file *file)
>  	return fd;
>  }
>  
> +static int restore_file_owner(struct ckpt_ctx *ctx, struct ckpt_hdr_file *h,
> +		struct file *file)
> +{
> +	int ret;
> +	struct pid *pid;
> +	uid_t uid, euid;
> +
> +	uid = h->f_owner_uid;
> +	euid = h->f_owner_euid;
> +
> +	ckpt_debug("restore_file_owner(): uid %u, euid %u, pid %d, type %d\n",
> +			uid, euid, h->f_owner_pid, h->f_owner_pid_type);
> +	/*
> +	 * We can't trust the uids in the checkpoint image and normally need
> +	 * CAP_KILL. But if the uids match our ids, should be fine since we
> +	 * have access to the file.
> +	 *
> +	 * TODO: Move this check to __f_setown() ?
> +	 */
> +	ret = -EACCES;
> +	if (!capable(CAP_KILL) &&
> +			(uid != current_uid() || euid != current_euid())) {
> +		ckpt_err(ctx, ret, "image uids [%d, %d] don't match current "
> +				"process uids [%d, %d] and no CAP_KILL\n",
> +				uid, euid, current_uid(), current_euid());
> +		return ret;
> +	}
> +
> +	ret = -EINVAL;
> +	if (!valid_signal(h->f_owner_signum)) {
> +		ckpt_err(ctx, ret, "Invalid signum %d\n", h->f_owner_signum);
> +		return ret;
> +	}
> +	file->f_owner.signum = h->f_owner_signum;
> +
> +	switch(h->f_owner_pid_type) {
> +		case PIDTYPE_PID:
> +		case PIDTYPE_PGID:
> +		case PIDTYPE_SID:
> +			break;
> +		default:
> +			ckpt_err(ctx, ret, "Invalid pid-type %d\n",
> +						h->f_owner_pid_type);
> +			return ret;
> +			
> +	}
> +
> +	rcu_read_lock();
> +
> +	/*
> +	 * If file had a non-NULL owner and we can't find the owner after
> +	 * restart, return error.
> +	 */
> +	pid = find_vpid(h->f_owner_pid);
> +	if (h->f_owner_pid && !pid)
> +		ret = -ESRCH;
> +	else {
> +		/*
> +		 * TODO: Do we need 'force' to be 1 here 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(file, pid, h->f_owner_pid_type, uid, euid, 1);

The owner should not be set by now. So I suggest removing the
comment and bailing if already set.

Oren.

> +	}
> +
> +	rcu_read_unlock();
> +
> +	if (ret < 0)
> +		ckpt_err(ctx, ret, "__fsetown_uid() failed\n");
> +
> +	return ret;
> +}
> +
>  #define CKPT_SETFL_MASK  \
>  	(O_APPEND | O_NONBLOCK | O_NDELAY | FASYNC | O_DIRECT | O_NOATIME)
>  
> @@ -651,6 +725,10 @@ int restore_file_common(struct ckpt_ctx *ctx, struct file *file,
>  	if (ret < 0)
>  		return ret;
>  
> +	ret = restore_file_owner(ctx, h, file);
> +	if (ret < 0)
> +		return ret;
> +
>  	/*
>  	 * Normally f_mode is set by open, and modified only via
>  	 * fcntl(), so its value now should match that at checkpoint.