| Message ID | 20240913080714.1809254-1-lizhi.xu@windriver.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: ethtool: phy: Distinguish whether dev is got by phy start or doit | expand |
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 > >
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
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 }
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);
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(-)