Message-ID: <e5f1688f-cb13-4d43-bf20-644cda6ab494@bootlin.com>
Date: Mon, 9 Feb 2026 09:09:34 +0100
From: Maxime Chevallier <maxime.chevallier@...tlin.com>
To: Dan Carpenter <dan.carpenter@...aro.org>
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

Hi Dan,

On 09/02/2026 08:09, Dan Carpenter wrote:
> 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,

While these commands don't have ethnl_request_ops handlers, they still
have a genetlink handler, see the ethtool_genl_ops array [1]

The ethnl_request_ops are there to provide a framework for ethtool
netlink commands, as most of them have roughly the same behaviour of
needing to grab some info from the netdev/phy_device under rtnl, then
populate a netlink message based on that outside rtnl.

It's expected that not all ethnl commands use that ethnl framework as
some behave in a manner that don't fit the ethnl scaffholding. In the
end, the "cmd" validation is done by the generic netlink infrastructure,
that's why we don't see reports from fuzzing bots.

The WARN_ONCE we see in ethnl_default_start() and ethnl_perphy_start()
is there in case a programmer tries to use the ethnl framework without
having the ethnl ops populated.

[1] :
https://elixir.bootlin.com/linux/v6.18.6/source/net/ethtool/netlink.c#L1132

In reality, we should never end-up with an out of bounds cmd as the
validation will occur higher-up, in the genetlink part.

However, I'm OK with adding a check, or a least a comment :)

Maxime

Powered by blists - more mailing lists