| 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: <Y4CJWeNMMacAwHiL@DEN-LT-70577>
Date: Fri, 25 Nov 2022 09:11:14 +0000
From: <Daniel.Machon@...rochip.com>
To: <petrm@...dia.com>
CC: <netdev@...r.kernel.org>, <dsahern@...nel.org>,
<stephen@...workplumber.org>, <maxime.chevallier@...tlin.com>,
<vladimir.oltean@....com>, <UNGLinuxDriver@...rochip.com>
Subject: Re: [PATCH iproute2-next 2/2] dcb: add new subcommand for apptrust
As always, thank you for the feedback - much appreciated!
> > diff --git a/dcb/dcb_apptrust.c b/dcb/dcb_apptrust.c
> > new file mode 100644
> > index 000000000000..14d18dcb7f83
> > --- /dev/null
> > +++ b/dcb/dcb_apptrust.c
> > @@ -0,0 +1,291 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +
> > +#include <errno.h>
> > +#include <linux/dcbnl.h>
> > +
> > +#include "dcb.h"
> > +#include "utils.h"
> > +
> > +static void dcb_apptrust_help_set(void)
> > +{
> > + fprintf(stderr,
> > + "Usage: dcb apptrust set dev STRING\n"
> > + " [ order [ eth | stream | dgram | any | dscp | pcp ] ]\n"
> > + "\n");
> > +}
> > +
> > +static void dcb_apptrust_help_show(void)
> > +{
> > + fprintf(stderr, "Usage: dcb [ -i ] apptrust show dev STRING\n"
> > + " [ order ]\n"
> > + "\n");
> > +}
> > +
> > +static void dcb_apptrust_help(void)
> > +{
> > + fprintf(stderr, "Usage: dcb apptrust help\n"
> > + "\n");
> > + dcb_apptrust_help_show();
> > + dcb_apptrust_help_set();
> > +}
> > +
> > +static const char *const selector_names[] = {
> > + [IEEE_8021QAZ_APP_SEL_ETHERTYPE] = "eth",
> > + [IEEE_8021QAZ_APP_SEL_STREAM] = "stream",
> > + [IEEE_8021QAZ_APP_SEL_DGRAM] = "dgram",
> > + [IEEE_8021QAZ_APP_SEL_ANY] = "any",
> > + [IEEE_8021QAZ_APP_SEL_DSCP] = "dscp",
> > + [DCB_APP_SEL_PCP] = "pcp",
> > +};
>
> These names should match how dcb-app names them. So ethtype,
> stream-port, dgram-port, port, dscp, pcp.
My intention was to match the selector names from the uapi header, and
not the dcb-app names (except "eth" should actually have been
"ethertype"). Afterall, we are specifying selector trust. But then
again, the user does not know about selector names, only what is visible
through dcb-app help() and the dcb-app man page. I guess you are right :)
>
> > +
> > +struct dcb_apptrust_table {
> > + __u8 selectors[IEEE_8021QAZ_APP_SEL_MAX + 1];
> > + int nselectors;
> > +};
> > +
> > +static bool dcb_apptrust_contains(const struct dcb_apptrust_table *table,
> > + __u8 selector)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < table->nselectors; i++)
> > + if (table->selectors[i] == selector)
> > + return true;
> > +
> > + return false;
> > +}
> > +
> > +static void dcb_apptrust_print(const struct dcb_apptrust_table *table)
> > +{
> > + const char *str;
> > + __u8 selector;
> > + int i;
> > +
> > + open_json_array(PRINT_JSON, "order");
> > + print_string(PRINT_FP, NULL, "order: ", NULL);
> > +
> > + for (i = 0; i < table->nselectors; i++) {
> > + selector = table->selectors[i];
> > + str = selector_names[selector];
> > + print_string(PRINT_ANY, NULL, "%s ", str);
> > + }
> > + print_nl();
> > +
> > + close_json_array(PRINT_JSON, "order");
> > +}
> > +
> > +static int dcb_apptrust_get_cb(const struct nlattr *attr, void *data)
> > +{
> > + struct dcb_apptrust_table *table = data;
> > + uint16_t type;
> > + __u8 selector;
> > +
> > + type = mnl_attr_get_type(attr);
> > +
> > + if (!dcb_app_attr_type_validate(type)) {
> > + fprintf(stderr,
> > + "Unknown attribute in DCB_ATTR_IEEE_APP_TRUST_TABLE: %d\n",
> > + type);
> > + return MNL_CB_OK;
> > + }
> > +
> > + if (mnl_attr_get_payload_len(attr) < 1) {
> > + fprintf(stderr,
> > + "DCB_ATTR_IEEE_APP_TRUST payload expected to have size %zd, not %d\n",
> > + sizeof(struct dcb_app), mnl_attr_get_payload_len(attr));
> > + return MNL_CB_OK;
> > + }
> > +
> > + selector = mnl_attr_get_u8(attr);
> > +
> > + /* Check that selector is encapsulated in the right attribute */
> > + if (!dcb_app_selector_validate(type, selector)) {
> > + fprintf(stderr, "Wrong type for selector: %s\n",
> > + selector_names[selector]);
> > + return MNL_CB_OK;
> > + }
> > +
> > + table->selectors[table->nselectors++] = selector;
> > +
> > + return MNL_CB_OK;
> > +}
> > +
> > +static int dcb_apptrust_get(struct dcb *dcb, const char *dev,
> > + struct dcb_apptrust_table *table)
> > +{
> > + uint16_t payload_len;
> > + void *payload;
> > + int ret;
> > +
> > + ret = dcb_get_attribute_va(dcb, dev, DCB_ATTR_DCB_APP_TRUST_TABLE,
> > + &payload, &payload_len);
> > + if (ret != 0)
> > + return ret;
> > +
> > + ret = mnl_attr_parse_payload(payload, payload_len, dcb_apptrust_get_cb,
> > + table);
> > + if (ret != MNL_CB_OK)
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +static int dcb_apptrust_set_cb(struct dcb *dcb, struct nlmsghdr *nlh,
> > + void *data)
> > +{
> > + const struct dcb_apptrust_table *table = data;
> > + enum ieee_attrs_app type;
> > + struct nlattr *nest;
> > + int i;
> > +
> > + nest = mnl_attr_nest_start(nlh, DCB_ATTR_DCB_APP_TRUST_TABLE);
> > +
> > + for (i = 0; i < table->nselectors; i++) {
> > + type = dcb_app_attr_type_get(table->selectors[i]);
> > + mnl_attr_put_u8(nlh, type, table->selectors[i]);
> > + }
> > +
> > + mnl_attr_nest_end(nlh, nest);
> > +
> > + return 0;
> > +}
> > +
> > +static int dcb_apptrust_set(struct dcb *dcb, const char *dev,
> > + const struct dcb_apptrust_table *table)
> > +{
> > + return dcb_set_attribute_va(dcb, DCB_CMD_IEEE_SET, dev,
> > + &dcb_apptrust_set_cb, (void *)table);
> > +}
> > +
> > +static int dcb_apptrust_parse_selector_list(int *argcp, char ***argvp,
> > + struct dcb_apptrust_table *table)
> > +{
> > + char **argv = *argvp;
> > + int argc = *argcp;
> > + __u8 selector;
> > + int ret;
> > +
> > + NEXT_ARG_FWD();
> > +
> > + /* No trusted selectors ? */
> > + if (argc == 0)
> > + goto out;
> > +
> > + while (argc > 0) {
> > + selector = parse_one_of("order", *argv, selector_names,
> > + ARRAY_SIZE(selector_names), &ret);
> > + if (ret < 0)
> > + return -EINVAL;
>
> I think this should legitimately conclude the parsing, because it could
> be one of the higher-level keywords. Currently there's only one,
> "order", but nonetheless. I think it should goto out, and be plonked by
> the caller with "what is X?". Similar to how the first argument that
> doesn't parse as e.g. DSCP:PRIO bails out and is attempted as a keyword
> higher up, and either parsed, or plonked with "what is X".
I dont quite follow you on this one. We are parsing the selector list
here. Any offending selector is printed, as well as the entire list of
valid ones. How could it be one of the higher-level keywords? Am I
missing something here? :-)
>
> > +
> > + if (table->nselectors > IEEE_8021QAZ_APP_SEL_MAX)
>
> Yeah, this is purely theoretical, so no need for a message.
>
> > + return -ERANGE;
> > +
> > + if (dcb_apptrust_contains(table, selector)) {
> > + fprintf(stderr, "Duplicate selector: %s\n",
> > + selector_names[selector]);
> > + return -EINVAL;
> > + }
> > +
> > + table->selectors[table->nselectors++] = selector;
> > +
> > + NEXT_ARG_FWD();
> > + }
> > +
> > +out:
> > + *argcp = argc;
> > + *argvp = argv;
> > +
> > + return 0;
> > +}
> > +
> > +static int dcb_cmd_apptrust_set(struct dcb *dcb, const char *dev, int argc,
> > + char **argv)
> > +{
> > + struct dcb_apptrust_table table = { 0 };
> > + int ret;
> > +
> > + if (!argc) {
> > + dcb_apptrust_help_set();
> > + return 0;
> > + }
> > +
> > + do {
> > + if (strcmp(*argv, "help") == 0) {
> > + dcb_apptrust_help_set();
> > + return 0;
> > + } else if (strcmp(*argv, "order") == 0) {
> > + ret = dcb_apptrust_parse_selector_list(&argc, &argv,
> > + &table);
> > + if (ret < 0) {
> > + fprintf(stderr, "Invalid list of selectors\n");
> > + return -EINVAL;
> > + }
> > + continue;
> > + } else {
> > + fprintf(stderr, "What is \"%s\"?\n", *argv);
> > + dcb_apptrust_help_set();
> > + return -EINVAL;
> > + }
> > +
> > + NEXT_ARG_FWD();
> > + } while (argc > 0);
> > +
> > + return dcb_apptrust_set(dcb, dev, &table);
> > +}
> > +
> > +static int dcb_cmd_apptrust_show(struct dcb *dcb, const char *dev, int argc,
> > + char **argv)
> > +{
> > + struct dcb_apptrust_table table = { 0 };
> > + int ret;
> > +
> > + ret = dcb_apptrust_get(dcb, dev, &table);
> > + if (ret)
> > + return ret;
> > +
> > + open_json_object(NULL);
> > +
> > + if (!argc) {
> > + dcb_apptrust_help();
>
> Given no arguments to show, the tool should show everything, not help.
Ahh. Yes. Will fix that.
>
> > + goto out;
> > + }
> > +
> > + do {
> > + if (strcmp(*argv, "help") == 0) {
> > + dcb_apptrust_help_show();
> > + return 0;
> > + } else if (strcmp(*argv, "order") == 0) {
> > + dcb_apptrust_print(&table);
>
> This should probably be dcb_apptrust_print_order, so that more stuff can
> be added cleanly.
>
> > + } else {
> > + fprintf(stderr, "What is \"%s\"?\n", *argv);
> > + dcb_apptrust_help_show();
> > + return -EINVAL;
> > + }
> > +
> > + NEXT_ARG_FWD();
> > + } while (argc > 0);
> > +
> > +out:
> > + close_json_object();
> > + return 0;
> > +}
> > +
> > +int dcb_cmd_apptrust(struct dcb *dcb, int argc, char **argv)
> > +{
> > + if (!argc || strcmp(*argv, "help") == 0) {
> > + dcb_apptrust_help();
> > + return 0;
> > + } else if (strcmp(*argv, "show") == 0) {
> > + NEXT_ARG_FWD();
> > + return dcb_cmd_parse_dev(dcb, argc, argv, dcb_cmd_apptrust_show,
> > + dcb_apptrust_help_show);
> > + } else if (strcmp(*argv, "set") == 0) {
> > + NEXT_ARG_FWD();
> > + return dcb_cmd_parse_dev(dcb, argc, argv, dcb_cmd_apptrust_set,
> > + dcb_apptrust_help_set);
> > + } else {
> > + fprintf(stderr, "What is \"%s\"?\n", *argv);
> > + dcb_apptrust_help();
> > + return -EINVAL;
> > + }
> > +}
> > diff --git a/man/man8/dcb-apptrust.8 b/man/man8/dcb-apptrust.8
> > new file mode 100644
> > index 000000000000..9ebe7c17292c
> > --- /dev/null
> > +++ b/man/man8/dcb-apptrust.8
> > @@ -0,0 +1,118 @@
> > +.TH DCB-APPTRUST 8 "22 November 2022" "iproute2" "Linux"
> > +.SH NAME
> > +dcb-apptrust \- show / manipulate per-selector trust and trust order of the application
> > +priority table of the DCB (Data Center Bridging) subsystem.
> > +.SH SYNOPSIS
> > +.sp
> > +.ad l
> > +.in +8
> > +
> > +.ti -8
> > +.B dcb
> > +.RI "[ " OPTIONS " ] "
> > +.B apptrust
> > +.RI "{ " COMMAND " | " help " }"
> > +.sp
> > +
> > +.ti -8
> > +.B dcb apptrust show dev order
> > +.RI DEV
> > +
> > +.ti -8
> > +.B dcb apptrust set dev order
> > +.RI DEV
> > +.RB "[ " eth " ]"
> > +.RB "[ " stream " ]"
> > +.RB "[ " dgram " ]"
> > +.RB "[ " any " ]"
> > +.RB "[ " dscp " ]"
> > +.RB "[ " pcp " ]"
>
> Taken literally, this prescribes the order, just allows omitting some of
> the selectors. I think you'll need to circumscribe like this:
>
> dcb apptrust set dev order [ SEL-LIST ]
> SEL-LIST := [ SEL-LIST ] SEL
> SEL := { ethtype | stream-port | etc. etc. }
>
Ack. Will be fixed.
> > +.SH DESCRIPTION
> > +
> > +.B dcb apptrust
> > +is used to configure and manipulate per-selector trust and trust order of the
> > +Application Priority Table, see
> > +.BR dcb-app (8)
> > +for details on how to configure app table entries.
> > +
> > +Selector trust can be used by the
> > +software stack, or drivers (most likely the latter), when querying the APP
> > +table, to determine if an APP entry should take effect, or not. Additionaly, the
> > +order of the trusted selectors will dictate which selector should take
> > +precedence, in the case of multiple different APP selectors being present in the
> > +APP table.
> > +
> > +.SH COMMANDS
> > +
> > +.TP
> > +.B show
> > +Display all trusted selectors.
> > +
> > +.TP
> > +.B set
> > +Set new list of trusted selectors. Empty list is effectively the same as
> > +removing trust entirely.
> > +
> > +.SH PARAMETERS
> > +
> > +The following describes only the write direction, i.e. as used with the
> > +\fBset\fR command. For the \fBshow\fR command, the parameter name is to be used
> > +as a simple keyword without further arguments. This instructs the tool to show
> > +the values of a given parameter.
> > +
> > +.TP
> > +.B order \fISELECTOR-NAMES
> > +\fISELECTOR-NAMES\fR is a space-separated list of selector names:\fR
> > +
> > +.B eth
> > +Trust EtherType.
> > +
> > +.B stream
> > +Trust TCP, or Stream Control Transmission Protocol (SCTP).
> > +
> > +.B dgram
> > +Trust UDP, or Datagram Congestion Control Protocol (DCCP).
> > +
> > +.B any
> > +Trust TCP, SCTP, UDP, or DCCP.
> > +
> > +.B dscp
> > +Trust Differentiated Services Code Point (DSCP) values.
> > +
> > +.B pcp
> > +Trust Priority Code Point/Drop Eligible Indicator (PCP/DEI).
>
> These names need to be updated as well.
Ack. Will be fixed.
/ Daniel
Powered by blists - more mailing lists