[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240913114429.GY572255@kernel.org>
Date: Fri, 13 Sep 2024 12:44:29 +0100
From: Simon Horman <horms@...nel.org>
To: Lizhi Xu <lizhi.xu@...driver.com>
Cc: syzbot+e9ed4e4368d450c8f9db@...kaller.appspotmail.com,
christophe.leroy@...roup.eu, davem@...emloft.net,
edumazet@...gle.com, kuba@...nel.org, linux-kernel@...r.kernel.org,
maxime.chevallier@...tlin.com, netdev@...r.kernel.org,
pabeni@...hat.com, syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH net-next] net: ethtool: phy: Distinguish whether dev is
got by phy start or doit
On Fri, Sep 13, 2024 at 04:07:13PM +0800, Lizhi Xu wrote:
> Syzbot reported a refcount bug in ethnl_phy_done.
> This is because when executing ethnl_phy_done, it does not know who obtained
> the dev(it can be got by ethnl_phy_doit or ethnl_phy_start) and directly
> executes ethnl_parse_header_dev_put as long as the dev is not NULL.
> Add dev_start_doit to the structure phy_req_info to distinguish who obtains dev.
>
> Fixes: 17194be4c8e1 ("net: ethtool: Introduce a command to list PHYs on an interface")
> Reported-and-tested-by: syzbot+e9ed4e4368d450c8f9db@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=e9ed4e4368d450c8f9db
> Signed-off-by: Lizhi Xu <lizhi.xu@...driver.com>
It seems that Maxime has also posted a patch for this problem.
- [PATCH net-next] net: ethtool: phy: Don't set the context dev pointer for unfiltered DUMP
https://lore.kernel.org/all/20240913100515.167341-1-maxime.chevallier@bootlin.com/
> ---
> net/ethtool/phy.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/net/ethtool/phy.c b/net/ethtool/phy.c
> index 4ef7c6e32d10..321a7f89803f 100644
> --- a/net/ethtool/phy.c
> +++ b/net/ethtool/phy.c
> @@ -13,6 +13,7 @@
> struct phy_req_info {
> struct ethnl_req_info base;
> struct phy_device_node *pdn;
> + u8 dev_start_doit;
I think bool might be a more suitable type for this field.
> };
>
> #define PHY_REQINFO(__req_base) \
> @@ -157,6 +158,9 @@ int ethnl_phy_doit(struct sk_buff *skb, struct genl_info *info)
> if (ret < 0)
> return ret;
>
> + if (req_info.base.dev)
> + req_info.dev_start_doit = 0;
> +
> rtnl_lock();
>
> ret = ethnl_phy_parse_request(&req_info.base, tb, info->extack);
> @@ -223,10 +227,14 @@ int ethnl_phy_start(struct netlink_callback *cb)
> false);
> ctx->ifindex = 0;
> ctx->phy_index = 0;
> + ctx->phy_req_info->dev_start_doit = 0;
>
> if (ret)
> kfree(ctx->phy_req_info);
>
> + if (ctx->phy_req_info->base.dev)
> + ctx->phy_req_info->dev_start_doit = 1;
This doesn't seem right, ctx->phy_req_info may have been freed above.
> +
> return ret;
> }
>
> @@ -234,7 +242,7 @@ int ethnl_phy_done(struct netlink_callback *cb)
> {
> struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
>
> - if (ctx->phy_req_info->base.dev)
> + if (ctx->phy_req_info->base.dev && ctx->phy_req_info->dev_start_doit)
> ethnl_parse_header_dev_put(&ctx->phy_req_info->base);
>
> kfree(ctx->phy_req_info);
> --
> 2.43.0
>
>
Powered by blists - more mailing lists