On Thu, May 14, 2020 at 7:25 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> Signed-off-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
> cc: netdev@xxxxxxxxxxxxxxx
> ---
> 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.
