[PATCH] net: Initialize ctx to avoid memory allocation error

[PATCH] net: Initialize ctx to avoid memory allocation error

[Chenyuan Yang]

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").

Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.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;
 
-- 
2.34.1

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

[Florian Westphal]

[ trim CCs, CC Casey ]

Chenyuan Yang <chenyuan0y@gmail.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@gmail.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.

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

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

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

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

[Casey Schaufler]

On 3/13/2025 1:10 PM, Florian Westphal wrote:
> [ trim CCs, CC Casey ]
>
> Chenyuan Yang <chenyuan0y@gmail.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@gmail.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;
>  }

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

[Florian Westphal]

Casey Schaufler <casey@schaufler-ca.com> wrote:
> 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.

Well, as-is security_release_secctx() can be called with garbage ctx;
seclen is inited to 0, but ctx is not initialized unconditionally.

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

[Casey Schaufler]

On 3/14/2025 9:47 AM, Florian Westphal wrote:
> Casey Schaufler <casey@schaufler-ca.com> wrote:
>> 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.
> Well, as-is security_release_secctx() can be called with garbage ctx;
> seclen is inited to 0, but ctx is not initialized unconditionally.

Which isn't an issue for any existing security module.

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

[Florian Westphal]

Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>> 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.
> > Well, as-is security_release_secctx() can be called with garbage ctx;
> > seclen is inited to 0, but ctx is not initialized unconditionally.
> 
> Which isn't an issue for any existing security module.

The splat quoted in
35fcac7a7c25 ("audit: Initialize lsmctx to avoid memory allocation error")

seems to disagree.  I see no difference to what nfnetlink_queue is
doing.

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

[Casey Schaufler]

On 3/14/2025 1:30 PM, Florian Westphal wrote:
> Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>> 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.
>>> Well, as-is security_release_secctx() can be called with garbage ctx;
>>> seclen is inited to 0, but ctx is not initialized unconditionally.
>> Which isn't an issue for any existing security module.
> The splat quoted in
> 35fcac7a7c25 ("audit: Initialize lsmctx to avoid memory allocation error")
>
> seems to disagree.  I see no difference to what nfnetlink_queue is
> doing.

Point. I see no harm in initializing the lsmctx = { } or seclen = 0;