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
| ||
|
Message-ID: <ZWJ5vZbbf2YaO6xN@Antony2201.local> Date: Sat, 25 Nov 2023 23:48:29 +0100 From: Antony Antony <antony@...nome.org> To: Steffen Klassert <steffen.klassert@...unet.com> Cc: Antony Antony <antony.antony@...unet.com>, Herbert Xu <herbert@...dor.apana.org.au>, "David S. Miller" <davem@...emloft.net>, devel@...ux-ipsec.org, Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org Subject: Re: [devel-ipsec] [PATCH v2 ipsec-next 1/2] xfrm: introduce forwarding of ICMP Error messages On Fri, Nov 17, 2023 at 10:13:15AM +0100, Steffen Klassert via Devel wrote: > On Fri, Oct 27, 2023 at 10:16:29AM +0200, Antony Antony wrote: > > + > > +static struct sk_buff *xfrm_icmp_flow_decode(struct sk_buff *skb, > > + unsigned short family, > > + struct flowi *fl, > > + struct flowi *fl1) > > +{ > > + struct net *net = dev_net(skb->dev); > > + struct sk_buff *newskb = skb_clone(skb, GFP_ATOMIC); > > + int hl = family == AF_INET ? (sizeof(struct iphdr) + sizeof(struct icmphdr)) : > > + (sizeof(struct ipv6hdr) + sizeof(struct icmp6hdr)); > > + skb_reset_network_header(newskb); > > This is not needed there. fixed. > > > + > > + if (!pskb_pull(newskb, hl)) > > + return NULL; > > This leaks newskb. fixedd > > > + skb_reset_network_header(newskb); > > + > > + if (xfrm_decode_session_reverse(net, newskb, fl1, family) < 0) { > > + kfree_skb(newskb); > > The newskb is not dropped because of an error, maybe better use > consume_skb(). good idea. I will change to consume_skb(). > > > + XFRM_INC_STATS(net, LINUX_MIB_XFRMINHDRERROR); > > This might bump a second stat counter for this packet. Also > this is not really an error, because you just can't parse > the payload of the icmp packet. yes fixed > > > + return NULL; > > + } > > + > > + fl1->flowi_oif = fl->flowi_oif; > > + fl1->flowi_mark = fl->flowi_mark; > > + fl1->flowi_tos = fl->flowi_tos; > > + nf_nat_decode_session(newskb, fl1, family); > > + > > + return newskb; > > Why do you return newskb here? It is not used in all > of the Acalling functions. I thought fl1, decode session, had pointers to skb fl1. That is not the case. Now the skb is consumed and not returned. > The rest looks good, thanks! thanks for the review. I will send out new version soon.
Powered by blists - more mailing lists