| Message ID | a5ac15fc-440e-4483-825a-2d6f08083af3@schaufler-ca.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [lsm/dev] net: corrections for security_secid_to_secctx returns | expand |
On Dec 10, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote: > > security_secid_to_secctx() returns the size of the new context, > whereas previous versions provided that via a pointer parameter. > Correct the type of the value returned in nfqnl_get_sk_secctx() > and the check for error in netlbl_unlhsh_add(). > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > --- > net/netfilter/nfnetlink_queue.c | 8 ++++---- > net/netlabel/netlabel_unlabeled.c | 6 +++--- > 2 files changed, 7 insertions(+), 7 deletions(-) This should also have a fixes tag for 2d470c778120 ("lsm: replace context+len with lsm_context"), what do you think? > diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c > index 5110f29b2f40..6ae6cdc5fa5b 100644 > --- a/net/netfilter/nfnetlink_queue.c > +++ b/net/netfilter/nfnetlink_queue.c > @@ -470,9 +470,9 @@ static int nfqnl_put_sk_classid(struct sk_buff *skb, struct sock *sk) > return 0; > } > > -static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, struct lsm_context *ctx) > +static int nfqnl_get_sk_secctx(struct sk_buff *skb, struct lsm_context *ctx) > { > - u32 seclen = 0; > + int seclen = 0; > #if IS_ENABLED(CONFIG_NETWORK_SECMARK) > > if (!skb || !sk_fullsock(skb->sk)) > @@ -568,7 +568,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue, > const struct nfnl_ct_hook *nfnl_ct; > bool csum_verify; > struct lsm_context ctx; > - u32 seclen = 0; > + int seclen = 0; > ktime_t tstamp; > > size = nlmsg_total_size(sizeof(struct nfgenmsg)) > @@ -782,7 +782,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue, > if (nfqnl_put_sk_classid(skb, entskb->sk) < 0) > goto nla_put_failure; > > - if (seclen && nla_put(skb, NFQA_SECCTX, ctx.len, ctx.context)) > + if (seclen > 0 && nla_put(skb, NFQA_SECCTX, ctx.len, ctx.context)) > goto nla_put_failure; > > if (ct && nfnl_ct->build(skb, ct, ctinfo, NFQA_CT, NFQA_CT_INFO) < 0) In addition to the changes above, I think we should also have some protection logic when we first call nfqnl_get_sk_secctx() and then use the return value to calculate the nla's size. Untested snippet below: diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c index d2773ce9b585..3abc132e7223 100644 --- a/net/netfilter/nfnetlink_queue.c +++ b/net/netfilter/nfnetlink_queue.c @@ -643,6 +643,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue, if ((queue->flags & NFQA_CFG_F_SECCTX) && entskb->sk) { seclen = nfqnl_get_sk_secctx(entskb, &secdata); + if (seclen < 0) + return NULL; if (seclen) size += nla_total_size(seclen); } -- paul-moore.com
diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c index 5110f29b2f40..6ae6cdc5fa5b 100644 --- a/net/netfilter/nfnetlink_queue.c +++ b/net/netfilter/nfnetlink_queue.c @@ -470,9 +470,9 @@ static int nfqnl_put_sk_classid(struct sk_buff *skb, struct sock *sk) return 0; } -static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, struct lsm_context *ctx) +static int nfqnl_get_sk_secctx(struct sk_buff *skb, struct lsm_context *ctx) { - u32 seclen = 0; + int seclen = 0; #if IS_ENABLED(CONFIG_NETWORK_SECMARK) if (!skb || !sk_fullsock(skb->sk)) @@ -568,7 +568,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue, const struct nfnl_ct_hook *nfnl_ct; bool csum_verify; struct lsm_context ctx; - u32 seclen = 0; + int seclen = 0; ktime_t tstamp; size = nlmsg_total_size(sizeof(struct nfgenmsg)) @@ -782,7 +782,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue, if (nfqnl_put_sk_classid(skb, entskb->sk) < 0) goto nla_put_failure; - if (seclen && nla_put(skb, NFQA_SECCTX, ctx.len, ctx.context)) + if (seclen > 0 && nla_put(skb, NFQA_SECCTX, ctx.len, ctx.context)) goto nla_put_failure; if (ct && nfnl_ct->build(skb, ct, ctinfo, NFQA_CT, NFQA_CT_INFO) < 0) diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c index bd7094f225d1..dfda9ea61971 100644 --- a/net/netlabel/netlabel_unlabeled.c +++ b/net/netlabel/netlabel_unlabeled.c @@ -437,7 +437,7 @@ int netlbl_unlhsh_add(struct net *net, unlhsh_add_return: rcu_read_unlock(); if (audit_buf != NULL) { - if (security_secid_to_secctx(secid, &ctx) == 0) { + if (security_secid_to_secctx(secid, &ctx) >= 0) { audit_log_format(audit_buf, " sec_obj=%s", ctx.context); security_release_secctx(&ctx); } @@ -490,7 +490,7 @@ static int netlbl_unlhsh_remove_addr4(struct net *net, addr->s_addr, mask->s_addr); dev_put(dev); if (entry != NULL && - security_secid_to_secctx(entry->secid, &ctx) == 0) { + security_secid_to_secctx(entry->secid, &ctx) >= 0) { audit_log_format(audit_buf, " sec_obj=%s", ctx.context); security_release_secctx(&ctx); } @@ -548,7 +548,7 @@ static int netlbl_unlhsh_remove_addr6(struct net *net, addr, mask); dev_put(dev); if (entry != NULL && - security_secid_to_secctx(entry->secid, &ctx) == 0) { + security_secid_to_secctx(entry->secid, &ctx) >= 0) { audit_log_format(audit_buf, " sec_obj=%s", ctx.context); security_release_secctx(&ctx); }
security_secid_to_secctx() returns the size of the new context, whereas previous versions provided that via a pointer parameter. Correct the type of the value returned in nfqnl_get_sk_secctx() and the check for error in netlbl_unlhsh_add(). Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> --- net/netfilter/nfnetlink_queue.c | 8 ++++---- net/netlabel/netlabel_unlabeled.c | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-)