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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230811071324.gfkzlpb3gbwvuufm@lion.mk-sys.cz>
Date: Fri, 11 Aug 2023 09:13:24 +0200
From: Michal Kubecek <mkubecek@...e.cz>
To: Jiri Pirko <jiri@...nulli.us>
Cc: Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net,
	netdev@...r.kernel.org, edumazet@...gle.com, pabeni@...hat.com,
	johannes@...solutions.net,
	Vladimir Oltean <vladimir.oltean@....com>, gal@...dia.com,
	tariqt@...dia.com, lucien.xin@...il.com, f.fainelli@...il.com,
	andrew@...n.ch, simon.horman@...igine.com, linux@...pel-privat.de
Subject: Re: [PATCH net-next v2 10/10] ethtool: netlink: always pass
 genl_info to .prepare_data

On Fri, Aug 11, 2023 at 08:42:45AM +0200, Jiri Pirko wrote:
> Fri, Aug 11, 2023 at 01:38:45AM CEST, kuba@...nel.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.

Michal

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)