1. Patchew
  2. linux
  3. View series

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

Lizhi Xu posted 1 patch 2 months, 2 weeks ago 
net/ethtool/phy.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
[PATCH net-next] net: ethtool: phy: Distinguish whether dev is got by phy start or doit
Posted by Lizhi Xu 2 months, 2 weeks ago 
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@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=e9ed4e4368d450c8f9db
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.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;
 };
 
 #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;
+
 	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
Re: [PATCH net-next] net: ethtool: phy: Distinguish whether dev is got by phy start or doit
Posted by Dan Carpenter 2 months, 2 weeks ago 
Hi Lizhi,

kernel test robot noticed the following build warnings:

url:    https://github.com/intel-lab-lkp/linux/commits/Lizhi-Xu/net-ethtool-phy-Distinguish-whether-dev-is-got-by-phy-start-or-doit/20240913-160835
base:   net-next/main
patch link:    https://lore.kernel.org/r/20240913080714.1809254-1-lizhi.xu%40windriver.com
patch subject: [PATCH net-next] net: ethtool: phy: Distinguish whether dev is got by phy start or doit
config: x86_64-randconfig-r072-20240914 (https://download.01.org/0day-ci/archive/20240916/202409161017.tjjHpXGT-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202409161017.tjjHpXGT-lkp@intel.com/

smatch warnings:
net/ethtool/phy.c:235 ethnl_phy_start() error: dereferencing freed memory 'ctx->phy_req_info'

vim +235 net/ethtool/phy.c

17194be4c8e1e8 Maxime Chevallier 2024-08-21  212  int ethnl_phy_start(struct netlink_callback *cb)
17194be4c8e1e8 Maxime Chevallier 2024-08-21  213  {
17194be4c8e1e8 Maxime Chevallier 2024-08-21  214  	const struct genl_info *info = genl_info_dump(cb);
17194be4c8e1e8 Maxime Chevallier 2024-08-21  215  	struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
17194be4c8e1e8 Maxime Chevallier 2024-08-21  216  	int ret;
17194be4c8e1e8 Maxime Chevallier 2024-08-21  217  
17194be4c8e1e8 Maxime Chevallier 2024-08-21  218  	BUILD_BUG_ON(sizeof(*ctx) > sizeof(cb->ctx));
17194be4c8e1e8 Maxime Chevallier 2024-08-21  219  
17194be4c8e1e8 Maxime Chevallier 2024-08-21  220  	ctx->phy_req_info = kzalloc(sizeof(*ctx->phy_req_info), GFP_KERNEL);
17194be4c8e1e8 Maxime Chevallier 2024-08-21  221  	if (!ctx->phy_req_info)
17194be4c8e1e8 Maxime Chevallier 2024-08-21  222  		return -ENOMEM;
17194be4c8e1e8 Maxime Chevallier 2024-08-21  223  
17194be4c8e1e8 Maxime Chevallier 2024-08-21  224  	ret = ethnl_parse_header_dev_get(&ctx->phy_req_info->base,
17194be4c8e1e8 Maxime Chevallier 2024-08-21  225  					 info->attrs[ETHTOOL_A_PHY_HEADER],
17194be4c8e1e8 Maxime Chevallier 2024-08-21  226  					 sock_net(cb->skb->sk), cb->extack,
17194be4c8e1e8 Maxime Chevallier 2024-08-21  227  					 false);
17194be4c8e1e8 Maxime Chevallier 2024-08-21  228  	ctx->ifindex = 0;
17194be4c8e1e8 Maxime Chevallier 2024-08-21  229  	ctx->phy_index = 0;
355b18bd0d5516 Lizhi Xu          2024-09-13  230  	ctx->phy_req_info->dev_start_doit = 0;
17194be4c8e1e8 Maxime Chevallier 2024-08-21  231  
17194be4c8e1e8 Maxime Chevallier 2024-08-21  232  	if (ret)
17194be4c8e1e8 Maxime Chevallier 2024-08-21  233  		kfree(ctx->phy_req_info);
                                                                      ^^^^^^^^^^^^^^^^^
Freed

17194be4c8e1e8 Maxime Chevallier 2024-08-21  234  
355b18bd0d5516 Lizhi Xu          2024-09-13 @235  	if (ctx->phy_req_info->base.dev)
                                                            ^^^^^^^^^^^^^^^^^
Use after free

355b18bd0d5516 Lizhi Xu          2024-09-13  236  		ctx->phy_req_info->dev_start_doit = 1;
355b18bd0d5516 Lizhi Xu          2024-09-13  237  
17194be4c8e1e8 Maxime Chevallier 2024-08-21  238  	return ret;
17194be4c8e1e8 Maxime Chevallier 2024-08-21  239  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH net-next] net: ethtool: phy: Distinguish whether dev is got by phy start or doit
Posted by Maxime Chevallier 2 months, 2 weeks ago 
Hi,

On Fri, 13 Sep 2024 16:07:13 +0800
Lizhi Xu <lizhi.xu@windriver.com> 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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=e9ed4e4368d450c8f9db
> Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>

Thanks for addressing this, however I've already sent a first fix for
this [1] yesterday, followed-up by a second one [2] with another
approach following the reviews.

[1] : https://lore.kernel.org/netdev/20240913091404.3d4a9d19@fedora.home/T/#m4777416dbe26bf97b3a0a323fc71a93b40e0f7fb
[2] : https://lore.kernel.org/netdev/20240913100515.167341-1-maxime.chevallier@bootlin.com/T/#u

Best regards,

Maxime
Re: [PATCH net-next] net: ethtool: phy: Distinguish whether dev is got by phy start or doit
Posted by Simon Horman 2 months, 2 weeks ago 
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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=e9ed4e4368d450c8f9db
> Signed-off-by: Lizhi Xu <lizhi.xu@windriver.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
> 
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.