Messages in this thread | | | From | Colin Ian King <> | Subject | re: GTP: add support for flow based tunneling API | Date | Mon, 18 Jan 2021 11:58:01 +0000 |
| |
Hi,
Static analysis of today's linux-next using Coverity has found a potential memory leak issue in the following commit:
commit 9ab7e76aefc97a9aa664accb59d6e8dc5e52514a Author: Pravin B Shelar <pbshelar@fb.com> Date: Sat Jan 9 23:00:21 2021 -0800
GTP: add support for flow based tunneling API
The analysis is as follows:
186 static int gtp_set_tun_dst(struct gtp_dev *gtp, struct sk_buff *skb, 187 unsigned int hdrlen, u8 gtp_version, 188 __be64 tid, u8 flags) 189 { 190 struct metadata_dst *tun_dst; 191 int opts_len = 0; 192
1. Condition !!(flags & 7), taking true branch. 2. Condition !!(flags & 7), taking true branch.
193 if (unlikely(flags & GTP1_F_MASK)) 194 opts_len = sizeof(struct gtpu_metadata); 195
3. alloc_fn: Storage is returned from allocation function udp_tun_rx_dst. 4. var_assign: Assigning: tun_dst = storage returned from udp_tun_rx_dst(skb, gtp->sk1u->__sk_common.skc_family, 1024, tid, opts_len).
196 tun_dst = udp_tun_rx_dst(skb, gtp->sk1u->sk_family, TUNNEL_KEY, tid, opts_len);
5. Condition !tun_dst, taking false branch.
197 if (!tun_dst) { 198 netdev_dbg(gtp->dev, "Failed to allocate tun_dst"); 199 goto err; 200 } 201
6. Condition 0 /* __builtin_types_compatible_p() */, taking false branch. 7. Condition 1 /* __builtin_types_compatible_p() */, taking true branch. 8. Falling through to end of if statement. 9. Condition !!branch, taking false branch. 10. Condition ({...; !!branch;}), taking false branch.
202 netdev_dbg(gtp->dev, "attaching metadata_dst to skb, gtp ver %d hdrlen %d\n", 203 gtp_version, hdrlen);
11. Condition !!opts_len, taking true branch. 12. Condition !!opts_len, taking true branch.
204 if (unlikely(opts_len)) { 205 struct gtpu_metadata *opts; 206 struct gtp1_header *gtp1; 207 208 opts = ip_tunnel_info_opts(&tun_dst->u.tun_info); 209 gtp1 = (struct gtp1_header *)(skb->data + sizeof(struct udphdr)); 210 opts->ver = GTP_METADATA_V1; 211 opts->flags = gtp1->flags; 212 opts->type = gtp1->type;
13. Condition 0 /* __builtin_types_compatible_p() */, taking false branch. 14. Condition 1 /* __builtin_types_compatible_p() */, taking true branch. 15. Falling through to end of if statement. 16. Condition !!branch, taking false branch. 17. Condition ({...; !!branch;}), taking false branch.
213 netdev_dbg(gtp->dev, "recved control pkt: flag %x type: %d\n", 214 opts->flags, opts->type); 215 tun_dst->u.tun_info.key.tun_flags |= TUNNEL_GTPU_OPT; 216 tun_dst->u.tun_info.options_len = opts_len; 217 skb->protocol = htons(0xffff); /* Unknown */ 218 } 219 /* Get rid of the GTP + UDP headers. */
18. Condition !net_eq(sock_net(gtp->sk1u), dev_net(gtp->dev)), taking false branch. 19. Condition !net_eq(sock_net(gtp->sk1u), dev_net(gtp->dev)), taking false branch. 20. Condition iptunnel_pull_header(skb, hdrlen, skb->protocol, !net_eq(sock_net(gtp->sk1u), dev_net(gtp->dev))), taking true branch.
220 if (iptunnel_pull_header(skb, hdrlen, skb->protocol, 221 !net_eq(sock_net(gtp->sk1u), dev_net(gtp->dev)))) { 222 gtp->dev->stats.rx_length_errors++;
21. Jumping to label err.
223 goto err; 224 } 225 226 skb_dst_set(skb, &tun_dst->dst); 227 return 0; 228 err:
Resource leak (RESOURCE_LEAK) 22. leaked_storage: Variable tun_dst going out of scope leaks the storage it points to.
229 return -1; 230 }
The goto on line 223 is leaking tun_dst. From what I can see, I believe a call to kfree(tun_dst) before the goto on line 223 looks like a pertinent fix, but I'm not 100% sure, so I'm flagging this up as an issue that need further investigation by folk who are more familiar with this code.
Colin
| |