netdev - Re: [bug report] net: ethtool: Introduce per-PHY DUMP operations

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: <aYmID1BOGmwb2rUD@stanley.mountain>
Date: Mon, 9 Feb 2026 10:09:03 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>
Cc: Simon Horman <horms@...nel.org>, netdev@...r.kernel.org,
	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [bug report] net: ethtool: Introduce per-PHY DUMP operations

On Fri, Feb 06, 2026 at 06:04:36PM +0100, Maxime Chevallier wrote:
> > net/ethtool/netlink.c
> >     700 static int ethnl_perphy_start(struct netlink_callback *cb)
> >     701 {
> >     702         struct ethnl_perphy_dump_ctx *phy_ctx = ethnl_perphy_dump_context(cb);
> >     703         const struct genl_dumpit_info *info = genl_dumpit_info(cb);
> >     704         struct ethnl_dump_ctx *ctx = &phy_ctx->ethnl_ctx;
> >     705         struct ethnl_reply_data *reply_data;
> >     706         const struct ethnl_request_ops *ops;
> >     707         struct ethnl_req_info *req_info;
> >     708         struct genlmsghdr *ghdr;
> >     709         int ret;
> >     710 
> >     711         BUILD_BUG_ON(sizeof(*ctx) > sizeof(cb->ctx));
> >     712 
> >     713         ghdr = nlmsg_data(cb->nlh);
> > --> 714         ops = ethnl_default_requests[ghdr->cmd];
> > 
> > Smatch thinks nlmsg_data() is untrusted data, so it could be out of bounds.
> > It's a u8, but there are only 52 elements in the ethnl_default_requests[]
> > array.
> 
> I see, then we also have the same problem in ethnl_default_start().
> 
> I'd expect the genl part to validate cmd (I haven't checked yet), but we
> do have a WARN_ONCE just below for the case 'cmd' is wrong, so we could
> definitely add some more sanity checks before accessing
> ethnl_default_requests[].

The WARN_ONCE() doesn't doesn't work as bounds checking since there is
no guarantee that the array will be followed by NULL pointers.  I didn't
see a bounds check for this, but I'm not an expert.

netlink_rcv_skb() <- receives untrusted data nlh = nlmsg_hdr(skb);
-> nfnetlink_rcv_msg() <- calls nc->call()
   -> ip_set_dump()
      -> netlink_dump_start()
         -> __netlink_dump_start() <- calls control->start(cb);
            -> genl_start() <- this is where the validation would be
                               when we call
                               genl_family_rcv_msg_attrs_parse()
               -> ethnl_perphy_start()

Also the WARN_ONCE() warns if we try to do a cmd which doesn't have a
matching operation in ethnl_default_requests[].  Every time we check
for missing commands it triggers a WARN_ONCE().  There are quite a few
which don't have a handler so I'm surprised that syzbot doesn't trigger
the warning and complain.  Here is a list of commands without a
handler:

ETHTOOL_MSG_USER_NONE, 
ETHTOOL_MSG_FEATURES_SET,
ETHTOOL_MSG_CABLE_TEST_ACT,
ETHTOOL_MSG_CABLE_TEST_TDR_ACT,
ETHTOOL_MSG_TUNNEL_INFO_GET,
ETHTOOL_MSG_MODULE_FW_FLASH_ACT,
ETHTOOL_MSG_RSS_CREATE_ACT,
ETHTOOL_MSG_RSS_DELETE_ACT,

regards,
dan carpenter