Socket c/r additional features
Subsystems (2)
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.
A message that is not in the archive (sent off-list or not yet received): 1250191750-3864-1-git-send-email-danms-r/[email protected]
PATCH 1/4 Set socket flags on restore using sock_setsockopt() where possible
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.
--- net/checkpoint.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 103 insertions(+), 14 deletions(-)
net/checkpoint.c +103 -14
@@ -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",
Signature · 1 line
1.6.0.4
A message that is not in the archive (sent off-list or not yet received): 1250191750-3864-2-git-send-email-danms-r/[email protected]
Re: [PATCH 1/4] Set socket flags on restore using sock_setsockopt() where possible
Quoted 8 lines · 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/[email protected]>---
Nice cleanup.
See one comment below.
Quoted 109 lines
net/checkpoint.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 103 insertions(+), 14 deletions(-)net/checkpoint.c +87
-4@@ -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 ; +} +
net/checkpoint.c static int sock_cptrst_bufopts(int op, struct sock *sock, · line 330
Quoted 10 lines
net/checkpoint.c +1
-4static 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.
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH 1/4] Set socket flags on restore using sock_setsockopt() where possible
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.
Signature · 3 lines
Dan Smith IBM Linux Technology Center email: danms-r/[email protected]
Serge E. Hallyn
Acked-byRe: [PATCH 1/4] Set socket flags on restore using sock_setsockopt() where possible
In reply to a message not in the archiveQuoted 7 lines · 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/[email protected]>
Nicely structured.
thanks,
-serge
PATCH 2/4 Expose may_setuid() in user.h
In reply to a message not in the archiveMake this helper available to others.
--- include/linux/user.h | 9 +++++++++ kernel/user.c | 16 +++++++++++++++- 2 files changed, 24 insertions(+), 1 deletions(-)
include/linux/user.h +9 -0
@@ -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
kernel/user.c +15 -1
@@ -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;
Signature · 1 line
1.6.0.4
A message that is not in the archive (sent off-list or not yet received): 1250191750-3864-3-git-send-email-danms-r/[email protected]
Re: [PATCH 2/4] Expose may_setuid() in user.h
Quoted 1 line · Dan Smith
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
Quoted 35 lines
Signed-off-by Dan Smith <danms-r/[email protected]>--- include/linux/user.h | 9 +++++++++ kernel/user.c | 16 +++++++++++++++- 2 files changed, 24 insertions(+), 1 deletions(-)include/linux/user.h +9
-0@@ -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 kernel/user.c +1
-1@@ -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
kernel/user.c static int may_setuid(struct user_namespace *ns, uid_t uid) · line 637
Quoted 7 lines
kernel/user.c +4
-0@@ -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.
kernel/user.c static int may_setuid(struct user_namespace *ns, uid_t uid) · line 639
Quoted 2 lines
kernel/user.c +2
-0+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...
Quoted 16 lines
kernel/user.c +6
-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;Signature · 6 lines
1.6.0.4 _______________________________________________ Containers mailing list Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org https://lists.linux-foundation.org/mailman/listinfo/containers
A message that is not in the archive (sent off-list or not yet received): 20090813222837.GB13219-r/[email protected]
Re: [PATCH 2/4] Expose may_setuid() in user.h
Quoted 70 lines · Serge E. Hallyn
Quoted 1 line · Dan Smith
Make this helper available to others.
No objection to exporting
may_setuid, nor to creatingmay_setgid(), but I don't thinkmay_setgid()is right. See belowQuoted 35 lines
Signed-off-by Dan Smith <danms-r/[email protected]>--- include/linux/user.h | 9 +++++++++ kernel/user.c | 16 +++++++++++++++- 2 files changed, 24 insertions(+), 1 deletions(-)include/linux/user.h +9
-0@@ -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 kernel/user.c +1
-1@@ -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 kernel/user.c static int may_setuid(struct user_namespace *ns, uid_t uid) · line 637
Quoted 7 lines
kernel/user.c +4
-0@@ -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_nsso we can eventually check forcapable_to(ns, CAP_SETGID). So the caller may not beCAP_SETGIDin the root user namespace, but may have created the child user namespace and be privileged there.kernel/user.c static int may_setuid(struct user_namespace *ns, uid_t uid) · line 639
Quoted 2 lines
kernel/user.c +2
-0+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 doin_egroup_p(g)for each.It's also possible that groups 1, 4, and 5 are in
current_group_infoand 6 iscurrent_egroup, while we're asking for agroup_infowith groups 1, 5 and 6, and egid of 4. That would be legal, right? Walking over the groups and doingin_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 keeplast_used_gin both groupinfos...
Quoted 2 lines
kernel/user.c static int may_setuid(struct user_namespace *ns, uid_t uid) · line 643
Quoted 2 lines
kernel/user.c +2
-0+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 2/4] Expose may_setuid() in user.h
In reply to a message not in the archiveQuoted 43 lines · Dan Smith
Make this helper available to others.
Signed-off-by Dan Smith <danms-r/[email protected]>--- include/linux/user.h | 9 +++++++++ kernel/user.c | 16 +++++++++++++++- 2 files changed, 24 insertions(+), 1 deletions(-)include/linux/user.h +9
-0@@ -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 kernel/user.c +4
-1@@ -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 ))
kernel/user.c static int may_setuid(struct user_namespace *ns, uid_t uid) · line 646
Quoted 10 lines
kernel/user.c +10
-0+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:
Quoted 10 lines
kernel/user.c +10
-0+int may_setgid (gid_t gid) +{ +if (capable (CAP_SETGID )) +return 1 ; + +if (in_egroup_p (gid)) +return 1 ; + +return 0 ; +}
PATCH 3/4 Save and restore UNIX socket peer credentials
In reply to a message not in the archiveThis 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.
--- include/linux/checkpoint_hdr.h | 2 ++ net/unix/checkpoint.c | 29 ++++++++++++++++------------- 2 files changed, 18 insertions(+), 13 deletions(-)
include/linux/checkpoint_hdr.h +2 -0
@@ -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;
net/unix/checkpoint.c +16 -13
@@ -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.
Signature · 1 line
1.6.0.4
A message that is not in the archive (sent off-list or not yet received): 1250191750-3864-4-git-send-email-danms-r/[email protected]
Re: [PATCH 3/4] Save and restore UNIX socket peer credentials
Quoted 66 lines · Dan Smith
This saves the uid/gid of the
sk_peercredstructure in the checkpoint stream. On restart, it usesmay_setuid()andmay_setgid()to determine if the uid/gid from the checkpoint stream may be used.Signed-off-by Dan Smith <danms-r/[email protected]>--- include/linux/checkpoint_hdr.h | 2 ++ net/unix/checkpoint.c | 29 ++++++++++++++++------------- 2 files changed, 18 insertions(+), 13 deletions(-)include/linux/checkpoint_hdr.h +2
-0@@ -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;net/unix/checkpoint.c +4
-13@@ -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))
net/unix/checkpoint.c static int sock_unix_restore_connected(struct ckpt_ctx *ctx, · line 302
Quoted 9 lines
net/unix/checkpoint.c +6
-0@@ -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
Quoted 16 lines
net/unix/checkpoint.c +6
-0+ }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. Signature · 6 lines
1.6.0.4 _______________________________________________ Containers mailing list Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org https://lists.linux-foundation.org/mailman/listinfo/containers
PATCH 4/4 Handle unconnected DGRAM sockets with buffers in-flight
In reply to a message not in the archiveThis 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.
--- net/unix/checkpoint.c | 41 +++++++++++++++++++++++++++-------------- 1 files changed, 27 insertions(+), 14 deletions(-)
net/unix/checkpoint.c +27 -14
@@ -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);
Signature · 1 line
1.6.0.4
A message that is not in the archive (sent off-list or not yet received): 1250191750-3864-5-git-send-email-danms-r/[email protected]
Re: [PATCH 4/4] Handle unconnected DGRAM sockets with buffers in-flight
Quoted 3 lines · 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.
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.
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH 4/4] Handle unconnected DGRAM sockets with buffers in-flight
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.
Signature · 3 lines
Dan Smith IBM Linux Technology Center email: danms-r/[email protected]
A message that is not in the archive (sent off-list or not yet received): 87my63phwp.fsf-FLMGYpZoEPULwtHQx/[email protected]
Re: [PATCH 4/4] Handle unconnected DGRAM sockets with buffers in-flight
Quoted 7 lines · Dan Smith
OL> Hmm.. I think this would break
recvfrom()syscall: it eventually OL> callsunix_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.
Quoted 12 lines
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->skof each pending packet, and checkpoint that OL> socket too.
Quoted 5 lines
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.