| Message ID | m1d3vvgirx.fsf@fess.ebiederm.org |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
> +static __inline__ void scm_destroy_cred(struct scm_cookie *scm) > +{ > + put_pid(scm->pid); > + scm->pid = NULL; > + > + if (scm->cred) > + put_cred(scm->cred); > + scm->cred = NULL; > +} > + > static __inline__ void scm_destroy(struct scm_cookie *scm) > { > + scm_destroy_cred(scm); > if (scm && scm->fp) > __scm_destroy(scm); > } I'm a bit worried by the "if (scm" check. It makes me think scm can be NULL here and thus scm_destroy_cred can oops. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Pavel Emelyanov <xemul@openvz.org> writes: >> +static __inline__ void scm_destroy_cred(struct scm_cookie *scm) >> +{ >> + put_pid(scm->pid); >> + scm->pid = NULL; >> + >> + if (scm->cred) >> + put_cred(scm->cred); >> + scm->cred = NULL; >> +} >> + >> static __inline__ void scm_destroy(struct scm_cookie *scm) >> { >> + scm_destroy_cred(scm); >> if (scm && scm->fp) >> __scm_destroy(scm); >> } > > I'm a bit worried by the "if (scm" check. It makes me think scm can > be NULL here and thus scm_destroy_cred can oops. Interesting point. I just looked at all of the callers and scm appears to always be valid. Eric -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Eric W. Biederman (ebiederm@xmission.com): > > Start capturing not only the userspace pid, uid and gid values of the > sending process but also the struct pid and struct cred of the sending > process as well. > > This is in preparation for properly supporting SCM_CREDENTIALS for > sockets that have different uid and/or pid namespaces at the different > ends. > > Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> > --- > include/net/scm.h | 28 ++++++++++++++++++++++++---- > net/core/scm.c | 24 ++++++++++++++++++++++++ > 2 files changed, 48 insertions(+), 4 deletions(-) > > diff --git a/include/net/scm.h b/include/net/scm.h > index 17d9d2e..3165650 100644 > --- a/include/net/scm.h > +++ b/include/net/scm.h > @@ -19,6 +19,8 @@ struct scm_fp_list { > }; > > struct scm_cookie { > + struct pid *pid; /* Skb credentials */ > + const struct cred *cred; > struct scm_fp_list *fp; /* Passed files */ > struct ucred creds; /* Skb credentials */ > #ifdef CONFIG_SECURITY_NETWORK > @@ -42,8 +44,27 @@ static __inline__ void unix_get_peersec_dgram(struct socket *sock, struct scm_co > { } > #endif /* CONFIG_SECURITY_NETWORK */ > > +static __inline__ void scm_set_cred(struct scm_cookie *scm, > + struct pid *pid, const struct cred *cred) > +{ > + scm->pid = get_pid(pid); > + scm->cred = get_cred(cred); > + cred_to_ucred(pid, cred, &scm->creds); > +} > + > +static __inline__ void scm_destroy_cred(struct scm_cookie *scm) > +{ > + put_pid(scm->pid); > + scm->pid = NULL; > + > + if (scm->cred) > + put_cred(scm->cred); > + scm->cred = NULL; > +} > + > static __inline__ void scm_destroy(struct scm_cookie *scm) > { > + scm_destroy_cred(scm); > if (scm && scm->fp) > __scm_destroy(scm); > } > @@ -51,10 +72,7 @@ static __inline__ void scm_destroy(struct scm_cookie *scm) > static __inline__ int scm_send(struct socket *sock, struct msghdr *msg, > struct scm_cookie *scm) > { > - struct task_struct *p = current; > - scm->creds.uid = current_uid(); > - scm->creds.gid = current_gid(); > - scm->creds.pid = task_tgid_vnr(p); > + scm_set_cred(scm, task_tgid(current), current_cred()); > scm->fp = NULL; > unix_get_peersec_dgram(sock, scm); > if (msg->msg_controllen <= 0) > @@ -96,6 +114,8 @@ static __inline__ void scm_recv(struct socket *sock, struct msghdr *msg, > if (test_bit(SOCK_PASSCRED, &sock->flags)) > put_cmsg(msg, SOL_SOCKET, SCM_CREDENTIALS, sizeof(scm->creds), &scm->creds); > > + scm_destroy_cred(scm); > + > scm_passec(sock, msg, scm); > > if (!scm->fp) > diff --git a/net/core/scm.c b/net/core/scm.c > index b88f6f9..681c976 100644 > --- a/net/core/scm.c > +++ b/net/core/scm.c > @@ -170,6 +170,30 @@ int __scm_send(struct socket *sock, struct msghdr *msg, struct scm_cookie *p) > err = scm_check_creds(&p->creds); > if (err) > goto error; > + I think this hunk needs to be documented. I.e. given that scm_send() will call scm_set_cred() before calling __scm_send, I don't see how these conditions could happen? If the condition can legitimately happen, then given all of the pid_t vs struct pid and 'cred' vs. 'creds' in these two hunks, I think a comment over each would be nice. > + if (pid_vnr(p->pid) != p->creds.pid) { > + struct pid *pid; > + err = -ESRCH; > + pid = find_get_pid(p->creds.pid); > + if (!pid) > + goto error; > + put_pid(p->pid); > + p->pid = pid; > + } > + > + if ((p->cred->euid != p->creds.uid) || > + (p->cred->egid != p->creds.gid)) { > + struct cred *cred; > + err = -ENOMEM; > + cred = prepare_creds(); > + if (!cred) > + goto error; > + > + cred->uid = cred->euid = p->creds.uid; > + cred->gid = cred->egid = p->creds.uid; > + put_cred(p->cred); > + p->cred = cred; > + } > break; > default: > goto error; > -- > 1.6.5.2.143.g8cc62 > > _______________________________________________ > Containers mailing list > Containers@lists.linux-foundation.org > https://lists.linux-foundation.org/mailman/listinfo/containers -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
"Serge E. Hallyn" <serge@hallyn.com> writes: > Quoting Eric W. Biederman (ebiederm@xmission.com): >> >> Start capturing not only the userspace pid, uid and gid values of the >> sending process but also the struct pid and struct cred of the sending >> process as well. >> >> This is in preparation for properly supporting SCM_CREDENTIALS for >> sockets that have different uid and/or pid namespaces at the different >> ends. >> >> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> >> --- >> include/net/scm.h | 28 ++++++++++++++++++++++++---- >> net/core/scm.c | 24 ++++++++++++++++++++++++ >> 2 files changed, 48 insertions(+), 4 deletions(-) >> >> diff --git a/include/net/scm.h b/include/net/scm.h >> index 17d9d2e..3165650 100644 >> --- a/include/net/scm.h >> +++ b/include/net/scm.h >> @@ -19,6 +19,8 @@ struct scm_fp_list { >> }; >> >> struct scm_cookie { >> + struct pid *pid; /* Skb credentials */ >> + const struct cred *cred; >> struct scm_fp_list *fp; /* Passed files */ >> struct ucred creds; /* Skb credentials */ >> #ifdef CONFIG_SECURITY_NETWORK >> @@ -42,8 +44,27 @@ static __inline__ void unix_get_peersec_dgram(struct socket *sock, struct scm_co >> { } >> #endif /* CONFIG_SECURITY_NETWORK */ >> >> +static __inline__ void scm_set_cred(struct scm_cookie *scm, >> + struct pid *pid, const struct cred *cred) >> +{ >> + scm->pid = get_pid(pid); >> + scm->cred = get_cred(cred); >> + cred_to_ucred(pid, cred, &scm->creds); >> +} >> + >> +static __inline__ void scm_destroy_cred(struct scm_cookie *scm) >> +{ >> + put_pid(scm->pid); >> + scm->pid = NULL; >> + >> + if (scm->cred) >> + put_cred(scm->cred); >> + scm->cred = NULL; >> +} >> + >> static __inline__ void scm_destroy(struct scm_cookie *scm) >> { >> + scm_destroy_cred(scm); >> if (scm && scm->fp) >> __scm_destroy(scm); >> } >> @@ -51,10 +72,7 @@ static __inline__ void scm_destroy(struct scm_cookie *scm) >> static __inline__ int scm_send(struct socket *sock, struct msghdr *msg, >> struct scm_cookie *scm) >> { >> - struct task_struct *p = current; >> - scm->creds.uid = current_uid(); >> - scm->creds.gid = current_gid(); >> - scm->creds.pid = task_tgid_vnr(p); >> + scm_set_cred(scm, task_tgid(current), current_cred()); >> scm->fp = NULL; >> unix_get_peersec_dgram(sock, scm); >> if (msg->msg_controllen <= 0) >> @@ -96,6 +114,8 @@ static __inline__ void scm_recv(struct socket *sock, struct msghdr *msg, >> if (test_bit(SOCK_PASSCRED, &sock->flags)) >> put_cmsg(msg, SOL_SOCKET, SCM_CREDENTIALS, sizeof(scm->creds), &scm->creds); >> >> + scm_destroy_cred(scm); >> + >> scm_passec(sock, msg, scm); >> >> if (!scm->fp) >> diff --git a/net/core/scm.c b/net/core/scm.c >> index b88f6f9..681c976 100644 >> --- a/net/core/scm.c >> +++ b/net/core/scm.c >> @@ -170,6 +170,30 @@ int __scm_send(struct socket *sock, struct msghdr *msg, struct scm_cookie *p) >> err = scm_check_creds(&p->creds); >> if (err) >> goto error; >> + > > I think this hunk needs to be documented. I.e. given that scm_send() > will call scm_set_cred() before calling __scm_send, I don't see how > these conditions could happen? If the condition can legitimately > happen, then given all of the pid_t vs struct pid and 'cred' vs. 'creds' > in these two hunks, I think a comment over each would be nice. I think if you have the full context of __scm_send it becomes pretty obvious. case SCM_CREDENTIALS: if (cmsg->cmsg_len != CMSG_LEN(sizeof(struct ucred))) goto error; memcpy(&p->creds, CMSG_DATA(cmsg), sizeof(struct ucred)); err = scm_check_creds(&p->creds); if (err) goto error; At this point we have just copied ucred from userspace. We have done scm_check_creds to ensure we allow the user to send the pid, uid, and gid they have passed in. These tests catch the case where the user is legitimately sending something other than their own credentials. >> + if (pid_vnr(p->pid) != p->creds.pid) { >> + struct pid *pid; >> + err = -ESRCH; >> + pid = find_get_pid(p->creds.pid); >> + if (!pid) >> + goto error; >> + put_pid(p->pid); >> + p->pid = pid; >> + } >> + >> + if ((p->cred->euid != p->creds.uid) || >> + (p->cred->egid != p->creds.gid)) { >> + struct cred *cred; >> + err = -ENOMEM; >> + cred = prepare_creds(); >> + if (!cred) >> + goto error; >> + >> + cred->uid = cred->euid = p->creds.uid; >> + cred->gid = cred->egid = p->creds.uid; >> + put_cred(p->cred); >> + p->cred = cred; >> + } >> break; >> default: >> goto error; Eric -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Eric W. Biederman (ebiederm@xmission.com): > "Serge E. Hallyn" <serge@hallyn.com> writes: > > I think this hunk needs to be documented. I.e. given that scm_send() > > will call scm_set_cred() before calling __scm_send, I don't see how > > these conditions could happen? If the condition can legitimately > > happen, then given all of the pid_t vs struct pid and 'cred' vs. 'creds' > > in these two hunks, I think a comment over each would be nice. > > I think if you have the full context of __scm_send it becomes pretty obvious. > > case SCM_CREDENTIALS: > if (cmsg->cmsg_len != CMSG_LEN(sizeof(struct ucred))) > goto error; > memcpy(&p->creds, CMSG_DATA(cmsg), sizeof(struct ucred)); > err = scm_check_creds(&p->creds); > if (err) > goto error; > > At this point we have just copied ucred from userspace. We have done > scm_check_creds to ensure we allow the user to send the pid, uid, and > gid they have passed in. > > These tests catch the case where the user is legitimately sending > something other than their own credentials. Of course. Sorry. And I even had the context in the window next to the email... So finally, Acked-by: Serge E. Hallyn <serge@hallyn.com> to the set, and I'm looking forward to this being in. And it should solve the nuisance of containers without private netns rebooting their hosts when both use upstart. thanks, -serge -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/net/scm.h b/include/net/scm.h index 17d9d2e..3165650 100644 --- a/include/net/scm.h +++ b/include/net/scm.h @@ -19,6 +19,8 @@ struct scm_fp_list { }; struct scm_cookie { + struct pid *pid; /* Skb credentials */ + const struct cred *cred; struct scm_fp_list *fp; /* Passed files */ struct ucred creds; /* Skb credentials */ #ifdef CONFIG_SECURITY_NETWORK @@ -42,8 +44,27 @@ static __inline__ void unix_get_peersec_dgram(struct socket *sock, struct scm_co { } #endif /* CONFIG_SECURITY_NETWORK */ +static __inline__ void scm_set_cred(struct scm_cookie *scm, + struct pid *pid, const struct cred *cred) +{ + scm->pid = get_pid(pid); + scm->cred = get_cred(cred); + cred_to_ucred(pid, cred, &scm->creds); +} + +static __inline__ void scm_destroy_cred(struct scm_cookie *scm) +{ + put_pid(scm->pid); + scm->pid = NULL; + + if (scm->cred) + put_cred(scm->cred); + scm->cred = NULL; +} + static __inline__ void scm_destroy(struct scm_cookie *scm) { + scm_destroy_cred(scm); if (scm && scm->fp) __scm_destroy(scm); } @@ -51,10 +72,7 @@ static __inline__ void scm_destroy(struct scm_cookie *scm) static __inline__ int scm_send(struct socket *sock, struct msghdr *msg, struct scm_cookie *scm) { - struct task_struct *p = current; - scm->creds.uid = current_uid(); - scm->creds.gid = current_gid(); - scm->creds.pid = task_tgid_vnr(p); + scm_set_cred(scm, task_tgid(current), current_cred()); scm->fp = NULL; unix_get_peersec_dgram(sock, scm); if (msg->msg_controllen <= 0) @@ -96,6 +114,8 @@ static __inline__ void scm_recv(struct socket *sock, struct msghdr *msg, if (test_bit(SOCK_PASSCRED, &sock->flags)) put_cmsg(msg, SOL_SOCKET, SCM_CREDENTIALS, sizeof(scm->creds), &scm->creds); + scm_destroy_cred(scm); + scm_passec(sock, msg, scm); if (!scm->fp) diff --git a/net/core/scm.c b/net/core/scm.c index b88f6f9..681c976 100644 --- a/net/core/scm.c +++ b/net/core/scm.c @@ -170,6 +170,30 @@ int __scm_send(struct socket *sock, struct msghdr *msg, struct scm_cookie *p) err = scm_check_creds(&p->creds); if (err) goto error; + + if (pid_vnr(p->pid) != p->creds.pid) { + struct pid *pid; + err = -ESRCH; + pid = find_get_pid(p->creds.pid); + if (!pid) + goto error; + put_pid(p->pid); + p->pid = pid; + } + + if ((p->cred->euid != p->creds.uid) || + (p->cred->egid != p->creds.gid)) { + struct cred *cred; + err = -ENOMEM; + cred = prepare_creds(); + if (!cred) + goto error; + + cred->uid = cred->euid = p->creds.uid; + cred->gid = cred->egid = p->creds.uid; + put_cred(p->cred); + p->cred = cred; + } break; default: goto error;
Start capturing not only the userspace pid, uid and gid values of the sending process but also the struct pid and struct cred of the sending process as well. This is in preparation for properly supporting SCM_CREDENTIALS for sockets that have different uid and/or pid namespaces at the different ends. Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> --- include/net/scm.h | 28 ++++++++++++++++++++++++---- net/core/scm.c | 24 ++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 4 deletions(-)