Re: [RFC net-next] ethtool: add a stricter length check

[Michal Kubecek <mkubecek@suse.cz>]

On Fri, Jun 11, 2021 at 08:11:35PM -0700, Jakub Kicinski wrote:
> There has been a few errors in the ethtool reply size calculations,
> most of those are hard to trigger during basic testing because of
> skb size rounding up and netdev names being shorter than max.
> Add a more precise check.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> Michal, WDYT?

It's definitely an improvement and I agree with it. I only have two
minor comments below.

> 
>  net/ethtool/netlink.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
> index 88d8a0243f35..3f9a1a96b4df 100644
> --- a/net/ethtool/netlink.c
> +++ b/net/ethtool/netlink.c
> @@ -315,9 +315,9 @@ static int ethnl_default_doit(struct sk_buff *skb, struct genl_info *info)
>  	struct ethnl_req_info *req_info = NULL;
>  	const u8 cmd = info->genlhdr->cmd;
>  	const struct ethnl_request_ops *ops;
> +	int hdr_len, reply_len;
>  	struct sk_buff *rskb;
>  	void *reply_payload;
> -	int reply_len;
>  	int ret;
>  
>  	ops = ethnl_default_requests[cmd];
> @@ -346,15 +346,20 @@ static int ethnl_default_doit(struct sk_buff *skb, struct genl_info *info)
>  	ret = ops->reply_size(req_info, reply_data);
>  	if (ret < 0)
>  		goto err_cleanup;
> -	reply_len = ret + ethnl_reply_header_size();
> +	reply_len = ret;
>  	ret = -ENOMEM;
> -	rskb = ethnl_reply_init(reply_len, req_info->dev, ops->reply_cmd,
> +	rskb = ethnl_reply_init(reply_len + ethnl_reply_header_size(),
> +				req_info->dev, ops->reply_cmd,
>  				ops->hdr_attr, info, &reply_payload);
>  	if (!rskb)
>  		goto err_cleanup;
> +	hdr_len = rskb->len;
>  	ret = ops->fill_reply(rskb, req_info, reply_data);
>  	if (ret < 0)
>  		goto err_msg;
> +	WARN(rskb->len - hdr_len > reply_len,
> +	     "ethnl cmd %d: calculated reply length %d, but consumed %d\n",
> +	     cmd, reply_len, rskb->len - hdr_len);
>  	if (ops->cleanup_data)
>  		ops->cleanup_data(reply_data);

We may want WARN_ONCE or ratelimited here, if there is bug in reply
length estimate for a request not requiring admin privileges, the
warning might be invoked by a regular user at will.

Also the patch changes the meaning of reply_len which is also used in
the original warning after err_msg label. But it's probably not a big
deal, it's not obvious what exactly "payload" means there so that anyone
trying to investigate the problem has to start by checking what exactly
the value reported means.

Michal