linux-kernel - Re: [PATCH] net: Initialize ctx to avoid memory allocation error

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <42e5bb33-1826-43df-940d-ec80774fc65b@schaufler-ca.com>
Date: Fri, 14 Mar 2025 09:41:24 -0700
From: Casey Schaufler <casey@...aufler-ca.com>
To: Florian Westphal <fw@...len.de>, Chenyuan Yang <chenyuan0y@...il.com>
Cc: netfilter-devel@...r.kernel.org, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH] net: Initialize ctx to avoid memory allocation error

On 3/13/2025 1:10 PM, Florian Westphal wrote:
> [ trim CCs, CC Casey ]
>
> Chenyuan Yang <chenyuan0y@...il.com> wrote:
>> It is possible that ctx in nfqnl_build_packet_message() could be used
>> before it is properly initialize, which is only initialized
>> by nfqnl_get_sk_secctx().
>>
>> This patch corrects this problem by initializing the lsmctx to a safe
>> value when it is declared.
>>
>> This is similar to the commit 35fcac7a7c25
>> ("audit: Initialize lsmctx to avoid memory allocation error").
> Fixes: 2d470c778120 ("lsm: replace context+len with lsm_context")
>
>> Signed-off-by: Chenyuan Yang <chenyuan0y@...il.com>
>> ---
>>  net/netfilter/nfnetlink_queue.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
>> index 5c913987901a..8b7b39d8a109 100644
>> --- a/net/netfilter/nfnetlink_queue.c
>> +++ b/net/netfilter/nfnetlink_queue.c
>> @@ -567,7 +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 ctx;
>> +	struct lsm_context ctx = { NULL, 0, 0 };
>>  	int seclen = 0;
>>  	ktime_t tstamp;
> Someone that understands LSM should clarify what seclen == 0 means.

If seclen is 0 it implies that there is no security context and that
the secctx is NULL. How that is handled in the release function is up
to the LSM. SELinux allocates secctx data, while Smack points to an
entry in a persistent table.

> seclen needs to be > 0 or no secinfo is passed to userland,
> yet the secctx release function is called anyway.

That is correct. The security module is responsible for handling
the release of secctx correctly.

> Should seclen be initialised to -1?  Or we need the change below too?

No. The security modules handle secctx their own way.

>
> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -812,7 +812,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>         }
>
>         nlh->nlmsg_len = skb->len;
> -       if (seclen >= 0)
> +       if (seclen > 0)
>                 security_release_secctx(&ctx);
>         return skb;
>
> @@ -821,7 +821,7 @@ 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 >= 0)
> +       if (seclen > 0)
>                 security_release_secctx(&ctx);
>         return NULL;
>  }