netdev - Re: [PATCH v4] ax25: Fix refcount imbalance on inbound connections

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <61368681-64b5-43f7-9a6d-5e56b188a826@moroto.mountain>
Date: Thu, 23 May 2024 21:23:08 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Dan Cross <crossd@...il.com>
Cc: lars@...bit.com, Duoming Zhou <duoming@....edu.cn>,
	linux-hams@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH v4] ax25: Fix refcount imbalance on inbound connections

On Thu, May 23, 2024 at 11:22:43AM -0400, Dan Cross wrote:
> On Thu, May 23, 2024 at 11:05 AM Dan Carpenter <dan.carpenter@...aro.org> wrote:
> > > [snip]
> >
> > I've already said that I don't think the patch is correct and offered
> > an alternative which takes a reference in accept() but also adds a
> > matching put()...  But I can't really test my patch so if we're going to
> > do something that we know is wrong, I'd prefer to just revert Duoming's
> > patch.
> 
> Dan, may I ask how you determined that Lars's patch is incorrect?

The problem is that accept() and ax25_release() are not mirrored pairs.
We're just taking the reference and never dropping it.  Which fixes the
use after free but introduces a leak.

> Testing so far indicates that it works as expected. On the other hand,
> Lars tested your patch and found that it did not address the
> underlying issue
> (https://marc.info/?l=linux-hams&m=171646940902757&w=2).

Yeah.  I've said a couple times that my patch wasn't complete.  I keep
hoping that Duoming will chime in here...

> 
> If I may suggest a path forward, given that observed results show that
> Lars's patch works as expected, perhaps we can commit that and then
> work to incorporate a more robust ref counting strategy a la your
> patch?

The argument for this patch is that it works in testing even though we
think it's not totally correct.  That's not really a good argument.
Like we can revert patches that clearly don't work so we could revert
Duoming's patch, but when we're adding code then that should work.

regards,
dan carpenter