Re: [PATCH v2] selinux: do not report error on connect(AF_UNSPEC)

From: Paul Moore <paul@paul-moore.com>
To: Paolo Abeni <pabeni@redhat.com>
Cc: selinux@vger.kernel.org, Tom Deseyn <tdeseyn@redhat.com>,
	Richard Haines <richard_c_haines@btinternet.com>,
	Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Subject: Re: [PATCH v2] selinux: do not report error on connect(AF_UNSPEC)
Date: Fri, 10 May 2019 12:09:31 -0400	[thread overview]
Message-ID: <CAHC9VhQ3acCnoQP=eoERg64gyDo56xJ5dnPrkg20d8cdVuz2AQ@mail.gmail.com> (raw)
In-Reply-To: <8edfb95424cdf4bac06adbeb6b4204e5cc86f643.camel@redhat.com>

On Fri, May 10, 2019 at 11:52 AM Paolo Abeni <pabeni@redhat.com> wrote:
> On Fri, 2019-05-10 at 11:32 -0400, Paul Moore wrote:
> > On Fri, May 10, 2019 at 9:49 AM Paolo Abeni <pabeni@redhat.com> wrote:
> > > calling connect(AF_UNSPEC) on an already connected TCP socket is an
> > > established way to disconnect() such socket. After commit 68741a8adab9
> > > ("selinux: Fix ltp test connect-syscall failure") it no longer works
> > > and, in the above scenario connect() fails with EAFNOSUPPORT.
> > >
> > > Fix the above skipping the checks when the address family is not
> > > AF_INET{4,6} - we don't have any port to validate, but leave the
> > > SCTP code path untouched, as it has specific constraints.
> > >
> > > Fixes: 68741a8adab9 ("selinux: Fix ltp test connect-syscall failure")
> > > Reported-by: Tom Deseyn <tdeseyn@redhat.com>
> > > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > > ---
> > > v1 -> v2:
> > >  - avoid validation for AF_UNSPEC
> > > ---
> > >  security/selinux/hooks.c | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > What was wrong with explicitly checking for AF_UNSPEC as I mentioned
> > in my last email?
>
> Whoops sorry, I missed a relevant part of that email.
>
> Reading it now.
>
> To me, the 2 options look quite similar, and I have a slighter
> preference for this one, being a smaller change possibly more suited to
> a stable fix.
>
> But if you have strong different opinions I can post the code you
> suggested. I don't see any performance related issue with that.

My thinking is that this patch affects address families other than
AF_UNSPEC, whereas the fix I suggested only affects AF_UNSPEC.  Given
the compatibility problems we have had with this code recently, I
would prefer what I suggested since it has the least impact to
userspace.  It also has the benefit of solving AF_UNSPEC regardless of
the SELinux socket class and moving the addrlen check higher up the
function; these things alone aren't really a big deal, but they are
nice side effects.

-- 
paul moore
www.paul-moore.com