From: Kees Cook <keescook@chromium.org>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
davem@davemloft.net, kuba@kernel.org,
Miklos Szeredi <mszeredi@redhat.com>,
stable@vger.kernel.org
Subject: Re: [PATCH net] af_unix: fix garbage collect vs. MSG_PEEK
Date: Mon, 26 Jul 2021 12:27:01 -0700 [thread overview]
Message-ID: <202107261049.DC0C9178@keescook> (raw)
In-Reply-To: <20210726153621.2658658-1-gregkh@linuxfoundation.org>
On Mon, Jul 26, 2021 at 05:36:21PM +0200, Greg Kroah-Hartman wrote:
> From: Miklos Szeredi <mszeredi@redhat.com>
>
> Gc assumes that in-flight sockets that don't have an external ref can't
I think this commit log could be expanded. I had to really study things
to even beging to understand what was going on. I assume "Gc" here means
specifically unix_gc()?
> gain one while unix_gc_lock is held. That is true because
> unix_notinflight() will be called before detaching fds, which takes
> unix_gc_lock.
In reading the code, I *think* what is being protected by unix_gc_lock is
user->unix_inflight, u->inflight, unix_tot_inflight, and gc_inflight_list?
I note that unix_tot_inflight isn't an atomic but is read outside of
locking by unix_release_sock() and wait_for_unix_gc(), which seems wrong
(or at least inefficient).
But regardless, are the "external references" the f_count (i.e. get_file()
of u->sk.sk_socket->file) being changed by scm_fp_dup() and read by
unix_gc() (i.e. file_count())? It seems the test in unix_gc() is for
the making sure f_count isn't out of sync with u->inflight (is this the
corresponding "internal" reference?):
total_refs = file_count(u->sk.sk_socket->file);
inflight_refs = atomic_long_read(&u->inflight);
BUG_ON(inflight_refs < 1);
BUG_ON(total_refs < inflight_refs);
if (total_refs == inflight_refs) {
> Only MSG_PEEK was somehow overlooked. That one also clones the fds, also
> keeping them in the skb. But through MSG_PEEK an external reference can
> definitely be gained without ever touching unix_gc_lock.
The idea appears to be that all scm_fp_dup() callers need to refresh the
u->inflight counts which is what unix_attach_fds() and unix_detach_fds()
do. Why is lock/unlock sufficient for unix_peek_fds()?
I assume the rationale is because MSG_PEEK uses a temporary scm, which
only gets fput() clean-up on destroy ("inflight" is neither incremented
nor decremented at any point in the scm lifetime).
But I don't see why any of this helps.
unix_attach_fds():
fget(), spin_lock(), inflight++, spin_unlock()
unix_detach_fds():
spin_lock(), inflight--, spin_unlock(), fput()
unix_peek_fds():
fget(), spin_lock(), spin_unlock()
unix_gx():
spin_lock(), "total_refs == inflight_refs" to hitlist,
spin_unlock(), free hitlist skbs
Doesn't this mean total_refs and inflight_refs can still get out of
sync? What keeps an skb from being "visible" to unix_peek_fds() between
the unix_gx() spin_unlock() and the unix_peek_fds() fget()?
A: unix_gx():
spin_lock()
find "total_refs == inflight_refs", add to hitlist
spin_unlock()
B: unix_peek_fds():
fget()
A: unix_gc():
walk hitlist and free(skb)
B: unix_peek_fds():
*use freed skb*
I feel like I must be missing something since the above race would
appear to exist even for unix_attach_fds()/unix_detach_fds():
A: unix_gx():
spin_lock()
find "total_refs == inflight_refs", add to hitlist
spin_unlock()
B: unix_attach_fds():
fget()
A: unix_gc():
walk hitlist and free(skb)
B: unix_attach_fds():
*use freed skb*
I'm assuming I'm missing a top-level usage count on skb that is held by
callers, which means the skb isn't actually freed by unix_gc(). But I
return to not understanding why adding the lock/unlock helps.
What are the expected locking semantics here?
-Kees
>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> net/unix/af_unix.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> Note, this is a resend of this old submission that somehow fell through
> the cracks:
> https://lore.kernel.org/netdev/CAOssrKcfncAYsQWkfLGFgoOxAQJVT2hYVWdBA6Cw7hhO8RJ_wQ@mail.gmail.com/
> and was never submitted "properly" and this issue never seemed to get
> resolved properly.
>
> I've cleaned it up and made the change much smaller and localized to
> only one file. I kept Miklos's authorship as he did the hard work on
> this, I just removed lines and fixed a formatting issue :)
>
>
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 23c92ad15c61..cdea997aa5bf 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -1526,6 +1526,18 @@ static int unix_getname(struct socket *sock, struct sockaddr *uaddr, int peer)
> return err;
> }
>
> +static void unix_peek_fds(struct scm_cookie *scm, struct sk_buff *skb)
> +{
> + scm->fp = scm_fp_dup(UNIXCB(skb).fp);
> +
> + /* During garbage collection it is assumed that in-flight sockets don't
> + * get a new external reference. So we need to wait until current run
> + * finishes.
> + */
> + spin_lock(&unix_gc_lock);
> + spin_unlock(&unix_gc_lock);
> +}
--
Kees Cook
next prev parent reply other threads:[~2021-07-26 19:27 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-26 15:36 [PATCH net] af_unix: fix garbage collect vs. MSG_PEEK Greg Kroah-Hartman
2021-07-26 19:27 ` Kees Cook [this message]
2021-07-29 14:29 ` Miklos Szeredi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=202107261049.DC0C9178@keescook \
--to=keescook@chromium.org \
--cc=davem@davemloft.net \
--cc=gregkh@linuxfoundation.org \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mszeredi@redhat.com \
--cc=netdev@vger.kernel.org \
--cc=stable@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.