Date:   Mon, 27 Feb 2023 20:32:26 +0200
From:   Leon Romanovsky <leon@kernel.org>
To:     Aleksandr Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
Cc:     davem@davemloft.net, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org, Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>
Subject: Re: [PATCH net-next] scm: fix MSG_CTRUNC setting condition for
 SO_PASSSEC
Message-ID: <Y/z3OtIA+25GjjH2@unreal>
References: <20230226201730.515449-1-aleksandr.mikhalitsyn@canonical.com>
 <Y/x8H4qCNsj4mEkA@unreal>
 <CAEivzxeorZoiE4VmJ45CoF4ZRoW3B+rkT0ufX7y1bxn510yzPQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAEivzxeorZoiE4VmJ45CoF4ZRoW3B+rkT0ufX7y1bxn510yzPQ@mail.gmail.com>
Precedence: bulk

On Mon, Feb 27, 2023 at 10:55:04AM +0100, Aleksandr Mikhalitsyn wrote:
> On Mon, Feb 27, 2023 at 10:47 AM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Sun, Feb 26, 2023 at 09:17:30PM +0100, Alexander Mikhalitsyn wrote:
> > > Currently, we set MSG_CTRUNC flag is we have no
> > > msg_control buffer provided and SO_PASSCRED is set
> > > or if we have pending SCM_RIGHTS.
> > >
> > > For some reason we have no corresponding check for
> > > SO_PASSSEC.
> > >
> > > Cc: "David S. Miller" <davem@davemloft.net>
> > > Cc: Eric Dumazet <edumazet@google.com>
> > > Cc: Jakub Kicinski <kuba@kernel.org>
> > > Cc: Paolo Abeni <pabeni@redhat.com>
> > > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> > > ---
> > >  include/net/scm.h | 13 ++++++++++++-
> > >  1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > Is it a bugfix? If yes, it needs Fixes line.
> 
> It's from 1da177e4c3 ("Linux-2.6.12-rc2") times :)
> I wasn't sure that it's correct to put the "Fixes" tag on such an old
> and big commit. Will do. Thanks!
> 
> >
> > >
> > > diff --git a/include/net/scm.h b/include/net/scm.h
> > > index 1ce365f4c256..585adc1346bd 100644
> > > --- a/include/net/scm.h
> > > +++ b/include/net/scm.h
> > > @@ -105,16 +105,27 @@ static inline void scm_passec(struct socket *sock, struct msghdr *msg, struct sc
> > >               }
> > >       }
> > >  }
> > > +
> > > +static inline bool scm_has_secdata(struct socket *sock)
> > > +{
> > > +     return test_bit(SOCK_PASSSEC, &sock->flags);
> > > +}
> > >  #else
> > >  static inline void scm_passec(struct socket *sock, struct msghdr *msg, struct scm_cookie *scm)
> > >  { }
> > > +
> > > +static inline bool scm_has_secdata(struct socket *sock)
> > > +{
> > > +     return false;
> > > +}
> > >  #endif /* CONFIG_SECURITY_NETWORK */
> >
> > There is no need in this ifdef, just test bit directly.
> 
> The problem is that even if the kernel is compiled without
> CONFIG_SECURITY_NETWORK
> userspace can still set the SO_PASSSEC option. IMHO it's better not to
> set MSG_CTRUNC
> if CONFIG_SECURITY_NETWORK is disabled, msg_control is not set but
> SO_PASSSEC is enabled.
> Because in this case SCM_SECURITY will never be sent. Please correct
> me if I'm wrong.

I don't know enough in this area to say if it is wrong or not.
My remark was due to the situation where user sets some bit which is
going to be ignored silently. It will be much cleaner do not set it
if CONFIG_SECURITY_NETWORK is disabled instead of masking its usage.

Thanks

> 
> Kind regards,
> Alex
> 
> >
> > >
> > >  static __inline__ void scm_recv(struct socket *sock, struct msghdr *msg,
> > >                               struct scm_cookie *scm, int flags)
> > >  {
> > >       if (!msg->msg_control) {
> > > -             if (test_bit(SOCK_PASSCRED, &sock->flags) || scm->fp)
> > > +             if (test_bit(SOCK_PASSCRED, &sock->flags) || scm->fp ||
> > > +                 scm_has_secdata(sock))
> > >                       msg->msg_flags |= MSG_CTRUNC;
> > >               scm_destroy(scm);
> > >               return;
> > > --
> > > 2.34.1
> > >