Re: [PATCH net-next v2 10/10] ethtool: netlink: always pass genl_info to .prepare_data

[Jiri Pirko <jiri@resnulli.us>]

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:
>> Fri, Aug 11, 2023 at 01:38:45AM CEST, kuba@kernel.org wrote:
>> >@@ -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.
>
>> 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.
>
>> 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.