netdev - Re: [PATCH v3 04/11] net: qrtr: support identical node ids

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <aIz4pj5qgXSNg8mt@stanley.mountain>
Date: Fri, 1 Aug 2025 20:25:58 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Simon Horman <horms@...nel.org>
Cc: Mihai Moldovan <ionic@...ic.de>, linux-arm-msm@...r.kernel.org,
	Manivannan Sadhasivam <mani@...nel.org>,
	Denis Kenzior <denkenz@...il.com>,
	Eric Dumazet <edumazet@...gle.com>,
	Kuniyuki Iwashima <kuniyu@...gle.com>,
	Paolo Abeni <pabeni@...hat.com>,
	Willem de Bruijn <willemb@...gle.com>,
	"David S . Miller" <davem@...emloft.net>,
	Jakub Kicinski <kuba@...nel.org>, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org
Subject: Re: [PATCH v3 04/11] net: qrtr: support identical node ids

On Sun, Jul 27, 2025 at 03:40:14PM +0100, Simon Horman wrote:
> + Dan Carpenter
> 
> On Sun, Jul 27, 2025 at 03:09:38PM +0200, Mihai Moldovan wrote:
> > * On 7/24/25 15:08, Simon Horman wrote:
> > > [...]
> > 
> > Thank you for the reviews, to both you and Jakub.
> > 
> > 
> > > This will leak holding qrtr_nodes_lock.
> > 
> > It certainly does, will be fixed in v4.
> > 
> > 
> > > Flagged by Smatch.
> > 
> > I haven't used smatch before, and probably should do so going forward.
> > 
> > Curiously, a simple kchecker net/qrtr/ run did not warn about the locking
> > issue (albeit it being obvious in the patch), while it did warn about the
> > second issue with ret. Am I missing something?
> 
> TL;DR: No, I seem to have been able to reproduce what you see.
> 
> I ran Smatch, compiled from a recent Git commit, like this:
> 
> kchecker net/qrtr/af_qrtr.o
> 
> The warnings I saw (new to this patch) are:
> 
> net/qrtr/af_qrtr.c:498 qrtr_node_assign() warn: inconsistent returns 'global &qrtr_nodes_lock'.
>   Locked on  : 484
>   Unlocked on: 498
> net/qrtr/af_qrtr.c:613 qrtr_endpoint_post() warn: missing error code 'ret'
> 
> That was with Smatch compiled from Git [1]
> commit e1d933013098 ("return_efault: don't rely on the cross function DB")
> 
> I tried again with the latest head,
> commit 2fb2b9093c5d ("sleep_info: The synchronize_srcu() sleeps").
> And in that case I no longer see the 1st warning, about locking.
> I think this is what you saw too.
> 
> This seems to a regression in Smatch wrt this particular case for this
> code. I bisected Smatch and it looks like it was introduced in commit
> d0367cd8a993 ("ranges: use absolute instead implied for possibly_true/false")
> 
> I CCed Dan in case he wants to dig into this.

The code looks like this:

	spin_lock_irqsave(&qrtr_nodes_lock, flags);

        if (node->ep->id > QRTR_INDEX_HALF_UNSIGNED_MAX ||
            nid > QRTR_INDEX_HALF_UNSIGNED_MAX)
                return -EINVAL;

The problem is that QRTR_INDEX_HALF_UNSIGNED_MAX is U32_MAX and
node->ep->id and nid are both u32 type.  The return statement is dead
code and I deliberately silenced warnings on impossible paths.

The following patch will enable the warning again and I'll test it tonight
to see what happens.  If it's not too painful then I'll delete it
properly, but if it's generates a bunch of false positives then, in the
end, I'm not overly stressed about bugs in dead code.

regards,
dan carpenter


View attachment "diff" of type "text/plain" (897 bytes)