linux-kernel - Re: [PATCH net-next v6 05/15] ethtool: helper functions for netlink interface

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: <20190708144008.GK2201@nanopsycho>
Date:   Mon, 8 Jul 2019 16:40:08 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Michal Kubecek <mkubecek@...e.cz>
Cc:     netdev@...r.kernel.org, David Miller <davem@...emloft.net>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        John Linville <linville@...driver.com>,
        Stephen Hemminger <stephen@...workplumber.org>,
        Johannes Berg <johannes@...solutions.net>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v6 05/15] ethtool: helper functions for netlink
 interface

Mon, Jul 08, 2019 at 02:22:51PM CEST, mkubecek@...e.cz wrote:
>On Wed, Jul 03, 2019 at 12:04:35PM +0200, Jiri Pirko wrote:
>> Tue, Jul 02, 2019 at 06:34:37PM CEST, mkubecek@...e.cz wrote:
>> >On Tue, Jul 02, 2019 at 03:05:15PM +0200, Jiri Pirko wrote:
>> >> Tue, Jul 02, 2019 at 01:50:04PM CEST, mkubecek@...e.cz wrote:
>> >> >+/**
>> >> >+ * ethnl_is_privileged() - check if request has sufficient privileges
>> >> >+ * @skb: skb with client request
>> >> >+ *
>> >> >+ * Checks if client request has CAP_NET_ADMIN in its netns. Unlike the flags
>> >> >+ * in genl_ops, this allows finer access control, e.g. allowing or denying
>> >> >+ * the request based on its contents or witholding only part of the data
>> >> >+ * from unprivileged users.
>> >> >+ *
>> >> >+ * Return: true if request is privileged, false if not
>> >> >+ */
>> >> >+static inline bool ethnl_is_privileged(struct sk_buff *skb)
>> >> 
>> >> I wonder why you need this helper. Genetlink uses
>> >> ops->flags & GENL_ADMIN_PERM for this. 
>> >
>> >It's explained in the function description. Sometimes we need finer
>> >control than by request message type. An example is the WoL password:
>> >ETHTOOL_GWOL is privileged because of it but I believe there si no
>> >reason why unprivileged user couldn't see enabled WoL modes, we can
>> >simply omit the password for him. (Also, it allows to combine query for
>> >WoL settings with other unprivileged settings.)
>> 
>> Why can't we have rather:
>> ETHTOOL_WOL_GET for all
>> ETHTOOL_WOL_PASSWORD_GET  with GENL_ADMIN_PERM
>> ?
>> Better to stick with what we have in gennetlink rather then to bend the
>> implementation from the very beginning I think.
>
>We can. But it would also mean two separate SET requests (or breaking
>the rule that _GET_REPLY, _SET and _NTF share the layout). That would be
>unfortunate as ethtool_ops callback does not actually allow setting only
>the modes so that the ETHTOOL_MSG_WOL_SET request (which would have to
>go first as many drivers ignore .sopass if WAKE_MAGICSECURE is not set)
>would have to pass a different password (most likely just leaving what
>->get_wol() put there) and that password would be actually set until the
>second request arrives. There goes the idea of getting rid of ioctl
>interface raciness...

I understand. That is my concern, not to bring baggage from ioclt :/


>
>I would rather see returning to WoL modes not being visible to
>unprivileged users than that (even if there is no actual reason for it).
>Anyway, shortening the series left WoL settings out if the first part so
>that I can split this out for now and leave the discussion for when we
>get to WoL one day.

Fine.


>
>> >> >+/**
>> >> >+ * ethnl_reply_header_size() - total size of reply header
>> >> >+ *
>> >> >+ * This is an upper estimate so that we do not need to hold RTNL lock longer
>> >> >+ * than necessary (to prevent rename between size estimate and composing the
>> >> 
>> >> I guess this description is not relevant anymore. I don't see why to
>> >> hold rtnl mutex for this function...
>> >
>> >You don't need it for this function, it's the other way around: unless
>> >you hold RTNL lock for the whole time covering both checking needed
>> >message size and filling the message - and we don't - the device could
>> >be renamed in between. Thus if we returned size based on current device
>> >name, it might not be sufficient at the time the header is filled.
>> >That's why this function returns maximum possible size (which is
>> >actually a constant).
>> 
>> I suggest to avoid the description. It is misleading. Perhaps something
>> to have in a patch description but not here in code.
>
>The reason I put the comment there was to prevent someone "optimizing"
>the helper by using strlen() later. Maybe something shorter and more to
>the point, e.g.
>
>  Using IFNAMSIZ is faster and prevents a race if the device is renamed
>  before we fill the name into skb.
>
>?

Sounds good, thanks!


>
>Michal