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

Date:   Tue, 13 Sep 2022 13:25:07 +0200
From:   Petr Machata <petrm@...dia.com>
To:     <Daniel.Machon@...rochip.com>
CC:     <petrm@...dia.com>, <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


<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.

>   - 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.