[PATCH] netfilter: nfnetlink_queue: Fix redundant comparison of unsigned value

[PATCH] netfilter: nfnetlink_queue: Fix redundant comparison of unsigned value

[Karol Przybylski]

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

Re: [PATCH] netfilter: nfnetlink_queue: Fix redundant comparison of unsigned value

[Pablo Neira Ayuso]

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
> 

Re: [PATCH] netfilter: nfnetlink_queue: Fix redundant comparison of unsigned value

[Karol P]

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

Re: [PATCH] netfilter: nfnetlink_queue: Fix redundant comparison of unsigned value

[Pablo Neira Ayuso]

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

Re: [PATCH] netfilter: nfnetlink_queue: Fix redundant comparison of unsigned value

[Florian Westphal]

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.

Re: [PATCH] netfilter: nfnetlink_queue: Fix redundant comparison of unsigned value

[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.