Socket c/r additional features

Socket c/r additional features

[Dan Smith]

This set provides various fixes for some corner cases where socket c/r will
currently fail.  This clears up all but one of the TODO items in the code.

[PATCH 1/4] Set socket flags on restore using sock_setsockopt() where possible

[Dan Smith]

Fail on the TIMESTAMPING_* flags for the moment, with a TODO in place to
handle them later.

Also remove other explicit flag checks because they're no longer copied
blindly into the socket object, so existing checks will be sufficient.

Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 net/checkpoint.c |  117 +++++++++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 103 insertions(+), 14 deletions(-)

diff --git a/net/checkpoint.c b/net/checkpoint.c
index ebbd68a..13c46c1 100644
--- a/net/checkpoint.c
+++ b/net/checkpoint.c
@@ -179,10 +179,6 @@ static int sock_cptrst_verify(struct ckpt_hdr_socket *h)
 	if (!ckpt_validate_errno(h->sock.err))
 		return -EINVAL;
 
-	/* None of our supported types use this flag */
-	if (h->sock.flags & SOCK_DESTROY)
-		return -EINVAL;
-
 	return 0;
 }
 
@@ -239,15 +235,99 @@ static int sock_cptrst_bufopts(int op, struct sock *sock,
 	return 0;
 }
 
+static int sock_rst_flags(struct socket *sock,
+			  struct ckpt_hdr_socket *h)
+{
+	int ret;
+	int v = 1;
+	unsigned long sk_flags = h->sock.flags;
+	unsigned long sock_flags = h->socket.flags;
+
+	if (test_and_clear_bit(SOCK_URGINLINE, &sk_flags)) {
+		ret = sock_setsockopt(sock, SOL_SOCKET, SO_OOBINLINE,
+				      (char *)&v, sizeof(v));
+		if (ret)
+			return ret;
+	}
+
+	if (test_and_clear_bit(SOCK_KEEPOPEN, &sk_flags)) {
+		ret = sock_setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE,
+				      (char *)&v, sizeof(v));
+		if (ret)
+			return ret;
+	}
+
+	if (test_and_clear_bit(SOCK_BROADCAST, &sk_flags)) {
+		ret = sock_setsockopt(sock, SOL_SOCKET, SO_BROADCAST,
+				      (char *)&v, sizeof(v));
+		if (ret)
+			return ret;
+	}
+
+	if (test_and_clear_bit(SOCK_RCVTSTAMP, &sk_flags)) {
+		ret = sock_setsockopt(sock, SOL_SOCKET, SO_TIMESTAMP,
+				      (char *)&v, sizeof(v));
+		if (ret)
+			return ret;
+	}
+
+	if (test_and_clear_bit(SOCK_RCVTSTAMPNS, &sk_flags)) {
+		ret = sock_setsockopt(sock, SOL_SOCKET, SO_TIMESTAMPNS,
+				      (char *)&v, sizeof(v));
+		if (ret)
+			return ret;
+	}
+
+	if (test_and_clear_bit(SOCK_DBG, &sk_flags)) {
+		ret = sock_setsockopt(sock, SOL_SOCKET, SO_DEBUG,
+				      (char *)&v, sizeof(v));
+		if (ret)
+			return ret;
+	}
+
+	if (test_and_clear_bit(SOCK_LOCALROUTE, &sk_flags)) {
+		ret = sock_setsockopt(sock, SOL_SOCKET, SO_DONTROUTE,
+				      (char *)&v, sizeof(v));
+		if (ret)
+			return ret;
+	}
+
+	if (test_and_clear_bit(SOCK_PASSCRED, &sock_flags)) {
+		ret = sock_setsockopt(sock, SOL_SOCKET, SO_PASSCRED,
+				      (char *)&v, sizeof(v));
+		if (ret)
+			return ret;
+	}
+
+	/* TODO: Handle SOCK_TIMESTAMPING_* flags */
+	if (test_bit(SOCK_TIMESTAMPING_TX_HARDWARE, &sk_flags) ||
+	    test_bit(SOCK_TIMESTAMPING_TX_SOFTWARE, &sk_flags) ||
+	    test_bit(SOCK_TIMESTAMPING_RX_HARDWARE, &sk_flags) ||
+	    test_bit(SOCK_TIMESTAMPING_RX_SOFTWARE, &sk_flags) ||
+	    test_bit(SOCK_TIMESTAMPING_SOFTWARE, &sk_flags) ||
+	    test_bit(SOCK_TIMESTAMPING_RAW_HARDWARE, &sk_flags) ||
+	    test_bit(SOCK_TIMESTAMPING_SYS_HARDWARE, &sk_flags)) {
+		ckpt_debug("SOF_TIMESTAMPING_* flags are not supported\n");
+		return -ENOSYS;
+	}
+
+	/* Anything that is still set in the flags that isn't part of
+	 * our protocol's default set, indicates an error
+	 */
+	if (sk_flags & ~sock->sk->sk_flags) {
+		ckpt_debug("Unhandled sock flags: %lx\n", sk_flags);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int sock_cptrst(struct ckpt_ctx *ctx,
 		       struct sock *sock,
 		       struct ckpt_hdr_socket *h,
 		       int op)
 {
-	if (sock->sk_socket) {
-		CKPT_COPY(op, h->socket.flags, sock->sk_socket->flags);
-		CKPT_COPY(op, h->socket.state, sock->sk_socket->state);
-	}
+	CKPT_COPY(op, h->socket.state, sock->sk_socket->state);
 
 	CKPT_COPY(op, h->sock_common.bound_dev_if, sock->sk_bound_dev_if);
 	CKPT_COPY(op, h->sock_common.family, sock->sk_family);
@@ -262,12 +342,6 @@ static int sock_cptrst(struct ckpt_ctx *ctx,
 	CKPT_COPY(op, h->sock.state, sock->sk_state);
 	CKPT_COPY(op, h->sock.backlog, sock->sk_max_ack_backlog);
 
-	/* TODO:
-	 * Break out setting each of the flags to use setsockopt() or
-	 * perform proper security check
-	 */
-	CKPT_COPY(op, h->sock.flags, sock->sk_flags);
-
 	if (sock_cptrst_bufopts(op, sock, h))
 		return -EINVAL;
 
@@ -301,6 +375,21 @@ static int sock_cptrst(struct ckpt_ctx *ctx,
 		return -EINVAL;
 	}
 
+	if (op == CKPT_CPT) {
+		h->sock.flags = sock->sk_flags;
+		h->socket.flags = sock->sk_socket->flags;
+	} else {
+		int ret;
+		mm_segment_t old_fs;
+
+		old_fs = get_fs();
+		set_fs(KERNEL_DS);
+		ret = sock_rst_flags(sock->sk_socket, h);
+		set_fs(old_fs);
+		if (ret)
+			return ret;
+	}
+
 	if ((h->socket.state == SS_CONNECTED) &&
 	    (h->sock.state != TCP_ESTABLISHED)) {
 		ckpt_debug("socket/sock in inconsistent state: %i/%i",
-- 
1.6.0.4

[PATCH 2/4] Expose may_setuid() in user.h

[Dan Smith]

Make this helper available to others.

Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 include/linux/user.h |    9 +++++++++
 kernel/user.c        |   16 +++++++++++++++-
 2 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/include/linux/user.h b/include/linux/user.h
index 68daf84..713bae7 100644
--- a/include/linux/user.h
+++ b/include/linux/user.h
@@ -1 +1,10 @@
+#ifndef _LINUX_USER_H
+#define _LINUX_USER_H
+
 #include <asm/user.h>
+#include <linux/sched.h>
+
+extern int may_setuid(struct user_namespace *ns, uid_t uid);
+extern int may_setgid(struct group_info *groupinfo, gid_t gid);
+
+#endif
diff --git a/kernel/user.c b/kernel/user.c
index a535ed6..38b8b50 100644
--- a/kernel/user.c
+++ b/kernel/user.c
@@ -604,7 +604,7 @@ int checkpoint_user(struct ckpt_ctx *ctx, void *ptr)
 	return do_checkpoint_user(ctx, (struct user_struct *) ptr);
 }
 
-static int may_setuid(struct user_namespace *ns, uid_t uid)
+int may_setuid(struct user_namespace *ns, uid_t uid)
 {
 	/*
 	 * this next check will one day become
@@ -631,6 +631,20 @@ static int may_setuid(struct user_namespace *ns, uid_t uid)
 	return 0;
 }
 
+int may_setgid(struct group_info *groupinfo, gid_t gid)
+{
+	if (capable(CAP_SETGID))
+		return 1;
+
+	if (current_cred_xxx(group_info) != groupinfo)
+		return 0;
+
+	if (in_egroup_p(gid))
+		return 1;
+
+	return 0;
+}
+
 static struct user_struct *do_restore_user(struct ckpt_ctx *ctx)
 {
 	struct user_struct *u;
-- 
1.6.0.4

[PATCH 3/4] Save and restore UNIX socket peer credentials

[Dan Smith]

This saves the uid/gid of the sk_peercred structure in the checkpoint
stream.  On restart, it uses may_setuid() and may_setgid() to determine
if the uid/gid from the checkpoint stream may be used.

Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 include/linux/checkpoint_hdr.h |    2 ++
 net/unix/checkpoint.c          |   29 ++++++++++++++++-------------
 2 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/include/linux/checkpoint_hdr.h b/include/linux/checkpoint_hdr.h
index 829ff2d..6c6780c 100644
--- a/include/linux/checkpoint_hdr.h
+++ b/include/linux/checkpoint_hdr.h
@@ -523,6 +523,8 @@ struct ckpt_hdr_socket_unix {
 	struct ckpt_hdr h;
 	__s32 this;
 	__s32 peer;
+	__u32 peercred_uid;
+	__u32 peercred_gid;
 	__u32 flags;
 	__u32 laddr_len;
 	__u32 raddr_len;
diff --git a/net/unix/checkpoint.c b/net/unix/checkpoint.c
index 841d25d..eb19e66 100644
--- a/net/unix/checkpoint.c
+++ b/net/unix/checkpoint.c
@@ -3,6 +3,7 @@
 #include <linux/fs_struct.h>
 #include <linux/checkpoint.h>
 #include <linux/checkpoint_hdr.h>
+#include <linux/user.h>
 #include <net/af_unix.h>
 #include <net/tcp_states.h>
 
@@ -98,6 +99,9 @@ int sock_unix_checkpoint(struct ckpt_ctx *ctx,
 		goto out;
 	}
 
+	un->peercred_uid = socket->sk->sk_peercred.uid;
+	un->peercred_gid = socket->sk->sk_peercred.gid;
+
 	ret = ckpt_write_obj(ctx, (struct ckpt_hdr *) h);
 	if (ret < 0)
 		goto out;
@@ -225,19 +229,6 @@ static int sock_unix_join(struct ckpt_ctx *ctx,
 	unix_sk(a)->peer = b;
 	unix_sk(b)->peer = a;
 
-	/* TODO:
-	 * Checkpoint the credentials, restore them here if the values match
-	 * the restored creds or we may_setuid()
-	 */
-
-	a->sk_peercred.pid = task_tgid_vnr(current);
-	a->sk_peercred.uid = ctx->realcred->uid;
-	a->sk_peercred.gid = ctx->realcred->gid;
-
-	b->sk_peercred.pid = a->sk_peercred.pid;
-	b->sk_peercred.uid = a->sk_peercred.uid;
-	b->sk_peercred.gid = a->sk_peercred.gid;
-
 	if (!UNIX_ADDR_EMPTY(un->raddr_len))
 		addr = sock_unix_makeaddr(&un->raddr, un->raddr_len);
 	else if (!UNIX_ADDR_EMPTY(un->laddr_len))
@@ -303,6 +294,18 @@ static int sock_unix_restore_connected(struct ckpt_ctx *ctx,
 		goto out;
 	}
 
+	this->sk_peercred.pid = task_tgid_vnr(current);
+
+	if (may_setuid(ctx->realcred->user->user_ns, un->peercred_uid) &&
+	    may_setgid(ctx->realcred->group_info, un->peercred_gid)) {
+		this->sk_peercred.uid = un->peercred_uid;
+		this->sk_peercred.gid = un->peercred_gid;
+	} else {
+		ckpt_debug("peercred %i:%i would require setuid",
+			   un->peercred_uid, un->peercred_gid);
+		return -1;
+	}
+
 	/* Prime the socket's buffer limit with the maximum.  These will be
 	 * overwritten with the values in the checkpoint stream in a later
 	 * phase.
-- 
1.6.0.4

[PATCH 4/4] Handle unconnected DGRAM sockets with buffers in-flight

[Dan Smith]

This patch avoids connecting an unconnected DGRAM socket to a fake peer as
part of the restore process.  Note that it will appear to rewrite all packets
received as coming from a single unbound sender.

Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 net/unix/checkpoint.c |   41 +++++++++++++++++++++++++++--------------
 1 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/net/unix/checkpoint.c b/net/unix/checkpoint.c
index eb19e66..a499246 100644
--- a/net/unix/checkpoint.c
+++ b/net/unix/checkpoint.c
@@ -118,7 +118,8 @@ int sock_unix_checkpoint(struct ckpt_ctx *ctx,
 	return ret;
 }
 
-static int sock_read_buffer_sendmsg(struct ckpt_ctx *ctx, struct sock *sock)
+static int sock_read_buffer_sendmsg(struct ckpt_ctx *ctx, struct sock *sock,
+				    struct sockaddr *addr, unsigned int alen)
 {
 	struct msghdr msg;
 	struct kvec kvec;
@@ -146,6 +147,11 @@ static int sock_read_buffer_sendmsg(struct ckpt_ctx *ctx, struct sock *sock)
 	if (ret < 0)
 		goto out;
 
+	if (addr) {
+		msg.msg_name = addr;
+		msg.msg_namelen = alen;
+	}
+
 	ret = kernel_sendmsg(sock->sk_socket, &msg, &kvec, 1, len);
 	ckpt_debug("kernel_sendmsg(%i): %i\n", len, ret);
 	if ((ret > 0) && (ret != len))
@@ -158,7 +164,8 @@ static int sock_read_buffer_sendmsg(struct ckpt_ctx *ctx, struct sock *sock)
 
 static int sock_unix_read_buffers(struct ckpt_ctx *ctx,
 				  struct sock *sock,
-				  uint32_t *bufsize)
+				  uint32_t *bufsize,
+				  struct ckpt_hdr_socket_unix *un)
 {
 	uint8_t sock_shutdown;
 	struct ckpt_hdr_socket_queue *h;
@@ -174,7 +181,16 @@ static int sock_unix_read_buffers(struct ckpt_ctx *ctx,
 	sock->sk_shutdown &= ~SHUTDOWN_MASK;
 
 	for (i = 0; i < h->skb_count; i++) {
-		ret = sock_read_buffer_sendmsg(ctx, sock);
+		struct sockaddr *addr = NULL;
+		unsigned int len = un->laddr_len;
+
+		/* If we don't have a real peer, then we're not connected,
+		 * and thus need to send this to a specific address
+		 */
+		if (!un->peer)
+			addr = (struct sockaddr *)&un->laddr;
+
+		ret = sock_read_buffer_sendmsg(ctx, sock, addr, len);
 		ckpt_debug("read_buffer_sendmsg(%i): %i\n", i, ret);
 		if (ret < 0)
 			break;
@@ -283,10 +299,12 @@ static int sock_unix_restore_connected(struct ckpt_ctx *ctx,
 		if (ret < 0)
 			goto out;
 
-		ret = sock_unix_join(ctx, this, peer, un);
-		ckpt_debug("sock_unix_join: %i\n", ret);
-		if (ret)
-			goto out;
+		if (h->sock.state == TCP_ESTABLISHED) {
+			ret = sock_unix_join(ctx, this, peer, un);
+			ckpt_debug("sock_unix_join: %i\n", ret);
+			if (ret)
+				goto out;
+		}
 
 	} else {
 		ckpt_debug("Order Error\n");
@@ -314,18 +332,13 @@ static int sock_unix_restore_connected(struct ckpt_ctx *ctx,
 	peer->sk_sndbuf = sysctl_wmem_max;
 
 	/* Read my buffers and sendmsg() them back to me via my peer */
-
-	/* TODO: handle the unconnected case, as well, as the case
-	 *       where sendto() has been used on some of the buffers
-	 */
-
-	ret = sock_unix_read_buffers(ctx, peer, &peer->sk_sndbuf);
+	ret = sock_unix_read_buffers(ctx, peer, &peer->sk_sndbuf, un);
 	ckpt_debug("sock_unix_read_buffers: %i\n", ret);
 	if (ret)
 		goto out;
 
 	/* Read peer's buffers and expect 0 */
-	ret = sock_unix_read_buffers(ctx, peer, NULL);
+	ret = sock_unix_read_buffers(ctx, peer, NULL, un);
  out:
 	if (tmp && ret)
 		sock_release(tmp);
-- 
1.6.0.4

Re: [PATCH 1/4] Set socket flags on restore using sock_setsockopt() where possible

[Oren Laadan]

Dan Smith wrote:
> Fail on the TIMESTAMPING_* flags for the moment, with a TODO in place to
> handle them later.
> 
> Also remove other explicit flag checks because they're no longer copied
> blindly into the socket object, so existing checks will be sufficient.
> 
> Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---

Nice cleanup.
See one comment below.

>  net/checkpoint.c |  117 +++++++++++++++++++++++++++++++++++++++++++++++------
>  1 files changed, 103 insertions(+), 14 deletions(-)
> 
> diff --git a/net/checkpoint.c b/net/checkpoint.c
> index ebbd68a..13c46c1 100644
> --- a/net/checkpoint.c
> +++ b/net/checkpoint.c
> @@ -179,10 +179,6 @@ static int sock_cptrst_verify(struct ckpt_hdr_socket *h)
>  	if (!ckpt_validate_errno(h->sock.err))
>  		return -EINVAL;
>  
> -	/* None of our supported types use this flag */
> -	if (h->sock.flags & SOCK_DESTROY)
> -		return -EINVAL;
> -
>  	return 0;
>  }
>  
> @@ -239,15 +235,99 @@ static int sock_cptrst_bufopts(int op, struct sock *sock,
>  	return 0;
>  }
>  
> +static int sock_rst_flags(struct socket *sock,
> +			  struct ckpt_hdr_socket *h)
> +{
> +	int ret;
> +	int v = 1;
> +	unsigned long sk_flags = h->sock.flags;
> +	unsigned long sock_flags = h->socket.flags;
> +
> +	if (test_and_clear_bit(SOCK_URGINLINE, &sk_flags)) {
> +		ret = sock_setsockopt(sock, SOL_SOCKET, SO_OOBINLINE,
> +				      (char *)&v, sizeof(v));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (test_and_clear_bit(SOCK_KEEPOPEN, &sk_flags)) {
> +		ret = sock_setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE,
> +				      (char *)&v, sizeof(v));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (test_and_clear_bit(SOCK_BROADCAST, &sk_flags)) {
> +		ret = sock_setsockopt(sock, SOL_SOCKET, SO_BROADCAST,
> +				      (char *)&v, sizeof(v));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (test_and_clear_bit(SOCK_RCVTSTAMP, &sk_flags)) {
> +		ret = sock_setsockopt(sock, SOL_SOCKET, SO_TIMESTAMP,
> +				      (char *)&v, sizeof(v));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (test_and_clear_bit(SOCK_RCVTSTAMPNS, &sk_flags)) {
> +		ret = sock_setsockopt(sock, SOL_SOCKET, SO_TIMESTAMPNS,
> +				      (char *)&v, sizeof(v));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (test_and_clear_bit(SOCK_DBG, &sk_flags)) {
> +		ret = sock_setsockopt(sock, SOL_SOCKET, SO_DEBUG,
> +				      (char *)&v, sizeof(v));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (test_and_clear_bit(SOCK_LOCALROUTE, &sk_flags)) {
> +		ret = sock_setsockopt(sock, SOL_SOCKET, SO_DONTROUTE,
> +				      (char *)&v, sizeof(v));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (test_and_clear_bit(SOCK_PASSCRED, &sock_flags)) {
> +		ret = sock_setsockopt(sock, SOL_SOCKET, SO_PASSCRED,
> +				      (char *)&v, sizeof(v));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* TODO: Handle SOCK_TIMESTAMPING_* flags */
> +	if (test_bit(SOCK_TIMESTAMPING_TX_HARDWARE, &sk_flags) ||
> +	    test_bit(SOCK_TIMESTAMPING_TX_SOFTWARE, &sk_flags) ||
> +	    test_bit(SOCK_TIMESTAMPING_RX_HARDWARE, &sk_flags) ||
> +	    test_bit(SOCK_TIMESTAMPING_RX_SOFTWARE, &sk_flags) ||
> +	    test_bit(SOCK_TIMESTAMPING_SOFTWARE, &sk_flags) ||
> +	    test_bit(SOCK_TIMESTAMPING_RAW_HARDWARE, &sk_flags) ||
> +	    test_bit(SOCK_TIMESTAMPING_SYS_HARDWARE, &sk_flags)) {
> +		ckpt_debug("SOF_TIMESTAMPING_* flags are not supported\n");
> +		return -ENOSYS;
> +	}
> +
> +	/* Anything that is still set in the flags that isn't part of
> +	 * our protocol's default set, indicates an error
> +	 */
> +	if (sk_flags & ~sock->sk->sk_flags) {
> +		ckpt_debug("Unhandled sock flags: %lx\n", sk_flags);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static int sock_cptrst(struct ckpt_ctx *ctx,
>  		       struct sock *sock,
>  		       struct ckpt_hdr_socket *h,
>  		       int op)
>  {
> -	if (sock->sk_socket) {
> -		CKPT_COPY(op, h->socket.flags, sock->sk_socket->flags);
> -		CKPT_COPY(op, h->socket.state, sock->sk_socket->state);
> -	}
> +	CKPT_COPY(op, h->socket.state, sock->sk_socket->state);

[...]

When you add support to new socket due to connect() that were not
yet accept()ed from the listening socket - there will be a case of
a sock without sock->sk_socket.

This probably means that we want the test for sock->sk_socket to
remain as is (and a similar one in sock_rst_flags above)

Super nit: perhaps s/rst/restore/ ?  Besides agreeing with current
practice, it may dodge a related rant from Linux :o

Oren.

Re: [PATCH 1/4] Set socket flags on restore using sock_setsockopt() where possible

[Dan Smith]

OL> When you add support to new socket due to connect() that were not
OL> yet accept()ed from the listening socket - there will be a case of
OL> a sock without sock->sk_socket.

OL> This probably means that we want the test for sock->sk_socket to
OL> remain as is (and a similar one in sock_rst_flags above)

Okay, but we've got a lot of other places above that where we assume
sock->sk_socket is valid, so the thinking here was that when we have a
real reason to handle that case, I'd fix all the other cases as well.

However, I shouldn't have done that in the middle of this patch anyway.

OL> Super nit: perhaps s/rst/restore/ ?  Besides agreeing with current
OL> practice, it may dodge a related rant from Linux :o
    
Sure.

I'll re-send with the rename and without the sock->sk_socket and then
follow up with a fix for the other cases in a later patch.

-- 
Dan Smith
IBM Linux Technology Center
email: danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org

Re: [PATCH 4/4] Handle unconnected DGRAM sockets with buffers in-flight

[Oren Laadan]

Dan Smith wrote:
> This patch avoids connecting an unconnected DGRAM socket to a fake peer as
> part of the restore process.  Note that it will appear to rewrite all packets
> received as coming from a single unbound sender.

Hmm.. I think this would break recvfrom() syscall:  it eventually calls
unix_dgram_recvmsg(), which grabs the next skb (datagram) in the queue,
and fills in the address of the socket from which the datagram had been
sent (af_unix.c:1672)

	if (msg->msg_name)
		unix_copy_addr(msg, skb->sk);

?

The more I think about it, it seems better to also checkpoint those
unconnected sockets that are the source of dgrams. IOW, when looping
through the received skb's of an unconnected dgram socket, then check
the skb->sk of each pending packet, and checkpoint that socket too.

And in restore, restore those too, even though they may be orphan
(closed, not referenced by any process anymore), and send each packet
from the correct origin socket.

This, btw, will be another case of sock without sk->sk_socket...

Oren.

Re: [PATCH 4/4] Handle unconnected DGRAM sockets with buffers in-flight

[Dan Smith]

OL> Hmm.. I think this would break recvfrom() syscall: it eventually
OL> calls unix_dgram_recvmsg(), which grabs the next skb (datagram) in
OL> the queue, and fills in the address of the socket from which the
OL> datagram had been sent (af_unix.c:1672)

Why would that be any different from the normal case of sending from
an unbound socket to a process receiving with recvfrom()?

OL> 	if (msg->msg_name)
OL> 		unix_copy_addr(msg, skb->sk);

If you look at unix_copy_addr() it bails if !sk->addr.

OL> The more I think about it, it seems better to also checkpoint
OL> those unconnected sockets that are the source of dgrams. IOW, when
OL> looping through the received skb's of an unconnected dgram socket,
OL> then check the skb->sk of each pending packet, and checkpoint that
OL> socket too.

That will, IMHO, make the restore process a little uglier.  I was
thinking more along the lines of saving the address of the skb's
sender (if needed) and doing something like a temporary bind of the
peer before sending the buffer on restore.

-- 
Dan Smith
IBM Linux Technology Center
email: danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org

Re: [PATCH 4/4] Handle unconnected DGRAM sockets with buffers in-flight

[Oren Laadan]

Dan Smith wrote:
> OL> Hmm.. I think this would break recvfrom() syscall: it eventually
> OL> calls unix_dgram_recvmsg(), which grabs the next skb (datagram) in
> OL> the queue, and fills in the address of the socket from which the
> OL> datagram had been sent (af_unix.c:1672)
> 
> Why would that be any different from the normal case of sending from
> an unbound socket to a process receiving with recvfrom()?

The recvfrom() would give wrong result (compared to original execution)
unless the correct socket-of-origin is used.

> 
> OL> 	if (msg->msg_name)
> OL> 		unix_copy_addr(msg, skb->sk);
> 
> If you look at unix_copy_addr() it bails if !sk->addr.
> 
> OL> The more I think about it, it seems better to also checkpoint
> OL> those unconnected sockets that are the source of dgrams. IOW, when
> OL> looping through the received skb's of an unconnected dgram socket,
> OL> then check the skb->sk of each pending packet, and checkpoint that
> OL> socket too.
> 
> That will, IMHO, make the restore process a little uglier.  I was
> thinking more along the lines of saving the address of the skb's
> sender (if needed) and doing something like a temporary bind of the
> peer before sending the buffer on restore.
> 

This would mean one socket for all skb's (dgrams), so one address
for all of them. So one result for recvfrom() on restored system
vs multiple different addresses on original system.

Oren.

Re: [PATCH 1/4] Set socket flags on restore using sock_setsockopt() where possible

[Serge E. Hallyn]

Quoting Dan Smith (danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> Fail on the TIMESTAMPING_* flags for the moment, with a TODO in place to
> handle them later.
> 
> Also remove other explicit flag checks because they're no longer copied
> blindly into the socket object, so existing checks will be sufficient.
> 
> Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

Nicely structured.

Acked-by: Serge Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

thanks,
-serge

Re: [PATCH 2/4] Expose may_setuid() in user.h

[Serge E. Hallyn]

Quoting Dan Smith (danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> Make this helper available to others.

No objection to exporting may_setuid, nor to creating may_setgid(),
but I don't think may_setgid() is right.  See below

> Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  include/linux/user.h |    9 +++++++++
>  kernel/user.c        |   16 +++++++++++++++-
>  2 files changed, 24 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/user.h b/include/linux/user.h
> index 68daf84..713bae7 100644
> --- a/include/linux/user.h
> +++ b/include/linux/user.h
> @@ -1 +1,10 @@
> +#ifndef _LINUX_USER_H
> +#define _LINUX_USER_H
> +
>  #include <asm/user.h>
> +#include <linux/sched.h>
> +
> +extern int may_setuid(struct user_namespace *ns, uid_t uid);
> +extern int may_setgid(struct group_info *groupinfo, gid_t gid);
> +
> +#endif
> diff --git a/kernel/user.c b/kernel/user.c
> index a535ed6..38b8b50 100644
> --- a/kernel/user.c
> +++ b/kernel/user.c
> @@ -604,7 +604,7 @@ int checkpoint_user(struct ckpt_ctx *ctx, void *ptr)
>  	return do_checkpoint_user(ctx, (struct user_struct *) ptr);
>  }
> 
> -static int may_setuid(struct user_namespace *ns, uid_t uid)
> +int may_setuid(struct user_namespace *ns, uid_t uid)
>  {
>  	/*
>  	 * this next check will one day become
> @@ -631,6 +631,20 @@ static int may_setuid(struct user_namespace *ns, uid_t uid)
>  	return 0;
>  }
> 
> +int may_setgid(struct group_info *groupinfo, gid_t gid)
> +{
> +	if (capable(CAP_SETGID))
> +		return 1;

We should pass in a user_ns so we can eventually check for
capable_to(ns, CAP_SETGID).  So the caller may not be
CAP_SETGID in the root user namespace, but may have created
the child user namespace and be privileged there.

> +	if (current_cred_xxx(group_info) != groupinfo)
> +		return 0;

I think it's possible for two different group_info's to have the same
member groups.

I think the thing to do is walk over all groups in group_info,
and do in_egroup_p(g) for each.

It's also possible that groups 1, 4, and 5 are in current_group_info and 6 is
current_egroup, while we're asking for a group_info with groups 1, 5 and 6, and
egid of 4.  That would be legal, right?  Walking over the groups and doing
in_egroup_p(g) should do that check.  Sure, it's n^2 on the # groups...  So
we could eventually optimize it to exploit the fact that both groupinfos
are sorted and keep last_used_g in both groupinfos...

> +	if (in_egroup_p(gid))
> +		return 1;
> +
> +	return 0;
> +}
> +
>  static struct user_struct *do_restore_user(struct ckpt_ctx *ctx)
>  {
>  	struct user_struct *u;
> -- 
> 1.6.0.4
> 
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linux-foundation.org/mailman/listinfo/containers

Re: [PATCH 2/4] Expose may_setuid() in user.h

[Serge E. Hallyn]

Quoting Serge E. Hallyn (serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> Quoting Dan Smith (danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> > Make this helper available to others.
> 
> No objection to exporting may_setuid, nor to creating may_setgid(),
> but I don't think may_setgid() is right.  See below
> 
> > Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> > ---
> >  include/linux/user.h |    9 +++++++++
> >  kernel/user.c        |   16 +++++++++++++++-
> >  2 files changed, 24 insertions(+), 1 deletions(-)
> > 
> > diff --git a/include/linux/user.h b/include/linux/user.h
> > index 68daf84..713bae7 100644
> > --- a/include/linux/user.h
> > +++ b/include/linux/user.h
> > @@ -1 +1,10 @@
> > +#ifndef _LINUX_USER_H
> > +#define _LINUX_USER_H
> > +
> >  #include <asm/user.h>
> > +#include <linux/sched.h>
> > +
> > +extern int may_setuid(struct user_namespace *ns, uid_t uid);
> > +extern int may_setgid(struct group_info *groupinfo, gid_t gid);
> > +
> > +#endif
> > diff --git a/kernel/user.c b/kernel/user.c
> > index a535ed6..38b8b50 100644
> > --- a/kernel/user.c
> > +++ b/kernel/user.c
> > @@ -604,7 +604,7 @@ int checkpoint_user(struct ckpt_ctx *ctx, void *ptr)
> >  	return do_checkpoint_user(ctx, (struct user_struct *) ptr);
> >  }
> > 
> > -static int may_setuid(struct user_namespace *ns, uid_t uid)
> > +int may_setuid(struct user_namespace *ns, uid_t uid)
> >  {
> >  	/*
> >  	 * this next check will one day become
> > @@ -631,6 +631,20 @@ static int may_setuid(struct user_namespace *ns, uid_t uid)
> >  	return 0;
> >  }
> > 
> > +int may_setgid(struct group_info *groupinfo, gid_t gid)
> > +{
> > +	if (capable(CAP_SETGID))
> > +		return 1;
> 
> We should pass in a user_ns so we can eventually check for
> capable_to(ns, CAP_SETGID).  So the caller may not be
> CAP_SETGID in the root user namespace, but may have created
> the child user namespace and be privileged there.
> 
> > +	if (current_cred_xxx(group_info) != groupinfo)
> > +		return 0;
> 
> I think it's possible for two different group_info's to have the same
> member groups.
> 
> I think the thing to do is walk over all groups in group_info,
> and do in_egroup_p(g) for each.
> 
> It's also possible that groups 1, 4, and 5 are in current_group_info and 6 is
> current_egroup, while we're asking for a group_info with groups 1, 5 and 6, and
> egid of 4.  That would be legal, right?  Walking over the groups and doing
> in_egroup_p(g) should do that check.  Sure, it's n^2 on the # groups...  So
> we could eventually optimize it to exploit the fact that both groupinfos
> are sorted and keep last_used_g in both groupinfos...
> 
> > +	if (in_egroup_p(gid))
> > +		return 1;

Ah, nm - you don't actually much care about the groupinfos, that
was a sanity check?

You don't actually want to check the ctx->realcred.  I think you
just want to check in_egroup_p(gid), as that will chekc against
the credentials used to call sys_restart(), which is what you want.

-serge

Re: [PATCH 3/4] Save and restore UNIX socket peer credentials

[Serge E. Hallyn]

Quoting Dan Smith (danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> This saves the uid/gid of the sk_peercred structure in the checkpoint
> stream.  On restart, it uses may_setuid() and may_setgid() to determine
> if the uid/gid from the checkpoint stream may be used.
> 
> Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  include/linux/checkpoint_hdr.h |    2 ++
>  net/unix/checkpoint.c          |   29 ++++++++++++++++-------------
>  2 files changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/include/linux/checkpoint_hdr.h b/include/linux/checkpoint_hdr.h
> index 829ff2d..6c6780c 100644
> --- a/include/linux/checkpoint_hdr.h
> +++ b/include/linux/checkpoint_hdr.h
> @@ -523,6 +523,8 @@ struct ckpt_hdr_socket_unix {
>  	struct ckpt_hdr h;
>  	__s32 this;
>  	__s32 peer;
> +	__u32 peercred_uid;
> +	__u32 peercred_gid;
>  	__u32 flags;
>  	__u32 laddr_len;
>  	__u32 raddr_len;
> diff --git a/net/unix/checkpoint.c b/net/unix/checkpoint.c
> index 841d25d..eb19e66 100644
> --- a/net/unix/checkpoint.c
> +++ b/net/unix/checkpoint.c
> @@ -3,6 +3,7 @@
>  #include <linux/fs_struct.h>
>  #include <linux/checkpoint.h>
>  #include <linux/checkpoint_hdr.h>
> +#include <linux/user.h>
>  #include <net/af_unix.h>
>  #include <net/tcp_states.h>
> 
> @@ -98,6 +99,9 @@ int sock_unix_checkpoint(struct ckpt_ctx *ctx,
>  		goto out;
>  	}
> 
> +	un->peercred_uid = socket->sk->sk_peercred.uid;
> +	un->peercred_gid = socket->sk->sk_peercred.gid;
> +
>  	ret = ckpt_write_obj(ctx, (struct ckpt_hdr *) h);
>  	if (ret < 0)
>  		goto out;
> @@ -225,19 +229,6 @@ static int sock_unix_join(struct ckpt_ctx *ctx,
>  	unix_sk(a)->peer = b;
>  	unix_sk(b)->peer = a;
> 
> -	/* TODO:
> -	 * Checkpoint the credentials, restore them here if the values match
> -	 * the restored creds or we may_setuid()
> -	 */
> -
> -	a->sk_peercred.pid = task_tgid_vnr(current);
> -	a->sk_peercred.uid = ctx->realcred->uid;
> -	a->sk_peercred.gid = ctx->realcred->gid;
> -
> -	b->sk_peercred.pid = a->sk_peercred.pid;
> -	b->sk_peercred.uid = a->sk_peercred.uid;
> -	b->sk_peercred.gid = a->sk_peercred.gid;
> -
>  	if (!UNIX_ADDR_EMPTY(un->raddr_len))
>  		addr = sock_unix_makeaddr(&un->raddr, un->raddr_len);
>  	else if (!UNIX_ADDR_EMPTY(un->laddr_len))
> @@ -303,6 +294,18 @@ static int sock_unix_restore_connected(struct ckpt_ctx *ctx,
>  		goto out;
>  	}
> 
> +	this->sk_peercred.pid = task_tgid_vnr(current);
> +
> +	if (may_setuid(ctx->realcred->user->user_ns, un->peercred_uid) &&
> +	    may_setgid(ctx->realcred->group_info, un->peercred_gid)) {
> +		this->sk_peercred.uid = un->peercred_uid;
> +		this->sk_peercred.gid = un->peercred_gid;

It's a real shame that we have this uid and gid with no indication of
which user_ns it belongs in.  But I do think that assuming
ctx->realcred->user->user_ns  is the right one is the best guess you
can make.

So the may_setuid() is right, but may_setgid() should be changed
to
	may_setgid(ctx->realcred->user->user_ns, un->peercred_gid,
			current_cred());

meaning: we want to know whether:
	1. current_cred() has cap_capable to ctx->realcred->user->user_ns
		(which it does if it created it - once that's implemented)
or
	(
	2. current_cred->user->user_ns == ctx->real_cred->user_user_ns
		and
	3. un->peercred_gid is equal to current_cred()->egid or is in
		current_cred->group_info.
	)

Then again, until we add a user_ns to peercred, that will result
in a safety problem with peercred!

/me thinks some more

> +	} else {
> +		ckpt_debug("peercred %i:%i would require setuid",
> +			   un->peercred_uid, un->peercred_gid);
> +		return -1;
> +	}
> +
>  	/* Prime the socket's buffer limit with the maximum.  These will be
>  	 * overwritten with the values in the checkpoint stream in a later
>  	 * phase.
> -- 
> 1.6.0.4
> 
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linux-foundation.org/mailman/listinfo/containers

Re: [PATCH 2/4] Expose may_setuid() in user.h

[Serge E. Hallyn]

Quoting Dan Smith (danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> Make this helper available to others.
> 
> Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  include/linux/user.h |    9 +++++++++
>  kernel/user.c        |   16 +++++++++++++++-
>  2 files changed, 24 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/user.h b/include/linux/user.h
> index 68daf84..713bae7 100644
> --- a/include/linux/user.h
> +++ b/include/linux/user.h
> @@ -1 +1,10 @@
> +#ifndef _LINUX_USER_H
> +#define _LINUX_USER_H
> +
>  #include <asm/user.h>
> +#include <linux/sched.h>
> +
> +extern int may_setuid(struct user_namespace *ns, uid_t uid);
> +extern int may_setgid(struct group_info *groupinfo, gid_t gid);
> +
> +#endif
> diff --git a/kernel/user.c b/kernel/user.c
> index a535ed6..38b8b50 100644
> --- a/kernel/user.c
> +++ b/kernel/user.c
> @@ -604,7 +604,7 @@ int checkpoint_user(struct ckpt_ctx *ctx, void *ptr)
>  	return do_checkpoint_user(ctx, (struct user_struct *) ptr);
>  }
> 
> -static int may_setuid(struct user_namespace *ns, uid_t uid)
> +int may_setuid(struct user_namespace *ns, uid_t uid)
>  {
>  	/*
>  	 * this next check will one day become
> @@ -631,6 +631,20 @@ static int may_setuid(struct user_namespace *ns, uid_t uid)
>  	return 0;
>  }
> 
> +int may_setgid(struct group_info *groupinfo, gid_t gid)
> +{
> +	if (capable(CAP_SETGID))
> +		return 1;
> +
> +	if (current_cred_xxx(group_info) != groupinfo)
> +		return 0;
> +
> +	if (in_egroup_p(gid))
> +		return 1;
> +
> +	return 0;
> +}

All right, so unless you want to implement sticking a struct user
or struct cred in the peercreds :), just take out the middle part,
so make it:

> +int may_setgid(gid_t gid)
> +{
> +	if (capable(CAP_SETGID))
> +		return 1;
> +
> +	if (in_egroup_p(gid))
> +		return 1;
> +
> +	return 0;
> +}