| 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: <ZLl6mF8iKBD/OeHb@corigine.com>
Date: Thu, 20 Jul 2023 19:19:04 +0100
From: Simon Horman <simon.horman@...igine.com>
To: Oleksij Rempel <o.rempel@...gutronix.de>
Cc: "David S. Miller" <davem@...emloft.net>, Andrew Lunn <andrew@...n.ch>,
Eric Dumazet <edumazet@...gle.com>,
Florian Fainelli <f.fainelli@...il.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Vladimir Oltean <olteanv@...il.com>,
Woojung Huh <woojung.huh@...rochip.com>,
Arun Ramadoss <arun.ramadoss@...rochip.com>,
"Russell King (Oracle)" <linux@...linux.org.uk>,
kernel@...gutronix.de, linux-kernel@...r.kernel.org,
netdev@...r.kernel.org, UNGLinuxDriver@...rochip.com,
Petr Machata <petrm@...dia.com>
Subject: Re: [PATCH net-next v2 1/1] net: dsa: microchip: Add partial ACL
support for ksz9477 switches
On Wed, Jul 19, 2023 at 01:19:29PM +0200, Oleksij Rempel wrote:
...
Hi Oleksij,
> +/**
> + * ksz9477_acl_port_enable - Enables ACL functionality on a given port.
> + * @dev: The ksz_device instance.
> + * @port: The port number on which to enable ACL functionality.
> + *
> + * This function enables ACL functionality on the specified port by configuring
> + * the appropriate control registers. It returns 0 if the operation is
> + * successful, or a negative error code if an error occurs.
> + *
> + * 0xn801 - KSZ9477S 5.2.8.2 Port Priority Control Register
> + * Bit 7 - Highest Priority
> + * Bit 6 - OR'ed Priority
> + * Bit 4 - MAC Address Priority Classification
> + * Bit 3 - VLAN Priority Classification
> + * Bit 2 - 802.1p Priority Classification
> + * Bit 1 - Diffserv Priority Classification
> + * Bit 0 - ACL Priority Classification
> + *
> + * Note: current driver implementation sets 802.1p priority classification
> + * by default. In this function we add ACL priority classification with OR'ed
> + * priority. According to testing, priority set by ACL will supersede the
> + * 802.1p priority.
> + *
> + * 0xn803 - KSZ9477S 5.2.8.4 Port Authentication Control Register
> + * Bit 2 - Access Control List (ACL) Enable
> + * Bits 1:0 - Authentication Mode
> + * 00 = Reserved
> + * 01 = Block Mode. Authentication is enabled. When ACL is
> + * enabled, all traffic that misses the ACL rules is
> + * blocked; otherwise ACL actions apply.
> + * 10 = Pass Mode. Authentication is disabled. When ACL is
> + * enabled, all traffic that misses the ACL rules is
> + * forwarded; otherwise ACL actions apply.
> + * 11 = Trap Mode. Authentication is enabled. All traffic is
> + * forwarded to the host port. When ACL is enabled, all
> + * traffic that misses the ACL rules is blocked; otherwise
> + * ACL actions apply.
> + *
> + * Note: we are using Pass Mode here.
kernel-doc gcc-12 W=1 and clang-16 W=1 all complains
that there is a duplicate Note section in the above.
...
> +/**
> + * ksz9477_acl_matching_rule_cfg_l2 - Configure an ACL filtering entry to match
> + * L2 types of Ethernet frames
> + * @entry: Pointer to ACL entry buffer
> + * @ethertype: Ethertype value
> + * @eth_addr: Pointer to Ethernet address
> + * @is_src: If true, match the source MAC address; if false, match the
> + * destination MAC address
> + *
> + * This function configures an Access Control List (ACL) filtering
> + * entry to match Layer 2 types of Ethernet frames based on the provided
> + * ethertype and Ethernet address. Additionally, it can match either the source
> + * or destination MAC address depending on the value of the is_src parameter.
> + *
> + * Register Descriptions for MD = 01 and ENB != 00 (Layer 2 MAC header
> + * filtering)
> + *
> + * 0x01 - Mode and Enable
> + * Bits 5:4 - MD (Mode)
> + * 01 = Layer 2 MAC header or counter filtering
> + * Bits 3:2 - ENB (Enable)
> + * 01 = Comparison is performed only on the TYPE value
> + * 10 = Comparison is performed only on the MAC Address value
> + * 11 = Both the MAC Address and TYPE are tested
> + * Bit 1 - S/D (Source / Destination)
> + * 0 = Destination address
> + * 1 = Source address
> + * Bit 0 - EQ (Equal / Not Equal)
> + * 0 = Not Equal produces true result
> + * 1 = Equal produces true result
> + *
> + * 0x02-0x07 - MAC Address
> + * 0x02 - MAC Address [47:40]
> + * 0x03 - MAC Address [39:32]
> + * 0x04 - MAC Address [31:24]
> + * 0x05 - MAC Address [23:16]
> + * 0x06 - MAC Address [15:8]
> + * 0x07 - MAC Address [7:0]
> + *
> + * 0x08-0x09 - EtherType
> + * 0x08 - EtherType [15:8]
> + * 0x09 - EtherType [7:0]
> + */
> +static void ksz9477_acl_matching_rule_cfg_l2(u8 *entry, __be16 ethertype,
> + u8 *eth_addr, bool is_src)
> +{
> + u8 enb = 0;
> + u8 val;
> +
> + if (ethertype)
> + enb |= KSZ9477_ACL_ENB_L2_TYPE;
> + if (eth_addr)
> + enb |= KSZ9477_ACL_ENB_L2_MAC;
> +
> + val = FIELD_PREP(KSZ9477_ACL_MD_MASK, KSZ9477_ACL_MD_L2_MAC) |
> + FIELD_PREP(KSZ9477_ACL_ENB_MASK, enb) |
> + FIELD_PREP(KSZ9477_ACL_SD_SRC, is_src) | KSZ9477_ACL_EQ_EQUAL;
> + ksz9477_acl_set_reg(entry, KSZ9477_ACL_PORT_ACCESS_1, val);
> +
> + if (eth_addr) {
> + int i;
> +
> + for (i = 0; i < ETH_ALEN; i++) {
> + ksz9477_acl_set_reg(entry,
> + KSZ9477_ACL_PORT_ACCESS_2 + i,
> + eth_addr[i]);
> + }
> + }
> +
> + ksz9477_acl_set_reg(entry, KSZ9477_ACL_PORT_ACCESS_8, ethertype & 0xff);
> + ksz9477_acl_set_reg(entry, KSZ9477_ACL_PORT_ACCESS_9, ethertype >> 8);
ethertype is big endian.
But host byte order math is done on it in the two lines above.
This doesn't seem right.
...
> +/**
> + * ksz9477_flower_parse_action - Parse flow rule actions for a specified port
> + * on a ksz_device.
> + * @dev: The ksz_device instance.
> + * @port: The port number to parse the flow rule actions for.
> + * @extack: The netlink extended ACK for reporting errors.
> + * @cls: The flow_cls_offload instance containing the flow rule.
> + * @entry_idx: The index of the ACL entry to store the action.
> + *
> + * This function checks if the actions in the flow rule are supported by
> + * the device. Currently, only actions that change priorities are supported.
> + * If unsupported actions are encountered, an error message is set in the
> + * extended ACK.
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +static int ksz9477_flower_parse_action(struct ksz_device *dev, int port,
> + struct netlink_ext_ack *extack,
> + struct flow_cls_offload *cls,
> + int entry_idx)
> +{
> + struct flow_rule *rule = flow_cls_offload_flow_rule(cls);
> + struct ksz9477_acl_priv *acl = dev->ports[port].acl_priv;
> + const struct flow_action_entry *act;
> + struct ksz9477_acl_entry *entry;
> + bool prio_force = false;
> + u8 prio_val;
> + int i;
> +
> + if (TC_H_MIN(cls->classid)) {
> + NL_SET_ERR_MSG_MOD(extack, "hw_tc is not supported. Use: action skbedit prio");
> + return -EOPNOTSUPP;
> + }
> +
> + flow_action_for_each(i, act, &rule->action) {
> + switch (act->id) {
> + case FLOW_ACTION_PRIORITY:
> + if (act->priority > KSZ9477_MAX_TC) {
> + NL_SET_ERR_MSG_MOD(extack, "Priority value is too high");
> + return -EOPNOTSUPP;
> + }
> + prio_force = true;
> + prio_val = act->priority;
> + break;
> + default:
> + NL_SET_ERR_MSG_MOD(extack, "action not supported");
> + return -EOPNOTSUPP;
> + }
> + }
> +
> + /* pick entry to store action */
> + entry = &acl->acles.entries[entry_idx];
> +
> + ksz9477_acl_action_rule_cfg(entry->entry, prio_force, prio_val);
If the flow_action_for_each loop iterates zero times then
prio_val will be uninitialised here. gcc-12 [-Wmaybe-uninitialized]
and Smatch warn about this. But I'm unsure if it can actually happen.
> + ksz9477_acl_processing_rule_set_action(entry->entry, entry_idx);
> +
> + return 0;
> +}
...
Powered by blists - more mailing lists