| 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: <YyBI/tF5x+3OE2dB@DEN-LT-70577>
Date: Tue, 13 Sep 2022 09:01:07 +0000
From: <Daniel.Machon@...rochip.com>
To: <petrm@...dia.com>
CC: <netdev@...r.kernel.org>, <Allan.Nielsen@...rochip.com>,
<UNGLinuxDriver@...rochip.com>, <maxime.chevallier@...tlin.com>,
<vladimir.oltean@....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
Hi Petr,
Thank you for your feedback.
> 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.
Ack.
> 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.
Yes, this is the number of selector-occupied indexes. I dont have any strongs
feelings on num vs count - we can go with count.
>
> > + __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.)
Ack.
>
> 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?
Hmm. It might seem excessive, but a quick few thoughts on your proposed solution:
- We need more code to define and parse the new DCB_ATTR_IEEE_APP_TRUST_TABLE /
DCB_ATTR_IEEE_APP_TRUST attributes.
- If the selectors are passed individually to the driver, we need a
dcbnl_delapptrust(), because now, the driver have to add and del from the
driver maintained array. You could of course accumulate selectors in an array
before passing them to the driver, but then why go away from the array in the
first place.
I kind of like the 'set' nature more than the 'add' 'del' nature. What do you think?
>
> > +};
> > +
> > /**
> > * 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.
It does give a warning. No empty lines on any of the other declarations though,
but lets indeed add one here.
>
> 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.
Agree.
>
> > + 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.
Validation (bogus input and unique selectors) is done in userspace (dcb-apptrust).
>
> > + 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