netdev - Re: [RFC PATCH net-next 2/2] net: dcb: add new apptrust attribute

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] [day] [month] [year] [list]

Message-ID: <YyDajuBHfAP/dIF6@DEN-LT-70577>
Date:   Tue, 13 Sep 2022 19:22:10 +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,

> <Daniel.Machon@...rochip.com> writes:
> >
> > Petr Machata <petrm@...dia.com> writes:
> >>
> >> 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.
> 
> Yes, a bit. But it's not too bad IMHO. Am I forgetting something here?
> 
>         u8 selectors[256];
>         int nselectors;
>         int rem;
> 
>         nla_for_each_nested(attr, ieee[DCB_ATTR_DCB_APP_TRUST_TABLE], rem) {
>                 if (nla_type(attr) != DCB_ATTR_DCB_APP_TRUST ||
>                     nla_len(attr) != 1 ||
>                     nselectors >= sizeof(selectors)) {
>                         err = -EINVAL;
>                         goto err;
>                 }
> 
>                 selectors[nselectors++] = nla_get_u8(attr);
>         }
> 
> ... and you have reconstructed the array.

LGTM.

> 
> >   - 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 have no problem with using an array for the in-kernel API. There it's
> easy to change. UAPI can't ever change.
> 
> >> > +             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).
> 
> Using iproute2 dcb is not mandatory, the UAPI is client-agnostic. The
> kernel needs to bounce bogons as well. Otherwise they will become part
> of the UAPI with the meaning "this doesn't do anything".
> 
> And yeah, drivers will validate supported configurations. But still the
> requests that go to the driver should already be sanitized, so that the
> driver code doesn't need to worry about this.

Good point.

Will prepare a v2 with suggested changes.