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

On Wed, 16 Jun 2021 01:10:33 +0200 Michal Kubecek wrote:

quoted

@@ -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.

Ah, good point!

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.

I'll add a note to this effect to the commit message.

Thanks!