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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <7fb4dd7f-6e89-4587-98d6-fc1a99bbeb88@schaufler-ca.com>
Date: Tue, 10 Dec 2024 08:24:56 -0800
From: Casey Schaufler <casey@...aufler-ca.com>
To: Florian Westphal <fw@...len.de>, Karol Przybylski <karprzy7@...il.com>
Cc: netfilter-devel@...r.kernel.org, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, skhan@...uxfoundation.org,
 Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH] netfilter: nfnetlink_queue: Fix redundant comparison of
 unsigned value

On 12/9/2024 2:20 PM, Florian Westphal wrote:
> Karol Przybylski <karprzy7@...il.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@...il.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.