Hi Casey,
This is a review of the netfilter chunk.
On Wed, Oct 23, 2024 at 02:21:55PM -0700, Casey Schaufler wrote:
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index 86a57a3afdd6..dd74d4c67c69 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -360,8 +360,8 @@ static int ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
> struct lsm_context ctx;
> int ret;
>
> - ret = security_secid_to_secctx(ct->secmark, &ctx.context, &ctx.len);
> - if (ret)
> + ret = security_secid_to_secctx(ct->secmark, &ctx);
> + if (ret < 0)
> return 0;
>
> ret = -1;
> @@ -665,8 +665,8 @@ static inline int ctnetlink_secctx_size(const struct nf_conn *ct)
> #ifdef CONFIG_NF_CONNTRACK_SECMARK
> int len, ret;
>
> - ret = security_secid_to_secctx(ct->secmark, NULL, &len);
> - if (ret)
> + ret = security_secid_to_secctx(ct->secmark, NULL);
This breaks here.
len is really used, this should be instead:
ret = security_secid_to_secctx(ct->secmark, &ctx);
[...]
return nla_total_size(0) /* CTA_SECCTX */
+ nla_total_size(sizeof(char) * ctx.len); /* CTA_SECCTX_NAME */
#else
return 0;
#endif
}
> + if (ret < 0)
> return 0;
>
> return nla_total_size(0) /* CTA_SECCTX */
> diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
> index 5f7fd23b7afe..502cf10aab41 100644
> --- a/net/netfilter/nf_conntrack_standalone.c
> +++ b/net/netfilter/nf_conntrack_standalone.c
> @@ -175,8 +175,8 @@ static void ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
> struct lsm_context ctx;
> int ret;
>
> - ret = security_secid_to_secctx(ct->secmark, &ctx.context, &ctx.len);
> - if (ret)
> + ret = security_secid_to_secctx(ct->secmark, &ctx);
> + if (ret < 0)
> return;
>
> seq_printf(s, "secctx=%s ", ctx.context);
> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> index 37757cd77cf1..5110f29b2f40 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -470,18 +470,18 @@ 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, char **secdata)
> +static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, struct lsm_context *ctx)
> {
> u32 seclen = 0;
> #if IS_ENABLED(CONFIG_NETWORK_SECMARK)
> +
remove unneeded line.
> if (!skb || !sk_fullsock(skb->sk))
> return 0;
>
> read_lock_bh(&skb->sk->sk_callback_lock);
>
> if (skb->secmark)
> - security_secid_to_secctx(skb->secmark, secdata, &seclen);
> -
> + seclen = security_secid_to_secctx(skb->secmark, ctx);
> read_unlock_bh(&skb->sk->sk_callback_lock);
> #endif
> return seclen;
> @@ -567,8 +567,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> enum ip_conntrack_info ctinfo = 0;
> const struct nfnl_ct_hook *nfnl_ct;
> bool csum_verify;
> - struct lsm_context scaff; /* scaffolding */
> - char *secdata = NULL;
> + struct lsm_context ctx;
Help us make this get closer to revert xmas tree:
enum ip_conntrack_info ctinfo = 0;
const struct nfnl_ct_hook *nfnl_ct;
+ struct lsm_context ctx;
bool csum_verify;
- struct lsm_context scaff; /* scaffolding */
- char *secdata = NULL;
> bool csum_verify;
> - struct lsm_context scaff; /* scaffolding */
> - char *secdata = NULL;
> u32 seclen = 0;
> ktime_t tstamp;
>
> @@ -643,8 +642,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)
> + seclen = nfqnl_get_sk_secctx(entskb, &ctx);
> + if (seclen >= 0)
> size += nla_total_size(seclen);
> }
>
> @@ -783,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, seclen, secdata))
> + if (seclen && 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)
> @@ -811,10 +810,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> }
>
> nlh->nlmsg_len = skb->len;
> - if (seclen) {
> - lsmcontext_init(&scaff, secdata, seclen, 0);
> - security_release_secctx(&scaff);
> - }
> + if (seclen >= 0)
> + security_release_secctx(&ctx);
> return skb;
>
> nla_put_failure:
> @@ -822,10 +819,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> kfree_skb(skb);
> net_err_ratelimited("nf_queue: error creating packet message\n");
> nlmsg_failure:
> - if (seclen) {
> - lsmcontext_init(&scaff, secdata, seclen, 0);
> - security_release_secctx(&scaff);
> - }
> + if (seclen >= 0)
> + security_release_secctx(&ctx);
> return NULL;
> }
>
