Re: [PATCH v17 05/23] net: Prepare UDS for security module stacking
On Thu, May 14, 2020 at 7:25 PM Casey Schaufler <casey(a)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(a)chromium.org>
Signed-off-by: Casey Schaufler <casey(a)schaufler-ca.com>
cc: netdev(a)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.