From: Dan Carpenter <dan.carpenter@linaro.org>
To: Lars Kellogg-Stedman <lars@oddbit.com>
Cc: Duoming Zhou <duoming@zju.edu.cn>, linux-hams@vger.kernel.org
Subject: Re: [PATCH RFC] ax25: Fix (more) netdev refcount issues
Date: Tue, 30 Apr 2024 13:08:24 +0300 [thread overview]
Message-ID: <7166e176-afbb-49e1-bd24-809a1856bf93@moroto.mountain> (raw)
In-Reply-To: <tkgm4hbsiccrlehjhbpmsigo5r2fmght72evmcdz47cynqewpb@7yevwv7wma6d>
On Mon, Apr 29, 2024 at 02:06:30PM -0400, Lars Kellogg-Stedman wrote:
> On Sun, Apr 28, 2024 at 02:01:54PM +0300, Dan Carpenter wrote:
> > We would want to have a Fixes tag to say how the bug was introduced.
> > That's the commit which introduced the underflow I think.
>
> It doesn't look like d01ffb9eee4a introduced either of the problems.
>
I really wanted this to be introduced by this patch because then the
issue would have been more simple.
> I ran a couple of git bisects between d01ffb9eee4a and c942a0cd36
> (that's the HEAD of my local repository, 6.9.0-rc5); the traces
> involving ax25_release first show up in 9fd75b66b8f ("ax25: Fix refcount
> leaks caused by ax25_cb_del()"). The "waiting for ax* to become free"
> problem first shows up in feef318c855 ("ax25: fix UAF bugs of net_device
> caused by rebinding operation"). Since feef318c855 is the older commit,
> I guess we pick that one as the target for the Fixes: tag.
Meanwhile commit feef318c855a ("ax25: fix UAF bugs of net_device caused
by rebinding operation") is a much more complicated patch...
I don't think your patch is correct. It's fixing the use after free but
it introduces leaks instead.
I don't think it makes sense to take a reference in the recv() path...
How would we balance that with a _put()? It has to be something like
increment in the open() and decrement in close(). In your patch, you
say that it's dropping the reference in ax25_release() but that pairs
with ax25_bind() and that takes the reference when it calls
ax25_addr_ax25dev(). I don't see a problem there.
1) The reference count starts as 2 in ax25_dev_device_up().
refcount_set(&ax25_dev->refcount, 1);
ax25_dev_hold(ax25_dev);
Then in ax25_dev_device_down() it drops the reference once or twice
depending on if we goto unlock_put or not. What is the logic there?
Seems suspicious.
2) The ax25_addr_ax25dev() has a small bug. It potentially increments
more than one reference. That's unrelated to the underflow. Also
The function should be renamed to show that it increments the ref
count.
diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c
index 282ec581c072..13fe57deafef 100644
--- a/net/ax25/ax25_dev.c
+++ b/net/ax25/ax25_dev.c
@@ -34,11 +34,12 @@ ax25_dev *ax25_addr_ax25dev(ax25_address *addr)
ax25_dev *ax25_dev, *res = NULL;
spin_lock_bh(&ax25_dev_lock);
- for (ax25_dev = ax25_dev_list; ax25_dev != NULL; ax25_dev = ax25_dev->next)
- if (ax25cmp(addr, (const ax25_address *)ax25_dev->dev->dev_addr) == 0) {
+ for (ax25_dev = ax25_dev_list; ax25_dev != NULL; ax25_dev = ax25_dev->next) {
+ if (ax25cmp(addr, (const ax25_address *)ax25_dev->dev->dev_addr) == 0)
res = ax25_dev;
- ax25_dev_hold(ax25_dev);
- }
+ }
+ if (res)
+ ax25_dev_hold(res);
spin_unlock_bh(&ax25_dev_lock);
return res;
3) The ax25_dev_free() function doesn't do have reference counting.
Suspicous.
The other increment/decrement is bind/release which seems okay to me.
regards,
dan carpenter
prev parent reply other threads:[~2024-04-30 10:08 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-26 21:29 [PATCH RFC] ax25: Fix (more) netdev refcount issues Lars Kellogg-Stedman
2024-04-27 8:48 ` Dan Carpenter
2024-04-27 17:15 ` Lars Kellogg-Stedman
2024-04-28 11:01 ` Dan Carpenter
2024-04-29 18:06 ` Lars Kellogg-Stedman
2024-04-30 10:08 ` Dan Carpenter [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7166e176-afbb-49e1-bd24-809a1856bf93@moroto.mountain \
--to=dan.carpenter@linaro.org \
--cc=duoming@zju.edu.cn \
--cc=lars@oddbit.com \
--cc=linux-hams@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.