| 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
| ||
|
Message-ID: <87czc0efij.fsf@nvidia.com>
Date: Mon, 12 Sep 2022 16:31:34 +0200
From: Petr Machata <petrm@...dia.com>
To: Daniel Machon <daniel.machon@...rochip.com>
CC: <netdev@...r.kernel.org>, <Allan.Nielsen@...rochip.com>,
<UNGLinuxDriver@...rochip.com>, <maxime.chevallier@...tlin.com>,
<vladimir.oltean@....com>, <petrm@...dia.com>, <kuba@...nel.org>,
<vinicius.gomes@...el.com>, <thomas.petazzoni@...tlin.com>
Subject: Re: [RFC PATCH net-next 2/2] net: dcb: add new apptrust attribute
Daniel Machon <daniel.machon@...rochip.com> writes:
> Add a new apptrust extension attribute to the 8021Qaz APP managed
> object.
>
> The new attribute is meant to allow drivers, whose hw supports the
> notion of trust, to be able to set whether a particular app selector is
> to be trusted - and also the order of precedence of selectors.
>
> A new structure ieee_apptrust has been created, which contains an array
> of selectors, where lower indexes has higher precedence.
>
> Signed-off-by: Daniel Machon <daniel.machon@...rochip.com>
> ---
> include/net/dcbnl.h | 2 ++
> include/uapi/linux/dcbnl.h | 14 ++++++++++++++
> net/dcb/dcbnl.c | 17 +++++++++++++++++
> 3 files changed, 33 insertions(+)
>
> diff --git a/include/net/dcbnl.h b/include/net/dcbnl.h
> index 2b2d86fb3131..0c4b0107981d 100644
> --- a/include/net/dcbnl.h
> +++ b/include/net/dcbnl.h
> @@ -61,6 +61,8 @@ struct dcbnl_rtnl_ops {
> int (*ieee_getapp) (struct net_device *, struct dcb_app *);
> int (*ieee_setapp) (struct net_device *, struct dcb_app *);
> int (*ieee_delapp) (struct net_device *, struct dcb_app *);
> + int (*ieee_setapptrust) (struct net_device *, struct ieee_apptrust *);
> + int (*ieee_getapptrust) (struct net_device *, struct ieee_apptrust *);
> int (*ieee_peer_getets) (struct net_device *, struct ieee_ets *);
> int (*ieee_peer_getpfc) (struct net_device *, struct ieee_pfc *);
>
> diff --git a/include/uapi/linux/dcbnl.h b/include/uapi/linux/dcbnl.h
> index 8eab16e5bc13..833466dec096 100644
> --- a/include/uapi/linux/dcbnl.h
> +++ b/include/uapi/linux/dcbnl.h
> @@ -248,6 +248,19 @@ struct dcb_app {
> __u16 protocol;
> };
>
> +#define IEEE_8021QAZ_APP_SEL_MAX 255
> +
> +/* This structure contains trust order extension to the IEEE 802.1Qaz APP
> + * managed object.
> + *
> + * @order: contains trust ordering of selector values for the IEEE 802.1Qaz
> + * APP managed object. Lower indexes has higher trust.
> + */
> +struct ieee_apptrust {
Trust level setting is not standard, so this should be something like
dcbnl_apptrust.
Ditto for DCB_ATTR_IEEE_APP_TRUST below. I believe DCB_ATTR_DCB_BUFFER
is in the DCB namespace for that reason, and the trust level artifacts
should be as well. Likewise with the DCB ops callbacks, which should
IMHO be dcbnl_get/setapptrust.
> + __u8 num;
Is this supposed to be a count of the "order" items?
If yes, I'd call it "count", because then it's clear the value is not
just a number, but actually a number of items.
> + __u8 order[IEEE_8021QAZ_APP_SEL_MAX];
Should be +1 IMHO, in case the whole u8 selector space is allocated. (In
particular, there's no guarantee that IEEE won't ever allocate the
selector 0.)
But of course this will never get anywhere close to that. We will end up
passing maybe one, two entries. So the UAPI seems excessive in how it
hands around this large array.
I wonder if it would be better to make the DCB_ATTR_IEEE_APP_TABLE
payload be an array of bytes, each byte a selector? Or something similar
to DCB_ATTR_IEEE_APP_TABLE / DCB_ATTR_IEEE_APP, a nest and an array of
payload attributes?
> +};
> +
> /**
> * struct dcb_peer_app_info - APP feature information sent by the peer
> *
> @@ -419,6 +432,7 @@ enum ieee_attrs {
> DCB_ATTR_IEEE_QCN,
> DCB_ATTR_IEEE_QCN_STATS,
> DCB_ATTR_DCB_BUFFER,
> + DCB_ATTR_IEEE_APP_TRUST,
> __DCB_ATTR_IEEE_MAX
> };
> #define DCB_ATTR_IEEE_MAX (__DCB_ATTR_IEEE_MAX - 1)
> diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c
> index dc4fb699b56c..e87f0128c3bd 100644
> --- a/net/dcb/dcbnl.c
> +++ b/net/dcb/dcbnl.c
> @@ -162,6 +162,7 @@ static const struct nla_policy dcbnl_ieee_policy[DCB_ATTR_IEEE_MAX + 1] = {
> [DCB_ATTR_IEEE_ETS] = {.len = sizeof(struct ieee_ets)},
> [DCB_ATTR_IEEE_PFC] = {.len = sizeof(struct ieee_pfc)},
> [DCB_ATTR_IEEE_APP_TABLE] = {.type = NLA_NESTED},
> + [DCB_ATTR_IEEE_APP_TRUST] = {.len = sizeof(struct ieee_apptrust)},
> [DCB_ATTR_IEEE_MAXRATE] = {.len = sizeof(struct ieee_maxrate)},
> [DCB_ATTR_IEEE_QCN] = {.len = sizeof(struct ieee_qcn)},
> [DCB_ATTR_IEEE_QCN_STATS] = {.len = sizeof(struct ieee_qcn_stats)},
> @@ -1133,6 +1134,14 @@ static int dcbnl_ieee_fill(struct sk_buff *skb, struct net_device *netdev)
> spin_unlock_bh(&dcb_lock);
> nla_nest_end(skb, app);
>
> + if (ops->ieee_getapptrust) {
> + struct ieee_apptrust trust;
I believe checkpatch warns if there's no empty line between the variable
declaration block and the rest of the code.
Maybe it's just because this is an RFC, but for the final submission
please make sure you run checkpatch.pl --max-line-length=80. The
80-character limit is not really a hard requirement anymore, but it's
still strongly preferred in net.
> + memset(&trust, 0, sizeof(trust));
> + err = ops->ieee_getapptrust(netdev, &trust);
> + if (!err && nla_put(skb, DCB_ATTR_IEEE_APP_TRUST, sizeof(trust), &trust))
> + return -EMSGSIZE;
> + }
> +
> /* get peer info if available */
> if (ops->ieee_peer_getets) {
> struct ieee_ets ets;
> @@ -1513,6 +1522,14 @@ static int dcbnl_ieee_set(struct net_device *netdev, struct nlmsghdr *nlh,
> }
> }
>
> + if (ieee[DCB_ATTR_IEEE_APP_TRUST] && ops->ieee_setapptrust) {
Hmm, weird that none of the set requests are bounced if there's no set
callback. That way the request appears to succeed but doesn't actually
set anything. I find this very strange.
Drivers that do not even have any DCB ops give -EOPNOTSUPP as expected.
I think lack of a particular op should do this as well. We can't change
this for the existing ones, but IMHO the new OPs should be like that.
> + struct ieee_apptrust *trust =
> + nla_data(ieee[DCB_ATTR_IEEE_APP_TRUST]);
Besides invoking the OP, this should validate the payload. E.g. no
driver is supposed to accept trust policies that contain invalid
selectors. Pretty sure there's no value in repeated entries either.
> + err = ops->ieee_setapptrust(netdev, trust);
> + if (err)
> + goto err;
> + }
> +
> err:
> err = nla_put_u8(skb, DCB_ATTR_IEEE, err);
> dcbnl_ieee_notify(netdev, RTM_SETDCB, DCB_CMD_IEEE_SET, seq, 0);
Powered by blists - more mailing lists