On 2026/11/03 22:57, Philippe Mathieu-Daudé wrote:
On 2026-11-03 14:24, Peter Maydell wrote:
On Tue, 3 Nov 2026 at 11:51, Philippe Mathieu-Daudé
<[email protected]> wrote:
On 2026-11-02 21:18, Peter Maydell wrote:
eth_get_protocols() parses an ethernet packet and tells the caller
about various offsets into it and other properties of the packet.
For the L4 protocol, in both the IP_PROTO_TCP and IP_PROTO_UDP
branches we implicitly check that the packet does contain a whole TCP
or UDP header at the l4hdr_off offset, because _eth_copy_chunk() will
fail if the input packet size isn't big enough for the whole header
we are copying out of it. But for IP_PROTO_SCTP we don't copy the
SCTP common header. In this case we should still do the length
check, so we don't set l4hdr_info->proto to say there's an SCTP
header present when there isn't.
This shouldn't have any visible effects if the downstream consumers
of the l4hdr_info are conservative about checking before they access
into the packet, but checking here too adds defence in depth, hence
the cc to stable.
Cc: [email protected]
Fixes: 907209e311 ("igb: Implement Rx SCTP CSO")
Signed-off-by: Peter Maydell <[email protected]>
---
include/net/eth.h | 2 ++
net/eth.c | 9 ++++++++-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/include/net/eth.h b/include/net/eth.h
index 46dc9ed5acd..e10684ed923 100644
--- a/include/net/eth.h
+++ b/include/net/eth.h
@@ -394,6 +394,8 @@ typedef struct eth_l4_hdr_info_st {
bool has_tcp_data;
} eth_l4_hdr_info;
+#define SCTP_COMMON_HEADER_LEN 12
+
void eth_get_protocols(const struct iovec *iov, size_t iovcnt,
size_t iovoff,
bool *hasip4, bool *hasip6,
size_t *l3hdr_off,
diff --git a/net/eth.c b/net/eth.c
index bc26d38dc21..341d3bdf02b 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -213,7 +213,14 @@ void eth_get_protocols(const struct iovec *iov,
size_t iovcnt, size_t iovoff,
break;
case IP_PROTO_SCTP:
- l4hdr_info->proto = ETH_L4_HDR_PROTO_SCTP;
+ /*
+ * We don't copy the SCTP common header into l4hdr_info, but
+ * don't report SCTP as present if the packet isn't big enough
+ * to actually have an SCTP header.
+ */
+ if (*l4hdr_off > input_size - SCTP_COMMON_HEADER_LEN) {
Hmm don't we want the opposite "is SCTP common header complete" check?
if (input_size >= *l4hdr_off
&& input_size - *l4hdr_off >= SCTP_COMMON_HEADER_LEN) {
Whoops, yes, I have the sense of the test the wrong way around,
should be
(*l4hdr_off <= input_size - SCTP_COMMON_HEADER_LEN)
Doing it this way we know we aren't going to underflow on
the subtraction (input_size must be bigger than 12 if we got here)
so we don't need to explictly also check for
input_size >= *l4hdr_off, but maybe that's being too
clever and your way of writing it is better?
Ah indeed I didn't read the full function body, we already have
a complete IP header so we are sure input_size >= 12. Let's go with
your way otherwise static analyzers might complain later about
impossible check.>
Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
With [1], I think the following would be better:
input_size - *l4hdr_off <= SCTP_COMMON_HEADER_LEN
input_size >= l4hdr_off is already enforced for IPv6, but not for IPv4.
[1] enforces that for IPv4 since failing to do so is inconsistent and
led to an out-of-bounds access [2].
SCTP header length and IP header lengths are independent values, and we
can avoid making assumptions between them this way.
Regards,
Akihiko Odaki
[1]
https://lore.kernel.org/qemu-devel/[email protected]/
[2] https://gitlab.com/qemu-project/qemu/-/work_items/4155
