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? -- PMM
