| Message ID | 56B0F574.5080105@stressinduktion.org |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, Feb 2, 2016 at 10:29 AM, Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: >> >> Anyway, can someone provide a high-level description of what exactly >> this patch is supposed to do? Which operation should be limited, who >> should inflight FDs be accounted on, and which rlimit should be used >> on each operation? I'm having a hard time auditing existing >> user-space, given just the scarce description of this commit. > > Yes, all your observations are true. I think we need to explicitly > need to refer to the sending socket while attaching the fds. I don't think that really helps. Maybe somebody passed a unix domain socket around, and now we're crediting the wrong socket again. So how about we actually add a "struct cred *" to the scm_cookie itself, and we initialize it to "get_current_cred()". And then always use that. That way it's always the person who actually does the send (rather than the opener of the socket _or_ the opener of the file that gets passed around) that gets credited, and thanks to the cred pointer we can then de-credit them properly. Hmm? Linus
On 02.02.2016 20:29, Linus Torvalds wrote: > On Tue, Feb 2, 2016 at 10:29 AM, Hannes Frederic Sowa > <hannes@stressinduktion.org> wrote: >>> >>> Anyway, can someone provide a high-level description of what exactly >>> this patch is supposed to do? Which operation should be limited, who >>> should inflight FDs be accounted on, and which rlimit should be used >>> on each operation? I'm having a hard time auditing existing >>> user-space, given just the scarce description of this commit. >> >> Yes, all your observations are true. I think we need to explicitly >> need to refer to the sending socket while attaching the fds. > > I don't think that really helps. Maybe somebody passed a unix domain > socket around, and now we're crediting the wrong socket again. I was struggling a bit what you meant but I think you are referring to the following scenario: process-1 opens up a unix socket and passes it to process-2 (this process has different credentials) via af-unix. Process-2 then sends multiple fds to another destination over this transferred unix-fd. True, in this case we again account the fds to the wrong process, which is bad. > So how about we actually add a "struct cred *" to the scm_cookie > itself, and we initialize it to "get_current_cred()". And then always > use that. Unfortunately we never transfer a scm_cookie via the skbs but merely use it to initialize unix_skb_parms structure in skb->cb and destroy it afterwards. But "struct pid *" in unix_skb_parms should be enough to get us to corresponding "struct cred *" so we can decrement the correct counter during skb destruction. So: We increment current task's unix_inflight and also check the current task's limit during attaching fds to skbs and decrement the inflight counter via "struct pid *". This looks like it should work. > That way it's always the person who actually does the send (rather > than the opener of the socket _or_ the opener of the file that gets > passed around) that gets credited, and thanks to the cred pointer we > can then de-credit them properly. Exactly, I try to implement that. Thanks a lot! Hannes
On Tue, Feb 02, 2016 at 09:32:56PM +0100, Hannes Frederic Sowa wrote: > But "struct pid *" in unix_skb_parms should be enough to get us to > corresponding "struct cred *" so we can decrement the correct counter > during skb destruction. > > So: > > We increment current task's unix_inflight and also check the current > task's limit during attaching fds to skbs and decrement the inflight > counter via "struct pid *". This looks like it should work. I like it as well, the principle sounds sane. > >That way it's always the person who actually does the send (rather > >than the opener of the socket _or_ the opener of the file that gets > >passed around) that gets credited, and thanks to the cred pointer we > >can then de-credit them properly. > > Exactly, I try to implement that. Thanks a lot! Thanks to you Hannes, I appreciate that you work on it, it would take much more time to me to dig into this. Willy
On Tue, Feb 2, 2016 at 12:32 PM, Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > > Unfortunately we never transfer a scm_cookie via the skbs but merely use it > to initialize unix_skb_parms structure in skb->cb and destroy it afterwards. Ok, I obviously didn't check very closely. > But "struct pid *" in unix_skb_parms should be enough to get us to > corresponding "struct cred *" so we can decrement the correct counter during > skb destruction. Umm. I think the "struct cred" may change in between, can't it? So I don't think you can later look up the cred based on the pid. Could we add the cred pointer (or just the user pointer) to the unix_skb_parms? Or maybe just add it to the "struct scm_fp_list"? Linus
On Tue, Feb 02, 2016 at 12:44:54PM -0800, Linus Torvalds wrote: > On Tue, Feb 2, 2016 at 12:32 PM, Hannes Frederic Sowa > <hannes@stressinduktion.org> wrote: > > But "struct pid *" in unix_skb_parms should be enough to get us to > > corresponding "struct cred *" so we can decrement the correct counter during > > skb destruction. > > Umm. I think the "struct cred" may change in between, can't it? You mean for example in case of setuid() or something like this ? willy
On Tue, Feb 2, 2016 at 12:49 PM, Willy Tarreau <w@1wt.eu> wrote: > On Tue, Feb 02, 2016 at 12:44:54PM -0800, Linus Torvalds wrote: >> >> Umm. I think the "struct cred" may change in between, can't it? > > You mean for example in case of setuid() or something like this ? Yeah. I'd be worried about looking up the creds or user structure later, and possibly getting a different one. I'd much rather look it up at attach time, and just carry an extra pointer around. That seems to be an inherently safer model where there's no worry about "what happens if the user does X in the meantime". Linus
On 02.02.2016 21:44, Linus Torvalds wrote: > On Tue, Feb 2, 2016 at 12:32 PM, Hannes Frederic Sowa > <hannes@stressinduktion.org> wrote: >> >> Unfortunately we never transfer a scm_cookie via the skbs but merely use it >> to initialize unix_skb_parms structure in skb->cb and destroy it afterwards. > > Ok, I obviously didn't check very closely. > >> But "struct pid *" in unix_skb_parms should be enough to get us to >> corresponding "struct cred *" so we can decrement the correct counter during >> skb destruction. > > Umm. I think the "struct cred" may change in between, can't it? While reviewing the task_struct->cred/real_cred assignments, I noticed that, too. I already went the same way and added a "struct cred *" to unix_skb_parms. > So I don't think you can later look up the cred based on the pid. Yep, it also looked to dangerous to me. > Could we add the cred pointer (or just the user pointer) to the unix_skb_parms? > > Or maybe just add it to the "struct scm_fp_list"? scm_fp_list seems to be an even better place. I have a look, thanks! Hannes
On Tue, Feb 02, 2016 at 12:53:20PM -0800, Linus Torvalds wrote: > On Tue, Feb 2, 2016 at 12:49 PM, Willy Tarreau <w@1wt.eu> wrote: > > On Tue, Feb 02, 2016 at 12:44:54PM -0800, Linus Torvalds wrote: > >> > >> Umm. I think the "struct cred" may change in between, can't it? > > > > You mean for example in case of setuid() or something like this ? > > Yeah. I'd be worried about looking up the creds or user structure > later, and possibly getting a different one. > > I'd much rather look it up at attach time, and just carry an extra > pointer around. That seems to be an inherently safer model where > there's no worry about "what happens if the user does X in the > meantime". Normally we can only change from root to non-root, and we don't apply the limits to root, so if we have the ability to only store one bit indicating "not tracked" or to simply nullify one pointer to avoid counting in flight FDs for root, we don't take the risk to recredit them to the target user after a change. I just don't know if we can do that though :-/ Willy
From: Linus Torvalds > Sent: 02 February 2016 20:45 > On Tue, Feb 2, 2016 at 12:32 PM, Hannes Frederic Sowa > <hannes@stressinduktion.org> wrote: > > > > Unfortunately we never transfer a scm_cookie via the skbs but merely use it > > to initialize unix_skb_parms structure in skb->cb and destroy it afterwards. > > Ok, I obviously didn't check very closely. > > > But "struct pid *" in unix_skb_parms should be enough to get us to > > corresponding "struct cred *" so we can decrement the correct counter during > > skb destruction. > > Umm. I think the "struct cred" may change in between, can't it? > > So I don't think you can later look up the cred based on the pid. > > Could we add the cred pointer (or just the user pointer) to the unix_skb_parms? > > Or maybe just add it to the "struct scm_fp_list"? Is that going to work if the sending process exits before the message is read? You need a reference count against the structure than contains the count. I think this is 'struct user' not 'struct cred' David
diff --git a/include/net/af_unix.h b/include/net/af_unix.h index 2a91a0561a4783..d7148ae04b02dd 100644 --- a/include/net/af_unix.h +++ b/include/net/af_unix.h @@ -6,8 +6,8 @@ #include <linux/mutex.h> #include <net/sock.h> -void unix_inflight(struct file *fp); -void unix_notinflight(struct file *fp); +void unix_inflight(struct sock *sk, struct file *fp); +void unix_notinflight(struct sock *sk, struct file *fp); void unix_gc(void); void wait_for_unix_gc(void); struct sock *unix_get_socket(struct file *filp); diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index 49d5093eb0553a..7bbb4762899924 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -1496,7 +1496,7 @@ static void unix_detach_fds(struct scm_cookie *scm, struct sk_buff *skb) UNIXCB(skb).fp = NULL; for (i = scm->fp->count-1; i >= 0; i--) - unix_notinflight(scm->fp->fp[i]); + unix_notinflight(skb->sk, scm->fp->fp[i]); } static void unix_destruct_scm(struct sk_buff *skb) @@ -1561,7 +1561,7 @@ static int unix_attach_fds(struct scm_cookie *scm, struct sk_buff *skb) return -ENOMEM; for (i = scm->fp->count - 1; i >= 0; i--) - unix_inflight(scm->fp->fp[i]); + unix_inflight(skb->sk, scm->fp->fp[i]); return max_level; } diff --git a/net/unix/garbage.c b/net/unix/garbage.c index 8fcdc2283af50c..874c42161717f0 100644 --- a/net/unix/garbage.c +++ b/net/unix/garbage.c @@ -116,7 +116,7 @@ struct sock *unix_get_socket(struct file *filp) * descriptor if it is for an AF_UNIX socket. */ -void unix_inflight(struct file *fp) +void unix_inflight(struct sock *sk, struct file *fp) { struct sock *s = unix_get_socket(fp); @@ -133,11 +133,11 @@ void unix_inflight(struct file *fp) } unix_tot_inflight++; } - fp->f_cred->user->unix_inflight++; + sk->sk_socket->file->f_cred->user->unix_inflight++; spin_unlock(&unix_gc_lock); } -void unix_notinflight(struct file *fp) +void unix_notinflight(struct sock *sk, struct file *fp) { struct sock *s = unix_get_socket(fp); @@ -152,7 +152,7 @@ void unix_notinflight(struct file *fp) list_del_init(&u->link); unix_tot_inflight--; } - fp->f_cred->user->unix_inflight--; + sk->sk_socket->file->f_cred->user->unix_inflight++; spin_unlock(&unix_gc_lock); }