Re: [PATCH v2 2/2] selinux: Implement mptcp_add_subflow hook

[Paul Moore <paul@paul-moore.com>]

On Mon, Dec 19, 2022 at 12:34 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> Newly added subflows should inherit the associated label
> from the current process context, regarless of the sk_kern_sock
> flag value.
>
> This patch implements the above resetting the subflow sid, deleting
> the existing subflow label, if any, and then re-creating a new one.
>
> The new helper reuses the selinux_netlbl_sk_security_free() function,
> and it can end-up being called multiple times with the same argument;
> we additionally need to make it idempotent.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> v1 -> v2:
>  - fix build issue with !CONFIG_NETLABEL
> ---
>  security/selinux/hooks.c    | 27 +++++++++++++++++++++++++++
>  security/selinux/netlabel.c |  4 +++-
>  2 files changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 3c5be76a9199..f785600b666a 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -5476,6 +5476,32 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk
>         selinux_netlbl_sctp_sk_clone(sk, newsk);
>  }
>
> +static int selinux_mptcp_add_subflow(struct sock *sk, struct sock *ssk)
> +{
> +       const struct task_security_struct *tsec = selinux_cred(current_cred());
> +       struct sk_security_struct *ssksec = ssk->sk_security;
> +       u16 sclass;
> +       u32 sid;
> +       int err;
> +
> +       /* create the sid using the current cred, regardless of the ssk kern
> +        * flag
> +        */
> +       sclass = socket_type_to_security_class(ssk->sk_family, ssk->sk_type,
> +                                              ssk->sk_protocol);
> +       err = socket_sockcreate_sid(tsec, sclass, &sid);
> +       if (err)
> +               return err;
> +
> +       ssksec->sid = sid;
> +
> +       /* replace the existing subflow label deleting the existing one
> +        * and re-recrating a new label using the current context
> +        */
> +       selinux_netlbl_sk_security_free(ssksec);
> +       return selinux_netlbl_socket_post_create(ssk, ssk->sk_family);
> +}

I thought the idea was to ensure that new subflows of an existing
MPTCP connection would be created with the same label as the main
MPTCP connection socket?  The code above labels the new subflow based
on the current process, not the main MPTCP connection; it matches the
commit description, but not what we had previously discussed - or I am
horribly mis-remembering something? :)

I was expecting something more like the following:

static int selinux_mptcp_add_subflow(...)
{
  struct sk_security_struct *sksec = sk->sk_security;
  struct sk_security_struct *ssksec = ssk->sk_security;

  ssksec->sclass = sksec->sclass;
  ssksec->sid = sksec->sid;

  selinux_netlbl_sk_security_free(ssksec);
  selinux_netlbl_socket_post_create(ssk, ssk->sk_family);
}

> diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c
> index 1321f15799e2..8e0080b8a8ef 100644
> --- a/security/selinux/netlabel.c
> +++ b/security/selinux/netlabel.c
> @@ -155,8 +155,10 @@ void selinux_netlbl_err(struct sk_buff *skb, u16 family, int error, int gateway)
>   */
>  void selinux_netlbl_sk_security_free(struct sk_security_struct *sksec)
>  {
> -       if (sksec->nlbl_secattr != NULL)
> +       if (sksec->nlbl_secattr != NULL) {
>                 netlbl_secattr_free(sksec->nlbl_secattr);
> +               sksec->nlbl_secattr = NULL;
> +       }
>  }

This is pretty nitpicky, but it might be a little cleaner to use the
pattern below.  At the very least I think it tends to better match a
lot of the various free helpers in the kernel.

void free_stuff(void *ptr)
{
  if (!ptr)
    return;
  free_properly(ptr);
}

I would probably also reset sk_security_struct::nlbl_state too, so
maybe something like the following:

void selinux_netlbl_sk_security_free(...)
{
  if (!sksec)
    return;
  netlbl_secattr_free(sksec->nlbl_secattr);
  sksec->nlbl_secattr = NULL;
  sksec->nlbl_state = NLBL_UNSET;
}

-- 
paul-moore.com