Re: [PATCH 04/16][cr][v3]: Restore file_owner info

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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<sukadev@xxxxxxxxxxxxxxxxxx>

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux