netdev - Re: [PATCH v2] 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: <20201201100422.u4ymvdxbzymlkoqu@lion.mk-sys.cz>
Date:   Tue, 1 Dec 2020 11:04:22 +0100
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Denis Kirjanov <kda@...ux-powerpc.org>,
        Al Viro <viro@...iv.linux.org.uk>, netdev@...r.kernel.org,
        davem@...emloft.net, Christoph Hellwig <hch@....de>
Subject: Re: [PATCH v2] net/af_unix: don't create a path for a binded socket

On Mon, Nov 30, 2020 at 05:30:00PM -0800, Jakub Kicinski wrote:
> On Mon, 30 Nov 2020 16:27:47 +0300 Denis Kirjanov wrote:
> > in the case of the socket which is bound to an adress
                                                    ^ address
> > there is no sense to create a path in the next attempts
> > 
> > here is a program that shows the issue:
> > 
> > int main()
> > {
> >     int s;
> >     struct sockaddr_un a;
> > 
> >     s = socket(AF_UNIX, SOCK_STREAM, 0);
> >     if (s<0)
> >         perror("socket() failed\n");
> > 
> >     printf("First bind()\n");
> > 
> >     memset(&a, 0, sizeof(a));
> >     a.sun_family = AF_UNIX;
> >     strncpy(a.sun_path, "/tmp/.first_bind", sizeof(a.sun_path));
> > 
> >     if ((bind(s, (const struct sockaddr*) &a, sizeof(a))) == -1)
> >         perror("bind() failed\n");
> > 
> >     printf("Second bind()\n");
> > 
> >     memset(&a, 0, sizeof(a));
> >     a.sun_family = AF_UNIX;
> >     strncpy(a.sun_path, "/tmp/.first_bind_failed", sizeof(a.sun_path));
> > 
> >     if ((bind(s, (const struct sockaddr*) &a, sizeof(a))) == -1)
> >         perror("bind() failed\n");
> > }
> > 
> > kda@...S15-SP2:~> ./test
> > First bind()
> > Second bind()
> > bind() failed
> > : Invalid argument
> > 
> > kda@...S15-SP2:~> ls -la /tmp/.first_bind
> > .first_bind         .first_bind_failed
> > 
> > Signed-off-by: Denis Kirjanov <kda@...ux-powerpc.org>
> > 
> > v2: move a new patch creation after the address assignment check.
                      ^ path

> 
> It is a behavior change, but IDK if anyone can reasonably depend on
> current behavior for anything useful. Otherwise LGTM.

For the record, we already changed the error code in this case once by
commit 0fb44559ffd6 ("af_unix: move unix_mknod() out of bindlock"), this
should revert it to the original value. As far as I'm aware, only LTP
guys noticed back then. I tried to raise the question in this list but
nobody seemed to care so I didn't pursue.

Technically, both errors are correct as both "address already in use"
and "socket already bound" conditions are met and the manual page
(neither linux nor posix) doesn't seem to specify which should take
precedence.

Michal