netdev - Re: [PATCH net-next 1/2] ethtool: Introduce WAKE

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 for Android: free password hash cracker in your pocket

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

Date: Thu, 12 Oct 2023 01:08:21 +0200
From: Michal Kubecek <mkubecek@...e.cz>
To: Florian Fainelli <florian.fainelli@...adcom.com>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Jonathan Corbet <corbet@....net>,
	Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>,
	Andrew Lunn <andrew@...n.ch>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>, opendmb@...il.com,
	justin.chen@...adcom.com
Subject: Re: [PATCH net-next 1/2] ethtool: Introduce WAKE_MDA

On Wed, Oct 11, 2023 at 03:12:39PM -0700, Florian Fainelli wrote:
> Allow Ethernet PHY and MAC drivers with simplified matching logic to be
> waking-up on a custom MAC destination address. This is particularly
> useful for Ethernet PHYs which have limited amounts of buffering but can
> still wake-up on a custom MAC destination address.
> 
> When WAKE_MDA is specified, the "sopass" field in the existing struct
> ethtool_wolinfo is re-purposed to hold a custom MAC destination address
> to match against.
> 
> Example:
> 	ethtool -s eth0 wol e mac-da 01:00:5e:00:00:fb
> 
> Signed-off-by: Florian Fainelli <florian.fainelli@...adcom.com>
> ---
>  Documentation/networking/ethtool-netlink.rst |  7 ++++++-
>  include/uapi/linux/ethtool.h                 | 10 ++++++++--
>  include/uapi/linux/ethtool_netlink.h         |  1 +
>  net/ethtool/common.c                         |  1 +
>  net/ethtool/netlink.h                        |  2 +-
>  net/ethtool/wol.c                            | 21 ++++++++++++++++++++
>  6 files changed, 38 insertions(+), 4 deletions(-)
> 
[...]
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index f7fba0dc87e5..8134ac8870bd 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -207,12 +207,17 @@ struct ethtool_drvinfo {
>   * @wolopts: Bitmask of %WAKE_* flags for enabled Wake-On-Lan modes.
>   * @sopass: SecureOn(tm) password; meaningful only if %WAKE_MAGICSECURE
>   *	is set in @wolopts.
> + * @mac_da: Destination MAC address to match; meaningful only if
> + *	%WAKE_MDA is set in @wolopts.
>   */
>  struct ethtool_wolinfo {
>  	__u32	cmd;
>  	__u32	supported;
>  	__u32	wolopts;
> -	__u8	sopass[SOPASS_MAX];
> +	union {
> +		__u8	sopass[SOPASS_MAX];
> +		__u8	mac_da[ETH_ALEN];
> +	};
>  };

If we use the union here, we should also make sure the request cannot
set both WAKE_MAGICSECURE and WAKE_MDA, otherwise the same data will be
used for two different values (and interpreted in two different ways in
GET requests and notifications).

Another option would be keeping the existing structure for ioctl UAPI
and using another structure (like we did in other cases where we needed
new attributes beyond frozen ioctl structures) so that a combination of
WAKE_MAGICSECURE and WAKE_MDA is possible. (It doesn't seem to make much
sense to me to combine WAKE_MAGICSECURE with other bits but I can't rule
out someone might want that one day.)

[...]
> diff --git a/net/ethtool/wol.c b/net/ethtool/wol.c
> index 0ed56c9ac1bc..13dfcc9bb1e5 100644
> --- a/net/ethtool/wol.c
> +++ b/net/ethtool/wol.c
> @@ -12,6 +12,7 @@ struct wol_reply_data {
>  	struct ethnl_reply_data		base;
>  	struct ethtool_wolinfo		wol;
>  	bool				show_sopass;
> +	bool				show_mac_da;
>  };
>  
>  #define WOL_REPDATA(__reply_base) \
> @@ -41,6 +42,8 @@ static int wol_prepare_data(const struct ethnl_req_info *req_base,
>  	/* do not include password in notifications */
>  	data->show_sopass = !genl_info_is_ntf(info) &&
>  		(data->wol.supported & WAKE_MAGICSECURE);
> +	data->show_mac_da = !genl_info_is_ntf(info) &&
> +		(data->wol.supported & WAKE_MDA);
>  
>  	return 0;
>  }

The test for !genl_info_is_ntf(info) above is meant to prevent the
sopass value (which is supposed to be secret) from being included in
notifications which can be seen by unprivileged users. Is the MAC
address to match also supposed to be secret?

Michal

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