Re: [PATCH v3 2/5] LSM: Replace context+len with lsm_context

[Pablo Neira Ayuso <pablo@netfilter.org>]

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;
>  }
>