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

[Casey Schaufler]

On 10/24/2024 9:10 AM, Pablo Neira Ayuso wrote:
> Hi Casey,
>
> This is a review of the netfilter chunk.

Thank you.

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

I'll fix that.

>> +	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.

Will do.

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

Will do.

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