Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 (3) · Low: 2
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] net: ibmveth: rx_csum_helper includes Ethernet padding in TCP
pseudo-header
- [Low] The kernel-doc for ibmveth_rx_mss_helper_test()
(drivers/net/ethernet/ibm/ibmveth.c:2597) says that 'Frames whose…
- [Low] The new comment at the bad_hdr label in ibmveth_rx_csum_helper()
(drivers/net/ethernet/ibm/ibmveth.c:1548-1550) says '/* headers do not…
Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the new code
makes it visible.
- [Medium] This problem wasn't introduced by this patch, but the patch
touches it.
- [Medium] This problem wasn't introduced by this patch.
--
ibmveth: check headers in the RX large send and checksum helpers
This patch adds header length, IPv4 ihl, TCP doff, IP length and fragment
checks to ibmveth_rx_mss_helper() and ibmveth_rx_csum_helper(). Frames
that fail a check are passed up without GSO fields or
CHECKSUM_UNNECESSARY. It also clamps gso_size, computes gso_segs from the
IP length, and adds KUnit cases for both helpers.
> diff --git a/drivers/net/ethernet/ibm/ibmveth.c
> b/drivers/net/ethernet/ibm/ibmveth.c
> index 85e176d0ed321..e9874dfedf378 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c
[ ... ]
> @@ -1436,11 +1453,18 @@ static void ibmveth_rx_mss_helper(struct sk_buff
> *skb, u16 mss, int lrg_pkt)
> }
>
> if (skb_shinfo(skb)->gso_size) {
> + /* chosen by the sending partition: at least the floor
> + * virtio_net_hdr_to_skb() uses, and never GSO_BY_FRAGS
> + */
> + skb_shinfo(skb)->gso_size =
> + clamp_t(u16, skb_shinfo(skb)->gso_size,
> + TCP_MIN_GSO_SIZE, GSO_BY_FRAGS - 1);
> hdr_len = offset + tcph->doff * 4;
> skb_shinfo(skb)->gso_segs =
> - DIV_ROUND_UP(skb->len - hdr_len,
> + DIV_ROUND_UP(ip_len - hdr_len,
> skb_shinfo(skb)->gso_size);
> }
> + return true;
> }
[Severity: Medium]
This isn't a bug introduced by this patch, but the frame is still marked
GSO whenever gso_size is nonzero, even if the TCP payload is no larger
than gso_size. Should there also be a check that the payload is larger
than gso_size?
The new comment points to __virtio_net_hdr_to_skb() as the model for the
clamp. That function only sets GSO after this check:
include/linux/virtio_net.h:__virtio_net_hdr_to_skb() {
...
/* Too small packets are not really GSO ones. */
if (skb->len - nh_off > gso_size) {
shinfo->gso_size = gso_size;
shinfo->gso_type = gso_type;
...
}
Without that check, ibmveth can build a GSO skb that is not
SKB_GSO_DODGY, with gso_segs == 1 and a payload no larger than gso_size.
If a forwarded or bridged frame then needs software segmentation,
tcp_gso_segment() rejects it:
net/ipv4/tcp_offload.c:tcp_gso_segment() {
...
mss = skb_shinfo(skb)->gso_size;
if (unlikely(skb->len <= mss))
goto out;
...
}
validate_xmit_skb() then drops the packet.
Three cases can reach this:
- a large send whose MSS is at least the payload
- the path where length > mtu + ETH_HLEN and gso_size is read from a
real TCP checksum
- new with this patch, an MSS of 1 to 7 raised to TCP_MIN_GSO_SIZE when
the payload is 8 bytes or less
The new KUnit case ibmveth_rx_mss_helper(skb, GSO_BY_FRAGS, 1) also
expects a 2896 byte payload with gso_size 65534 to be accepted as a large
send.
[ ... ]
> @@ -1493,6 +1523,8 @@ static void ibmveth_rx_csum_helper(struct sk_buff *skb,
> if (iph_proto == IPPROTO_TCP) {
> struct tcphdr *tcph = (struct tcphdr *)(skb->data + iphlen);
>
> + if (skb_headlen(skb) < iphlen + sizeof(*tcph))
> + goto bad_hdr;
> if (tcph->check == 0x0000) {
> /* Recompute TCP pseudo header checksum */
> tcphdrlen = skb->len - iphlen;
[Severity: Medium]
This is a pre-existing issue, but the pseudo-header seed below still
uses skb->len, and skb->len includes any Ethernet padding.
ibmveth_poll() does not trim the frame to the IP length before it
calls ibmveth_rx_csum_helper().
Take a padded IPv4 ACK with tot_len 40. It arrives with skb->len 46,
so tcphdrlen becomes 26 instead of 20 when the seed is built with
csum_tcpudp_magic(). The same happens on the IPv6 side with
csum_ipv6_magic().
The helper then marks the frame CHECKSUM_PARTIAL through
skb_partial_csum_set(). Suppose the frame is bridged, or routed out of
a device with checksum offload. The egress NIC adds the payload sum to
this seed. The final receiver trims the padding and verifies against
the real TCP length, so the checksum does not match and the segment is
dropped.
Pure ACKs are the frames most likely to be padded, so a forwarded TCP
connection could stall.
On the routed path, ip_rcv_core() does trim the padding. But
pskb_trim_rcsum_slow() only checks where the checksum field sits for
CHECKSUM_PARTIAL, and it does not correct the seed. Local delivery is
not affected.
The IP length check added to ibmveth_rx_mss_helper() does not help
here. A frame under the MTU that is not a large send never reaches that
helper.
Should tcphdrlen come from tot_len - iphlen for IPv4 and from
payload_len for IPv6 instead? And should a frame whose IP length
exceeds skb_headlen() go to bad_hdr?
ibmveth_start_xmit() does not pad, so this needs a peer that uses CSO
and pads its frames.
[Severity: Medium]
This is a pre-existing issue, but ibmveth_rx_mss_helper() now refuses
frames with bytes past the IP length. Should the pseudo-header length
here also come from the IP length instead of skb->len?
tcphdrlen ignores the IPv4 tot_len and the IPv6 payload_len. The helper
then marks the frame CHECKSUM_PARTIAL with skb_partial_csum_set().
Take an IPv4 ACK with csum_good set, tcph->check == 0, tot_len 40 and
skb->len 46 because of Ethernet padding. It is under the MTU and is not a
large send. So ibmveth_poll() skips ibmveth_rx_mss_helper() and calls
ibmveth_rx_csum_helper() directly, and the seed uses a TCP length of 26
instead of 20.
If the frame is bridged, the egress device includes the padding in the
checksum.
If it is routed, ip_rcv_core() trims the padding through
pskb_trim_rcsum_slow(). For CHECKSUM_PARTIAL, that function only checks
where the checksum field sits. It does not correct the seed.
Wouldn't the frame go out with a bad TCP checksum in both cases? Local
delivery is not affected.
Linux ibmveth_start_xmit() does not pad, so this needs a peer that uses
CSO and pads its frames.
> @@ -1510,6 +1542,13 @@ static void ibmveth_rx_csum_helper(struct sk_buff *skb,
> skb_reset_network_header(skb);
> }
> }
[Severity: Medium]
This isn't a bug introduced by this patch, but the return value of
skb_partial_csum_set() just above is ignored. By that point,
tcph->check already holds the pseudo-header seed:
/* Setup SKB fields for checksum offload */
skb_partial_csum_set(skb, iphlen,
offsetof(struct tcphdr, check));
skb_reset_network_header(skb);
skb_partial_csum_set() returns false without touching ip_summed when
skb_headroom(skb) + start >= U16_MAX. Its kernel-doc says the packet
should be dropped in that case.
Here the frame would keep the CHECKSUM_UNNECESSARY that ibmveth_poll()
set, with only the seed in its TCP checksum field. Could a forwarded
frame go out with that seed as its checksum?
This needs about 65.5 KB of headroom, for example a 128 KiB pool buffer
with the frame at an offset of about 65 KB. The offset comes from the
hypervisor. The new header checks do not compare the headroom with
iphlen.
> + return;
> +
> +bad_hdr:
> + /* headers do not fit the frame, or a fragment has no whole TCP
> + * segment; let the stack check it
> + */
> + skb->ip_summed = CHECKSUM_NONE;
> }
[Severity: Low]
This comment says "let the stack check it", but the v5 changelog says:
message and comment no longer say a refused frame is checked by the
stack; bridged or switched frames are not
Should this comment be updated to match? Setting CHECKSUM_NONE only leads
to verification on local delivery. Bridged, OVS-switched and forwarded
frames are not checked.
The comment also gives only two reasons for reaching bad_hdr. It leaves
out the iph->ihl < 5 check, which also jumps here.
[ ... ]
> @@ -2508,10 +2549,237 @@ static void ibmveth_rxq_harvest_buffer_test(struct
> kunit *test)
[ ... ]
> +/**
> + * ibmveth_rx_mss_helper_test - unit test for ibmveth_rx_mss_helper
> + * @test: pointer to kunit structure
> + *
> + * Large send frames get gso_size from the descriptor or the TCP checksum
> + * field, clamped, and gso_segs from the IP length. Frames whose headers
> + * do not fit, whose IP length is not the frame length, with no TCP
> + * payload within the IP length, or that are IPv4 fragments, are refused
> + * and left without GSO.
> + *
> + * Return: void
> + */
> +static void ibmveth_rx_mss_helper_test(struct kunit *test)
[Severity: Low]
This isn't a bug, but the kernel-doc says frames whose headers do not fit
are refused. No case in ibmveth_rx_mss_helper_test() builds a frame
shorter than the IPv4 header, the IPv6 header, or offset +
sizeof(struct tcphdr).
Every fixture comes from ibmveth_test_tcp_skb() with complete headers.
The skb_trim() cases are only in ibmveth_rx_csum_helper_test().
The ihl 0 case is refused by iph->ihl < 5. The tot_len + 1, padding and
zero payload cases are refused by the ip_len checks.
So the skb_headlen() parts of these checks in ibmveth_rx_mss_helper()
could be removed without any case failing:
if (skb_headlen(skb) < sizeof(*iph) || iph->ihl < 5)
if (skb_headlen(skb) < sizeof(*iph6))
if (skb_headlen(skb) < offset + sizeof(*tcph) || tcph->doff < 5 ||
The commit message's list of tested checks correctly leaves these out.
Should truncated-frame cases be added, or the kernel-doc narrowed?
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013182926.30417-1-mmc%40linux.ibm.com
