netdev - Re: [PATCH] net/af_unix: don't create a path for a binded socket

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20201127144811.4b68f8be@kicinski-fedora-pc1c0hjn.DHCP.thefacebook.com>
Date:   Fri, 27 Nov 2020 14:48:11 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Denis Kirjanov <kda@...ux-powerpc.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net
Subject: Re: [PATCH] net/af_unix: don't create a path for a binded socket

On Thu, 26 Nov 2020 17:22:08 +0300 Denis Kirjanov wrote:
> On 11/26/20, Jakub Kicinski <kuba@...nel.org> wrote:
> > On Tue, 24 Nov 2020 15:24:21 +0300 Denis Kirjanov wrote:  
> >> in the case of the socket which is bound to an adress
> >> there is no sense to create a path in the next attempts  
> >  
> >> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> >> index 41c3303c3357..fd76a8fe3907 100644
> >> --- a/net/unix/af_unix.c
> >> +++ b/net/unix/af_unix.c
> >> @@ -1021,7 +1021,7 @@ static int unix_bind(struct socket *sock, struct
> >> sockaddr *uaddr, int addr_len)
> >>
> >>  	err = -EINVAL;
> >>  	if (addr_len < offsetofend(struct sockaddr_un, sun_family) ||
> >> -	    sunaddr->sun_family != AF_UNIX)
> >> +	    sunaddr->sun_family != AF_UNIX || u->addr)
> >>  		goto out;
> >>
> >>  	if (addr_len == sizeof(short)) {
> >> @@ -1049,10 +1049,6 @@ static int unix_bind(struct socket *sock, struct
> >> sockaddr *uaddr, int addr_len)
> >>  	if (err)
> >>  		goto out_put;
> >>
> >> -	err = -EINVAL;
> >> -	if (u->addr)
> >> -		goto out_up;
> >> -
> >>  	err = -ENOMEM;
> >>  	addr = kmalloc(sizeof(*addr)+addr_len, GFP_KERNEL);
> >>  	if (!addr)  
> >
> > Well, after your change the check on u->addr is no longer protected by
> > u->bindlock. Is that okay?  
> 
> Since we're just checking the assigned address and it's an atomic
> operation I think it's okay.

The access to the variable may be atomic, but what protects two
concurrent binds() from progressing past the check and binding to
different paths?

I don't know this code at all, but looks to me like the pattern is
basically:

	lock()
	if (obj->thing)
		goto err; /* already bound to a thing */

	thing = alloc()
	setup_thing(thing);

	obj->thing = thing;
err:
	unlock()

> A process performing binding is still protected.

Isn't checking "did someone already bind" not part of the process of
binding?