Re: [PATCH net-next v7 4/5] net: dsa: microchip: Support Microchip KSZ8995XA / KS8995XA

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:

New issues:
- [Critical] Uninitialized Stack Variables Corrupt Hardware Registers
- [Critical] MAC Header Corruption on Receive via Misuse of skb_vlan_untag
- [High] Incorrect Register Polarity for MTU Configuration on KSZ8995XA
- [High] Loss of VLAN Tags and Broken 802.1AD Routing on Egress
- [High] Hardware PVID Leaks into Host Network Stack for Untagged Packets
- [High] Invalid PHY Soft Reset on CPU Port Targets Reserved/MAC Registers

Pre-existing issues:
- [High] SPI Device Tree Flags Discarded by Unconditional Mode Overwrite
--

commit 0676ac96adf5e00a23c99f1617beebb88d95ead0
Author: Linus Walleij <linusw@kernel.org>
Subject: net: dsa: microchip: Support Microchip KSZ8995XA / KS8995XA

This adds support for the Microchip KSZ8995XA also known as the Micrel
KS8995XA switch to the KSZ driver. It includes specific register
definitions, switch operations, and compatibility configurations.

> diff --git a/drivers/net/dsa/microchip/Kconfig b/drivers/net/dsa/microchip/Kconfig
> index c71d3fd5dfeb8..75c9b2114afd3 100644
> --- a/drivers/net/dsa/microchip/Kconfig
> +++ b/drivers/net/dsa/microchip/Kconfig
> @@ -2,6 +2,7 @@
>  menuconfig NET_DSA_MICROCHIP_KSZ_COMMON
>  	tristate "Microchip KSZ8XXX/KSZ9XXX/LAN937X series switch support"
>  	depends on NET_DSA
> +	select NET_DSA_TAG_KS8995

[Severity: Critical]
Looking at ks8995_rcv() in net/dsa/tag_ks8995.c which this config enables,
it calls skb_vlan_untag(skb). At this point, the DSA core has pulled the
Ethernet MAC header, so skb->data points to the MAC destination address,
not the VLAN header.

Does this unconditionally read the MAC destination address as a VLAN header,
extract a garbage TCI, and pull 4 bytes, stripping half the MAC address?

[Severity: High]
Also looking at ks8995_xmit() in net/dsa/tag_ks8995.c, if the packet has an
802.1Q payload tag, the driver overwrites its TPID with the DSA routing tag
via hdr->h_vlan_proto = KS8995M_STPID(portmask).

Since the switch is configured with PORT_REMOVE_TAG on external ports, will
this cause it to strip this single tag on egress and transmit the packet
untagged, resulting in a loss of 802.1Q VLAN tags?

[ ... ]

> diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
> index 586916570a841..03a5981a6315c 100644
> --- a/drivers/net/dsa/microchip/ksz8.c
> +++ b/drivers/net/dsa/microchip/ksz8.c
> @@ -137,7 +138,17 @@ static int ksz8_pme_pwrite8(struct ksz_device *dev, int port, int offset, u8 dat
>  
>  static int ksz8_reset_switch(struct ksz_device *dev)
>  {
> -	if (ksz_is_ksz88x3(dev)) {
> +	if (ksz_is_ksz8995xa(dev)) {
> +		unsigned int port;
> +
> +		/* The KSZ8995XA switch itself cannot be reset by software, but
> +		 * often boot loaders have fun with ports, so reset them. This
> +		 * reset bit is self-clearing.
> +		 */
> +		for (port = 0; port < dev->info->port_cnt; port++)
> +			ksz_port_cfg(dev, port, REG_PORT_STATUS_3,
> +				     PORT_PHY_SOFT_RESET, true);

[Severity: High]
This loop in ksz8_reset_switch() iterates over all ports, including the
CPU port which lacks an internal PHY. Could writing PORT_PHY_SOFT_RESET to
REG_PORT_STATUS_3 (0x0F) on the CPU port target reserved or MAC control
registers and cause hardware initialization failures?

> +	} else if (ksz_is_ksz88x3(dev)) {
>  		/* reset switch */
>  		ksz_cfg(dev, KSZ8863_REG_SW_RESET,
>  			KSZ8863_GLOBAL_SOFTWARE_RESET | KSZ8863_PCS_RESET, true);

[ ... ]

> @@ -161,8 +172,14 @@ static int ksz8_reset_switch(struct ksz_device *dev)
>  static int ksz8863_change_mtu(struct ksz_device *dev, int frame_size)
>  {
>  	u8 ctrl2 = 0;
> +	unsigned int legalsz;
> +
> +	if (ksz_is_ksz8995xa(dev))
> +		legalsz = KSZ8995XA_LEGAL_PACKET_SIZE;
> +	else
> +		legalsz = KSZ8_LEGAL_PACKET_SIZE;
>  
> -	if (frame_size <= KSZ8_LEGAL_PACKET_SIZE)
> +	if (frame_size <= legalsz)
>  		ctrl2 |= KSZ8863_LEGAL_PACKET_ENABLE;

[Severity: High]
Does ksz8863_change_mtu() apply inverted BIT(1) logic for MTU limits on the
KSZ8995XA?

For small MTUs 1522 bytes or less, the driver sets BIT(1) of REG_SW_CTRL_2.
On the KSZ8863 this enables the size limit, but on KSZ8995XA, BIT(1)
represents SW_LEGAL_PACKET_DISABLE and disables the limit. Could this cause
valid packets to be dropped while huge packets bypass the size limit?

[ ... ]

> @@ -242,6 +260,10 @@ static int ksz8_port_queue_split(struct ksz_device *dev, int port, int queues)
>  		mask_2q = KSZ8873_PORT_2QUEUE_SPLIT_EN;
>  		reg_4q = P1CR1;
>  		reg_2q = P1CR1 + 1;
> +	} else if (ksz_is_ksz8995xa(dev)) {
> +		/* This switch has no 4way split support */
> +		mask_2q = KSZ8795_PORT_2QUEUE_SPLIT_EN;
> +		reg_2q = REG_PORT_CTRL_0;
>  	} else {
>  		mask_4q = KSZ8795_PORT_4QUEUE_SPLIT_EN;
>  		mask_2q = KSZ8795_PORT_2QUEUE_SPLIT_EN;

[Severity: Critical]
When ksz_is_ksz8995xa(dev) is true in ksz8_port_queue_split(), do we use
uninitialized reg_4q and mask_4q variables?

The function unconditionally passes them to ksz_prmw8() later on. Could this
perform a read-modify-write on a garbage I/O register address using a
garbage mask?

[ ... ]

> @@ -2061,13 +2126,17 @@ static int ksz8_setup(struct dsa_switch *ds)
>  	struct ksz_port *p;
>  	const u16 *regs;
>  	int i, ret;
> +	u8 val;
>  
>  	regs = dev->info->regs;
>  
> -	dev->vlan_cache = devm_kcalloc(dev->dev, sizeof(struct vlan_table),
> -				       dev->info->num_vlans, GFP_KERNEL);
> -	if (!dev->vlan_cache)
> -		return -ENOMEM;
> +	/* KSZ8995XA has no SW controlled VLAN handling */
> +	if (!ksz_is_ksz8995xa(dev)) {
> +		dev->vlan_cache = devm_kcalloc(dev->dev, sizeof(struct vlan_table),
> +					       dev->info->num_vlans, GFP_KERNEL);
> +		if (!dev->vlan_cache)
> +			return -ENOMEM;
> +	}

[Severity: High]
By skipping VLAN initialization here in ksz8_setup(), the hardware PVID is
never cleared to 0.

In net/dsa/tag_ks8995.c, ks8995_rcv() uses a check for !skb->vlan_tci to
identify originally untagged packets. Does this mean the switch will insert
its non-zero hardware default PVID into untagged packets, causing them to be
erroneously presented to the network stack as 802.1Q VLAN tagged packets?

[ ... ]

> diff --git a/drivers/net/dsa/microchip/ksz_spi.c b/drivers/net/dsa/microchip/ksz_spi.c
> index 77aecac324667..2c55014246e5f 100644
> --- a/drivers/net/dsa/microchip/ksz_spi.c
> +++ b/drivers/net/dsa/microchip/ksz_spi.c
> @@ -185,7 +186,10 @@ static int ksz_spi_probe(struct spi_device *spi)
>  		dev->pdata = spi->dev.platform_data;
>  
>  	/* setup spi */
> -	spi->mode = SPI_MODE_3;
> +	if (chip->chip_id == KSZ8995XA_CHIP_ID)
> +		spi->mode = SPI_MODE_0;
> +	else
> +		spi->mode = SPI_MODE_3;
>  	ret = spi_setup(spi);
>  	if (ret)
>  		return ret;

[Severity: High]
This is a pre-existing issue, but by unconditionally assigning spi->mode
here in ksz_spi_probe(), does it discard all SPI bus flags configured by
the SPI subsystem based on Device Tree properties (such as spi-cs-high
or spi-3wire)?

This could break SPI communication on boards requiring non-default
polarities or phases.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260704-ks8995-to-ksz8-v7-0-2af0eaa545a8@kernel.org?part=4