netfilter: nfnetlink_queue: Fix redundant comparison of unsigned value

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

Posted by Karol Przybylski 2 weeks, 2 days ago

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

Posted by Florian Westphal 2 weeks, 2 days ago

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

Posted by Casey Schaufler 2 weeks, 2 days ago

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.

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

Posted by Pablo Neira Ayuso 2 weeks, 2 days ago

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

Posted by Karol P 2 weeks, 2 days ago

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

Posted by Pablo Neira Ayuso 2 weeks, 2 days ago

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