On Mon, Jul 26, 2021 at 9:27 PM Kees Cook keescook@chromium.org wrote:
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()?
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