diff mbox series

[net-next] scm: fix MSG_CTRUNC setting condition for SO_PASSSEC

Message ID 20230226201730.515449-1-aleksandr.mikhalitsyn@canonical.com
State New
Headers
Series [net-next] scm: fix MSG_CTRUNC setting condition for SO_PASSSEC |

Commit Message

Aleksandr Mikhalitsyn Feb. 26, 2023, 8:17 p.m. UTC 
  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(-)

Comments

Leon Romanovsky Feb. 27, 2023, 9:47 a.m. UTC | #1 
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.

> 
> 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.

>  
>  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
>
Aleksandr Mikhalitsyn Feb. 27, 2023, 9:55 a.m. UTC | #2 
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.

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
> >
Eric Dumazet Feb. 27, 2023, 10:01 a.m. UTC | #3 
On Sun, Feb 26, 2023 at 9:17 PM Alexander Mikhalitsyn
<aleksandr.mikhalitsyn@canonical.com> 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.

Can you describe what side effects this patch has ?

I think it could break some applications, who might not be able to
recover from MSG_CTRUNC in this case.
This should be documented, in order to avoid a future revert.

In any case, net-next is currently closed.
Aleksandr Mikhalitsyn Feb. 27, 2023, 10:21 a.m. UTC | #4 
On Mon, Feb 27, 2023 at 11:01 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Sun, Feb 26, 2023 at 9:17 PM Alexander Mikhalitsyn
> <aleksandr.mikhalitsyn@canonical.com> 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.
>

Hi Eric,

> Can you describe what side effects this patch has ?
>
> I think it could break some applications, who might not be able to
> recover from MSG_CTRUNC in this case.
> This should be documented, in order to avoid a future revert.

Yes, it can break applications but only those who use SO_PASSSEC
and not properly check MSG_CTRUNC. According to the recv(2) man:

       MSG_CTRUNC
              indicates that some control data was discarded due to lack
              of space in the buffer for ancillary data.

So, there is no specification about a particular SCM type. It seems more correct
to handle SCM_SECURITY the same way as SCM_RIGHTS / SCM_CREDENTIALS.

>
> In any case, net-next is currently closed.

Oh, I'm sorry.

Kind regards,
Alex
Leon Romanovsky Feb. 27, 2023, 6:32 p.m. UTC | #5 
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
> > >
Aleksandr Mikhalitsyn Feb. 28, 2023, 10:06 a.m. UTC | #6 
On Mon, Feb 27, 2023 at 7:32 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> 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.

Hi Leon,

I agree with you, but IMHO then it looks more correct to return -EOPNOTSUPP on
setsockopt(fd, SO_PASSSEC, ...) if CONFIG_SECURITY_NETWORK is disabled.
But such a change may break things.

Okay, anyway I'll wait until net-next will be opened and present a
patch with a more
detailed description and Fixes tag. Speaking about this problem with
CONFIG_SECURITY_NETWORK
if you insist that it will be more correct then I'm ready to fix it too.

Thanks,
Alex

>
> 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
> > > >
Leon Romanovsky Feb. 28, 2023, 2:45 p.m. UTC | #7 
On Tue, Feb 28, 2023 at 11:06:12AM +0100, Aleksandr Mikhalitsyn wrote:
> On Mon, Feb 27, 2023 at 7:32 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > 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.
> 
> Hi Leon,
> 
> I agree with you, but IMHO then it looks more correct to return -EOPNOTSUPP on
> setsockopt(fd, SO_PASSSEC, ...) if CONFIG_SECURITY_NETWORK is disabled.
> But such a change may break things.
> 
> Okay, anyway I'll wait until net-next will be opened and present a
> patch with a more
> detailed description and Fixes tag. Speaking about this problem with
> CONFIG_SECURITY_NETWORK
> if you insist that it will be more correct then I'm ready to fix it too.

I won't insist on anything, most likely Eric will comment if you need to
fix it.

Thanks

> 
> Thanks,
> Alex
> 
> >
> > 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
> > > > >
Aleksandr Mikhalitsyn Feb. 28, 2023, 3:10 p.m. UTC | #8 
On Tue, Feb 28, 2023 at 3:45 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Tue, Feb 28, 2023 at 11:06:12AM +0100, Aleksandr Mikhalitsyn wrote:
> > On Mon, Feb 27, 2023 at 7:32 PM Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > 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.
> >
> > Hi Leon,
> >
> > I agree with you, but IMHO then it looks more correct to return -EOPNOTSUPP on
> > setsockopt(fd, SO_PASSSEC, ...) if CONFIG_SECURITY_NETWORK is disabled.
> > But such a change may break things.
> >
> > Okay, anyway I'll wait until net-next will be opened and present a
> > patch with a more
> > detailed description and Fixes tag. Speaking about this problem with
> > CONFIG_SECURITY_NETWORK
> > if you insist that it will be more correct then I'm ready to fix it too.
>
> I won't insist on anything, most likely Eric will comment if you need to
> fix it.

Got it.

Thanks a lot for your attention to the patch!

Kind regards,
Alex

>
> Thanks
>
> >
> > Thanks,
> > Alex
> >
> > >
> > > 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
> > > > > >
diff mbox series

Patch

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 */
 
 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;