netdev - Re: [net-next,v8] net: ethernet: rtsn: Add support for Renesas Ethernet-TSN

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]

Date: Mon, 17 Jun 2024 19:20:41 +0200
From: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
To: Paul Barker <paul.barker.ct@...renesas.com>
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
	linux-renesas-soc@...r.kernel.org
Subject: Re: [net-next,v8] net: ethernet: rtsn: Add support for Renesas
 Ethernet-TSN

Hi Paul,

On 2024-06-17 14:41:52 +0100, Paul Barker wrote:
<snip>
> >>> +	entry = priv->cur_tx % priv->num_tx_ring;
> >>> +	priv->tx_skb[entry] = skb;
> >>> +	desc = &priv->tx_ring[entry];
> >>> +	desc->dptr = cpu_to_le32(dma_addr);
> >>> +	desc->info_ds = cpu_to_le16(skb->len);
> >>
> >> Should we check against the maximum TX frame size supported by the
> >> hardware here?
> >>
> >> Whatever we do here, we should also do in the ravb driver as that makes
> >> a similar cpu_to_le16() call to fill the DS field with no check that the
> >> HW actually supports transmitting a frame of the given size.
> > 
> > Compared to RAVB the RTSN driver do not support splitting a packet over 
> > multiple descriptors, so the max frame size adhering to the MTU will 
> > always fit using a single descriptor.
> > 
> 
> My concern here is with pathological or malicious packets.

A malicious Tx packet due to local VLAN setup? There must be easier ways 
to crash a machine if you got the permissions to configure interface 
VLAN settings :-) But I agree the user shall be protected from
misconfiguration.

> For example,
> you can use stacked VLANS (QinQ, QinQinQ, etc) to expand the size of the
> TX frame for a given MTU since the bytes used by the extra VLAN tags are
> not counted as payload bytes.

This is interesting, I only played with single and double tagging never 
QinQinQ.. . For some reason I assumed that after double tagging space 
where going to be consumed from the payload. But indeed setting up 4 
levels of VLAN tags shows the payload can be force to stay the same and 
the skb->len do indeed grow.

  $ ip link add link end0 name end0.2 type vlan id 2
  $ ifconfig end0.2 10.0.2.10 netmask 255.255.255.0 up
  $ ip link add link end0.2 name end0.3 type vlan id 3
  $ ifconfig end0.3 10.0.3.10 netmask 255.255.255.0 up
  $ ip link add link end0.3 name end0.4 type vlan id 4
  $ ifconfig end0.4 10.0.4.10 netmask 255.255.255.0 up
  $ ip link add link end0.4 name end0.5 type vlan id 5
  $ ifconfig end0.5 10.0.5.10 netmask 255.255.255.0 up
  $ ping -s 1500 10.0.5.1  # Give an skb->len of 1530
  $ ping -s 1500 10.0.4.1  # Give an skb->len of 1526
  $ ping -s 1500 10.0.3.1  # Give an skb->len of 1522
  $ ping -s 1500 10.0.2.1  # Give an skb->len of 1518
  $ ping -s 1500 10.0.1.1  # Give an skb->len of 1514 (no tags)

The above was produced using RAVB and not RTSN and similar VLANS on a 
2nd device was setup to allow for ICMP traffic and replies.

> 
> At least with the RZ/G2L family, no verification has been performed on
> sending packets larger than 1526 bytes to my knowledge. Even using only
> one TX descriptor, I was able to completely lock up the GbEth IP by
> pushing a 2kB frame into the TX queue. So I do think it is worth adding
> some checks here.

I will add a check for this to RTSN.

-- 
Kind Regards,
Niklas Söderlund