netdev - Re: [PATCH net-next] net: ethtool: phy: Distinguish whether dev is got by phy start or doit

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: <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
> 
>