* [PATCH] netfilter: nfnetlink_queue: Fix redundant comparison of unsigned value
@ 2024-12-09 20:49 Karol Przybylski
2024-12-09 21:32 ` Pablo Neira Ayuso
2024-12-09 22:20 ` Florian Westphal
0 siblings, 2 replies; 6+ messages in thread
From: Karol Przybylski @ 2024-12-09 20:49 UTC (permalink / raw)
To: karprzy7, pablo, kadlec, davem, edumazet, kuba, pabeni, horms
Cc: netfilter-devel, coreteam, netdev, linux-kernel, skhan
The comparison seclen >= 0 in net/netfilter/nfnetlink_queue.c is redundant because seclen is an unsigned value, and such comparisons are always true.
This patch removes the unnecessary comparison replacing it with just 'greater than'
Discovered in coverity, CID 1602243
Signed-off-by: Karol Przybylski <karprzy7@gmail.com>
---
net/netfilter/nfnetlink_queue.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index 5110f29b2..eacb34ffb 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -643,7 +643,7 @@ 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, &ctx);
- if (seclen >= 0)
+ if (seclen > 0)
size += nla_total_size(seclen);
}
@@ -810,7 +810,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;
@@ -819,7 +819,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;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] netfilter: nfnetlink_queue: Fix redundant comparison of unsigned value
2024-12-09 20:49 [PATCH] netfilter: nfnetlink_queue: Fix redundant comparison of unsigned value Karol Przybylski
@ 2024-12-09 21:32 ` Pablo Neira Ayuso
2024-12-09 21:50 ` Karol P
2024-12-09 22:20 ` Florian Westphal
1 sibling, 1 reply; 6+ messages in thread
From: Pablo Neira Ayuso @ 2024-12-09 21:32 UTC (permalink / raw)
To: Karol Przybylski
Cc: kadlec, davem, edumazet, kuba, pabeni, horms, netfilter-devel,
coreteam, netdev, linux-kernel, skhan
Hi,
On Mon, Dec 09, 2024 at 09:49:18PM +0100, Karol Przybylski wrote:
> The comparison seclen >= 0 in net/netfilter/nfnetlink_queue.c is redundant because seclen is an unsigned value, and such comparisons are always true.
>
> This patch removes the unnecessary comparison replacing it with just 'greater than'
>
> Discovered in coverity, CID 1602243
>
> Signed-off-by: Karol Przybylski <karprzy7@gmail.com>
> ---
> net/netfilter/nfnetlink_queue.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> index 5110f29b2..eacb34ffb 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -643,7 +643,7 @@ 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, &ctx);
> - if (seclen >= 0)
> + if (seclen > 0)
What tree are you using? I don't see this code in net-next.git
> size += nla_total_size(seclen);
> }
>
> @@ -810,7 +810,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;
>
> @@ -819,7 +819,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;
> }
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] netfilter: nfnetlink_queue: Fix redundant comparison of unsigned value
2024-12-09 21:32 ` Pablo Neira Ayuso
@ 2024-12-09 21:50 ` Karol P
2024-12-09 21:57 ` Pablo Neira Ayuso
0 siblings, 1 reply; 6+ messages in thread
From: Karol P @ 2024-12-09 21:50 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: kadlec, davem, edumazet, kuba, pabeni, horms, netfilter-devel,
coreteam, netdev, linux-kernel, skhan
On Mon, 9 Dec 2024 at 22:32, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> Hi,
>
> On Mon, Dec 09, 2024 at 09:49:18PM +0100, Karol Przybylski wrote:
> > The comparison seclen >= 0 in net/netfilter/nfnetlink_queue.c is redundant because seclen is an unsigned value, and such comparisons are always true.
> >
> > This patch removes the unnecessary comparison replacing it with just 'greater than'
> >
> > Discovered in coverity, CID 1602243
> >
> > Signed-off-by: Karol Przybylski <karprzy7@gmail.com>
> > ---
> > net/netfilter/nfnetlink_queue.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> > index 5110f29b2..eacb34ffb 100644
> > --- a/net/netfilter/nfnetlink_queue.c
> > +++ b/net/netfilter/nfnetlink_queue.c
> > @@ -643,7 +643,7 @@ 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, &ctx);
> > - if (seclen >= 0)
> > + if (seclen > 0)
>
> What tree are you using? I don't see this code in net-next.git
linux-next, next-20241209
Link: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/net/netfilter/nfnetlink_queue.c?h=next-20241209#n646
>
> > size += nla_total_size(seclen);
> > }
> >
> > @@ -810,7 +810,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;
> >
> > @@ -819,7 +819,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;
> > }
> > --
> > 2.34.1
> >
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] netfilter: nfnetlink_queue: Fix redundant comparison of unsigned value
2024-12-09 21:50 ` Karol P
@ 2024-12-09 21:57 ` Pablo Neira Ayuso
0 siblings, 0 replies; 6+ messages in thread
From: Pablo Neira Ayuso @ 2024-12-09 21:57 UTC (permalink / raw)
To: Karol P
Cc: kadlec, davem, edumazet, kuba, pabeni, horms, netfilter-devel,
coreteam, netdev, linux-kernel, skhan
On Mon, Dec 09, 2024 at 10:50:17PM +0100, Karol P wrote:
> On Mon, 9 Dec 2024 at 22:32, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >
> > Hi,
> >
> > On Mon, Dec 09, 2024 at 09:49:18PM +0100, Karol Przybylski wrote:
> > > The comparison seclen >= 0 in net/netfilter/nfnetlink_queue.c is redundant because seclen is an unsigned value, and such comparisons are always true.
> > >
> > > This patch removes the unnecessary comparison replacing it with just 'greater than'
> > >
> > > Discovered in coverity, CID 1602243
> > >
> > > Signed-off-by: Karol Przybylski <karprzy7@gmail.com>
> > > ---
> > > net/netfilter/nfnetlink_queue.c | 6 +++---
> > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> > > index 5110f29b2..eacb34ffb 100644
> > > --- a/net/netfilter/nfnetlink_queue.c
> > > +++ b/net/netfilter/nfnetlink_queue.c
> > > @@ -643,7 +643,7 @@ 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, &ctx);
> > > - if (seclen >= 0)
> > > + if (seclen > 0)
> >
> > What tree are you using? I don't see this code in net-next.git
>
> linux-next, next-20241209
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/net/netfilter/nfnetlink_queue.c?h=next-20241209#n646
Could you trace from what commit ID and what tree this is coming?
Then, post the patch to the corresponding tree and add a Fixes: tag?
Possibly use:
if (seclen)
as this code was before?
Thanks.
> > > size += nla_total_size(seclen);
> > > }
> > >
> > > @@ -810,7 +810,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;
> > >
> > > @@ -819,7 +819,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;
> > > }
> > > --
> > > 2.34.1
> > >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: nfnetlink_queue: Fix redundant comparison of unsigned value
2024-12-09 20:49 [PATCH] netfilter: nfnetlink_queue: Fix redundant comparison of unsigned value Karol Przybylski
2024-12-09 21:32 ` Pablo Neira Ayuso
@ 2024-12-09 22:20 ` Florian Westphal
2024-12-10 16:24 ` Casey Schaufler
1 sibling, 1 reply; 6+ messages in thread
From: Florian Westphal @ 2024-12-09 22:20 UTC (permalink / raw)
To: Karol Przybylski; +Cc: netfilter-devel, netdev, linux-kernel, skhan, casey
Karol Przybylski <karprzy7@gmail.com> wrote:
[ CC original patch author and mass-trimming CCs ]
> The comparison seclen >= 0 in net/netfilter/nfnetlink_queue.c is redundant because seclen is an unsigned value, and such comparisons are always true.
>
> This patch removes the unnecessary comparison replacing it with just 'greater than'
>
> Discovered in coverity, CID 1602243
>
> Signed-off-by: Karol Przybylski <karprzy7@gmail.com>
> ---
> net/netfilter/nfnetlink_queue.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> index 5110f29b2..eacb34ffb 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -643,7 +643,7 @@ 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, &ctx);
> - if (seclen >= 0)
> + if (seclen > 0)
> size += nla_total_size(seclen);
Casey, can you please have a look?
AFAICS security_secid_to_secctx() could return -EFOO, so it seems
nfqnl_get_sk_secctx has a bug and should conceal < 0 retvals
(the function returns u32), in addition to the always-true >= check
fixup.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] netfilter: nfnetlink_queue: Fix redundant comparison of unsigned value
2024-12-09 22:20 ` Florian Westphal
@ 2024-12-10 16:24 ` Casey Schaufler
0 siblings, 0 replies; 6+ messages in thread
From: Casey Schaufler @ 2024-12-10 16:24 UTC (permalink / raw)
To: Florian Westphal, Karol Przybylski
Cc: netfilter-devel, netdev, linux-kernel, skhan, Casey Schaufler
On 12/9/2024 2:20 PM, Florian Westphal wrote:
> Karol Przybylski <karprzy7@gmail.com> wrote:
>
> [ CC original patch author and mass-trimming CCs ]
>
>> The comparison seclen >= 0 in net/netfilter/nfnetlink_queue.c is redundant because seclen is an unsigned value, and such comparisons are always true.
>>
>> This patch removes the unnecessary comparison replacing it with just 'greater than'
>>
>> Discovered in coverity, CID 1602243
>>
>> Signed-off-by: Karol Przybylski <karprzy7@gmail.com>
>> ---
>> net/netfilter/nfnetlink_queue.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
>> index 5110f29b2..eacb34ffb 100644
>> --- a/net/netfilter/nfnetlink_queue.c
>> +++ b/net/netfilter/nfnetlink_queue.c
>> @@ -643,7 +643,7 @@ 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, &ctx);
>> - if (seclen >= 0)
>> + if (seclen > 0)
>> size += nla_total_size(seclen);
> Casey, can you please have a look?
Yes, there is indeed an issue here. I will look into the correct change today.
Thank you.
>
> AFAICS security_secid_to_secctx() could return -EFOO, so it seems
> nfqnl_get_sk_secctx has a bug and should conceal < 0 retvals
> (the function returns u32), in addition to the always-true >= check
> fixup.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-12-10 16:25 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-09 20:49 [PATCH] netfilter: nfnetlink_queue: Fix redundant comparison of unsigned value Karol Przybylski
2024-12-09 21:32 ` Pablo Neira Ayuso
2024-12-09 21:50 ` Karol P
2024-12-09 21:57 ` Pablo Neira Ayuso
2024-12-09 22:20 ` Florian Westphal
2024-12-10 16:24 ` Casey Schaufler
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).