[PATCH] net: corrections for security_secid_to_secctx returns

[Paul Moore]

On Dec 10, 2024 Casey Schaufler <casey at 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 at 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