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

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZjNsnb-PEM0Hnie1@calendula>
Date: Thu, 2 May 2024 12:36:13 +0200
From: Pablo Neira Ayuso <pablo@...filter.org>
To: Simon Horman <horms@...nel.org>
Cc: netdev@...r.kernel.org, davem@...emloft.net, laforge@...ocom.org,
	pespin@...mocom.de, osmith@...mocom.de, kuba@...nel.org,
	pabeni@...hat.com, edumazet@...gle.com, fw@...len.de
Subject: Re: [PATCH net-next 02/12] gtp: properly parse extension headers

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@...filter.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.