From mboxrd@z Thu Jan 1 00:00:00 1970 From: Oren Laadan Subject: Re: [PATCH 04/16][cr][v3]: Restore file_owner info Date: Wed, 04 Aug 2010 19:01:29 -0400 Message-ID: <4C59F149.90606@cs.columbia.edu> References: <1280877097-12377-1-git-send-email-sukadev@linux.vnet.ibm.com> <1280877097-12377-5-git-send-email-sukadev@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Serge Hallyn , Matt Helsley , Dan Smith , John Stultz , Matthew Wilcox , Jamie Lokier , linux-fsdevel@vger.kernel.org, Containers To: Sukadev Bhattiprolu Return-path: Received: from tarap.cc.columbia.edu ([128.59.29.7]:39708 "EHLO tarap.cc.columbia.edu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932865Ab0HDXBb (ORCPT ); Wed, 4 Aug 2010 19:01:31 -0400 In-Reply-To: <1280877097-12377-5-git-send-email-sukadev@linux.vnet.ibm.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On 08/03/2010 07:11 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[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 I may have missed a previous discussion on this - but: do you plan to relate the uid/euid to the userns ? [...] > diff --git a/fs/checkpoint.c b/fs/checkpoint.c > index ce1b4af..b5486c1 100644 > --- a/fs/checkpoint.c > +++ b/fs/checkpoint.c > @@ -618,6 +618,68 @@ 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; > + > + 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); __f_setown() does not check its pid_type argument - you need to sanitize here, no ? (and not that I expect the PIDTYPE_... will ever change, but - possibly encode the PIDTYPE_... types into CKPT_PIDTYPE_... ) [...] Oren.