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