netdev - Re: [PATCH net-next 04/12] gtp: add IPv6 support

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

Message-ID: <ZjNtDEJFDgSjWanp@calendula>
Date: Thu, 2 May 2024 12:38:04 +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 04/12] gtp: add IPv6 support

Hi Simon,

On Fri, Apr 26, 2024 at 09:41:01PM +0100, Simon Horman wrote:
> On Thu, Apr 25, 2024 at 12:51:30PM +0200, Pablo Neira Ayuso wrote:
[...]
> > @@ -131,6 +134,11 @@ static inline u32 ipv4_hashfn(__be32 ip)
> >  	return jhash_1word((__force u32)ip, gtp_h_initval);
> >  }
> >  
> > +static inline u32 ipv6_hashfn(const struct in6_addr *ip6)
> > +{
> > +	return jhash(ip6, sizeof(*ip6), gtp_h_initval);
> > +}
> > +
> 
> Hi Pablo,
> 
> I'm would naively expect that the compiler can work out if this needs to
> be inline.

I will remove inline, I saw the warnings from patchwork on this too
after my v2.

> >  /* Resolve a PDP context structure based on the 64bit TID. */
> >  static struct pdp_ctx *gtp0_pdp_find(struct gtp_dev *gtp, u64 tid)
> >  {
> 
> ...
> 
> > @@ -878,6 +951,20 @@ static inline void gtp_set_pktinfo_ipv4(struct gtp_pktinfo *pktinfo,
> >  	pktinfo->dev	= dev;
> >  }
> >  
> > +static inline void gtp_set_pktinfo_ipv6(struct gtp_pktinfo *pktinfo,
> > +					struct sock *sk, struct ipv6hdr *ip6h,
> > +					struct pdp_ctx *pctx, struct rt6_info *rt6,
> > +					struct flowi6 *fl6,
> > +					struct net_device *dev)
> > +{
> > +	pktinfo->sk	= sk;
> > +	pktinfo->ip6h	= ip6h;
> > +	pktinfo->pctx	= pctx;
> > +	pktinfo->rt6	= rt6;
> > +	pktinfo->fl6	= *fl6;
> > +	pktinfo->dev	= dev;
> > +}
> 
> Here too.

OK.

> > @@ -1441,7 +1736,14 @@ static struct pdp_ctx *gtp_pdp_add(struct gtp_dev *gtp, struct sock *sk,
> >  		if (!pctx)
> >  			pctx = pctx_tid;
> >  
> > -		ipv4_pdp_fill(pctx, info);
> > +		switch (pctx->af) {
> > +		case AF_INET:
> > +			ipv4_pdp_fill(pctx, info);
> > +			break;
> > +		case AF_INET6:
> > +			ipv6_pdp_fill(pctx, info);
> > +			break;
> > +		}
> >  
> >  		if (pctx->gtp_version == GTP_V0)
> >  			netdev_dbg(dev, "GTPv0-U: update tunnel id = %llx (pdp %p)\n",
> 
> The code just before the following hunk is:
> 
> 	pctx = kmalloc(sizeof(*pctx), GFP_ATOMIC);
> 	if (pctx == NULL)
> 		return ERR_PTR(-ENOMEM);
> 
> 
> > @@ -1461,7 +1763,24 @@ static struct pdp_ctx *gtp_pdp_add(struct gtp_dev *gtp, struct sock *sk,
> >  	sock_hold(sk);
> >  	pctx->sk = sk;
> >  	pctx->dev = gtp->dev;
> > -	ipv4_pdp_fill(pctx, info);
> > +	pctx->af = family;
> > +
> > +	switch (pctx->af) {
> > +	case AF_INET:
> > +		if (!info->attrs[GTPA_MS_ADDRESS] ||
> > +		    !info->attrs[GTPA_PEER_ADDRESS])
> > +			return ERR_PTR(-EINVAL);
> 
> So this appears to leak pctx.

Good catch.

> > +
> > +		ipv4_pdp_fill(pctx, info);
> > +		break;
> > +	case AF_INET6:
> > +		if (!info->attrs[GTPA_MS_ADDR6] ||
> > +		    !info->attrs[GTPA_PEER_ADDR6])
> > +			return ERR_PTR(-EINVAL);
> 
> Likewise here.
> 
> Flagged by Smatch.

Thanks Simon.