Re: [PATCH net-next 02/12] gtp: properly parse extension headers

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Fri, Apr 26, 2024 at 09:28:52PM +0100, Simon Horman wrote:
> On Thu, Apr 25, 2024 at 12:51:28PM +0200, Pablo Neira Ayuso wrote:
> > Currently GTP packets are dropped if the next extension field is set to
> > non-zero value, but this are valid GTP packets.
> > 
> > TS 29.281 provides a longer header format, which is defined as struct
> > gtp1_header_long. Such long header format is used if any of the S, PN, E
> > flags is set.
> > 
> > This long header is 4 bytes longer than struct gtp1_header, plus
> > variable length (optional) extension headers. The next extension header
> > field is zero is no extension header is provided.
> > 
> > The extension header is composed of a length field which includes total
> > number of 4 byte words including the extension header itself (1 byte),
> > payload (variable length) and next type (1 byte). The extension header
> > size and its payload is aligned to 4 bytes.
> > 
> > A GTP packet might come with a chain extensions headers, which makes it
> > slightly cumbersome to parse because the extension next header field
> > comes at the end of the extension header, and there is a need to check
> > if this field becomes zero to stop the extension header parser.
> > 
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> >  drivers/net/gtp.c | 41 +++++++++++++++++++++++++++++++++++++++++
> >  include/net/gtp.h |  5 +++++
> >  2 files changed, 46 insertions(+)
> > 
> > diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
> > index 4680cdf4fa70..9451c74c1a7d 100644
> > --- a/drivers/net/gtp.c
> > +++ b/drivers/net/gtp.c
> > @@ -567,6 +567,43 @@ static int gtp1u_handle_echo_resp(struct gtp_dev *gtp, struct sk_buff *skb)
> >  				       msg, 0, GTP_GENL_MCGRP, GFP_ATOMIC);
> >  }
> >  
> > +static int gtp_parse_exthdrs(struct sk_buff *skb, unsigned int *hdrlen)
> > +{
> > +	struct gtp_ext_hdr *gtp_exthdr, _gtp_exthdr;
> > +	unsigned int offset = *hdrlen;
> > +	__u8 *next_type, _next_type;
> > +
> > +	/* From 29.060: "The Extension Header Length field specifies the length
> > +	 * of the particular Extension header in 4 octets units."
> > +	 *
> > +	 * This length field includes length field size itself (1 byte),
> > +	 * payload (variable length) and next type (1 byte). The extension
> > +	 * header is aligned to to 4 bytes.
> > +	 */
> > +
> > +	do {
> > +		gtp_exthdr = skb_header_pointer(skb, offset, sizeof(gtp_exthdr),
> 
> Hi Pablo,
> 
> Should this be sizeof(*gtp_exthdr)?

Indeed, coincidentally, extension header size if 4 bytes, then this is
checking 8 bytes on x86_64 and 4 bytes in x86.

> And likewise, in the ip_version calculation in gtp_inner_proto()
> in [PATCH 11/12] gtp: support for IPv4-in-IPv6-GTP and IPv6-in-IPv4-GTP 
> 
> Flagged by Coccinelle.

Thanks; I will fix and revamp.