[PATCH v17 05/23] net: Prepare UDS for security module stacking

[Stephen Smalley]

On Thu, May 14, 2020 at 7:25 PM Casey Schaufler <casey at schaufler-ca.com> wrote:
>
> Change the data used in UDS SO_PEERSEC processing from a
> secid to a more general struct lsmblob. Update the
> security_socket_getpeersec_dgram() interface to use the
> lsmblob. There is a small amount of scaffolding code
> that will come out when the security_secid_to_secctx()
> code is brought in line with the lsmblob.
>
> The secid field of the unix_skb_parms structure has been
> replaced with a pointer to an lsmblob structure, and the
> lsmblob is allocated as needed. This is similar to how the
> list of passed files is managed. While an lsmblob structure
> will fit in the available space today, there is no guarantee
> that the addition of other data to the unix_skb_parms or
> support for additional security modules wouldn't exceed what
> is available.

I preferred the previous approach (in v15 and earlier) but I see that
this was suggested by Paul.  Lifecycle management of lsmdata seems
rather tenuous. I guess the real question is what does netdev prefer.
Regardless, you need to check for memory allocation failure below if
this approach stands.

> Reviewed-by: Kees Cook <keescook at chromium.org>
> Signed-off-by: Casey Schaufler <casey at schaufler-ca.com>
> cc: netdev at vger.kernel.org
> ---

> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 3385a7a0b231..a5c1a029095d 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -138,17 +138,18 @@ static struct hlist_head *unix_sockets_unbound(void *addr)
>  #ifdef CONFIG_SECURITY_NETWORK
>  static void unix_get_secdata(struct scm_cookie *scm, struct sk_buff *skb)
>  {
> -       UNIXCB(skb).secid = scm->secid;
> +       UNIXCB(skb).lsmdata = kmemdup(&scm->lsmblob, sizeof(scm->lsmblob),
> +                                     GFP_KERNEL);
>  }

Somewhere you need to check for and handle kmemdup() failure here.

>
>  static inline void unix_set_secdata(struct scm_cookie *scm, struct sk_buff *skb)
>  {
> -       scm->secid = UNIXCB(skb).secid;
> +       scm->lsmblob = *(UNIXCB(skb).lsmdata);
>  }

Lest we have a bad day here.

>
>  static inline bool unix_secdata_eq(struct scm_cookie *scm, struct sk_buff *skb)
>  {
> -       return (scm->secid == UNIXCB(skb).secid);
> +       return lsmblob_equal(&scm->lsmblob, UNIXCB(skb).lsmdata);
>  }

Or here.

> diff --git a/net/unix/scm.c b/net/unix/scm.c
> index 8c40f2b32392..3094323935a4 100644
> --- a/net/unix/scm.c
> +++ b/net/unix/scm.c
> @@ -142,6 +142,12 @@ void unix_destruct_scm(struct sk_buff *skb)
>         scm.pid  = UNIXCB(skb).pid;
>         if (UNIXCB(skb).fp)
>                 unix_detach_fds(&scm, skb);
> +#ifdef CONFIG_SECURITY_NETWORK
> +       if (UNIXCB(skb).lsmdata) {
> +               kfree(UNIXCB(skb).lsmdata);
> +               UNIXCB(skb).lsmdata = NULL;
> +       }
> +#endif

Does this suffice to ensure that lsmdata is always freed?  Seems
weakly connected to the allocation.