| Message ID | 20250313195441.515267-1-chenyuan0y@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: Initialize ctx to avoid memory allocation error | expand |
[ 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; }
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; > }
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.
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.
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.
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;
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(-)