Re: [PATCH net] af_unix: fix garbage collect vs. MSG_PEEK

On Mon, Jul 26, 2021 at 9:27 PM Kees Cook [off-list ref] wrote:

On Mon, Jul 26, 2021 at 05:36:21PM +0200, Greg Kroah-Hartman wrote:

quoted

From: Miklos Szeredi <redacted>

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()?

Yeah, the original description was not too good.  Commit cbcf01128d0a
("af_unix: fix garbage collect vs MSG_PEEK") now in Linus' tree has a
much expanded description.

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).

I don't think it matters in practice.   Do you have specific worries?

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():

What you are missing is that anything that could have been peeked must
not have been garbage collected.  I.e. the garbage collection
algorithm will find that there's an external in-flight reference to
the peeked socket and so it will not add it to the hitlist.

Thanks,
Miklos