Fri, Aug 11, 2023 at 09:13:24AM CEST, mkubecek@suse.cz wrote:

On Fri, Aug 11, 2023 at 08:42:45AM +0200, Jiri Pirko wrote:

quoted

Fri, Aug 11, 2023 at 01:38:45AM CEST, kuba@kernel.org wrote:

quoted

@@ -24,7 +24,7 @@ const struct nla_policy ethnl_wol_get_policy[] = {

static int wol_prepare_data(const struct ethnl_req_info *req_base,
			    struct ethnl_reply_data *reply_base,
-			    struct genl_info *info)
+			    const struct genl_info *info)
{
	struct wol_reply_data *data = WOL_REPDATA(reply_base);
	struct net_device *dev = reply_base->dev;

@@ -39,7 +39,8 @@ static int wol_prepare_data(const struct ethnl_req_info *req_base,

	dev->ethtool_ops->get_wol(dev, &data->wol);
	ethnl_ops_complete(dev);
	/* do not include password in notifications */
-	data->show_sopass = info && (data->wol.supported & WAKE_MAGICSECURE);
+	data->show_sopass = genl_info_is_ntf(info) &&
+		(data->wol.supported & WAKE_MAGICSECURE);

I believe that you are missing "!" here:
	data->show_sopass = !genl_info_is_ntf(info) &&
		(data->wol.supported & WAKE_MAGICSECURE);

Agreed.

quoted

But, you are changing the output for dumpit if I'm not mistaken.
ethnl_default_dump_one() currently calls this with info==NULL too, not
only ethnl_default_notify().

I would rather see this as a fix. Not showing the password in dumps made
little sense as it meant the dump output was different from single
device queries. It was the price to pay for inability to distinguish
between a dump and a notification.

IIRC the early versions submitted went even further and did not set
GENL_UNS_ADMIN_PERM for ETHTOOL_MSG_WOL_GET and only omitted the
password when the request came from an unprivileged process (so that
unprivileged processes could still query the rest of WoL information)
but this was dropped during the review as an unnecessary complication.

quoted

Anyway, the genl_info_is_ntf() itself seems a bit odd to me. The only
user is here and I doubt there ever going to be any other. This
conditional per-op attr fill seems a bit odd.

Can't you handle this in side ethtool somehow? IDK :/

I don't think so. The point here is that notification can be seen by any
unprivileged process so as long as we agree that those should not see
the wake up passwords, we must not include the password in them. While
ethtool could certanly drop the password from its output, any other
utility parsing the notifications (or even patched ethtool) could still
show it to anyone.

Yeah, the question is, if it is a good design to have one CMD type
to conditionally send sensitive data. I would argue that sensitive data
could be sent over separate CMD with no notifier for it.