Messages in this thread Patch in this message | | | Date | Fri, 06 Mar 1998 22:33:14 -0500 | From | Bill Hawes <> | Subject | patch for 2.1.88 skb leaks |
| |
In reviewing the network code I found a number of places where an error exit would fail to free an skb obtained from a skb_recv_datagram() call. The errors in question involve faults when copying iovecs, and so are probably rare but could possibly be generated intentionally. The attached patch should fix the leaks.
Also included is a change for ipv6 multicasting, where a cloned skb was being queued on the wrong sk (and the allocation wasn't being checked.) I also added code to lock the hashes following the ipv4 practice.
I'm running with the ipv4 and af_packet changes, but haven't tested the other subsystems.
Regards, Bill--- linux-2.1.88/net/ipv4/udp.c.old Sat Feb 21 09:25:04 1998 +++ linux-2.1.88/net/ipv4/udp.c Fri Mar 6 18:52:30 1998 @@ -816,8 +816,10 @@ struct sk_buff *skb; unsigned long amount; - if (sk->state == TCP_LISTEN) return(-EINVAL); + if (sk->state == TCP_LISTEN) + return(-EINVAL); amount = 0; + /* N.B. Is this interrupt safe?? */ skb = skb_peek(&sk->receive_queue); if (skb != NULL) { /* @@ -843,13 +845,11 @@ */ int udp_recvmsg(struct sock *sk, struct msghdr *msg, int len, - int noblock, int flags,int *addr_len) + int noblock, int flags, int *addr_len) { - int copied = 0; - int truesize; + struct sockaddr_in *sin = (struct sockaddr_in *)msg->msg_name; struct sk_buff *skb; - int er; - struct sockaddr_in *sin=(struct sockaddr_in *)msg->msg_name; + int copied, err; /* * Check any passed addresses @@ -859,14 +859,12 @@ *addr_len=sizeof(*sin); if (sk->ip_recverr && (skb = skb_dequeue(&sk->error_queue)) != NULL) { - er = sock_error(sk); - if (msg->msg_controllen == 0) { - skb_free_datagram(sk, skb); - return er; + err = sock_error(sk); + if (msg->msg_controllen != 0) { + put_cmsg(msg, SOL_IP, IP_RECVERR, skb->len, skb->data); + err = 0; } - put_cmsg(msg, SOL_IP, IP_RECVERR, skb->len, skb->data); - skb_free_datagram(sk, skb); - return 0; + goto out_free; } /* @@ -874,25 +872,25 @@ * the finished NET3, it will do _ALL_ the work! */ - skb=skb_recv_datagram(sk,flags,noblock,&er); - if(skb==NULL) - return er; + skb = skb_recv_datagram(sk, flags, noblock, &err); + if (!skb) + goto out; - truesize = skb->len - sizeof(struct udphdr); - copied = truesize; - if (len < truesize) + copied = skb->len - sizeof(struct udphdr); + if (copied > len) { - msg->msg_flags |= MSG_TRUNC; copied = len; + msg->msg_flags |= MSG_TRUNC; } /* * FIXME : should use udp header size info value */ - er = skb_copy_datagram_iovec(skb,sizeof(struct udphdr),msg->msg_iov,copied); - if (er) - return er; + err = skb_copy_datagram_iovec(skb, sizeof(struct udphdr), msg->msg_iov, + copied); + if (err) + goto out_free; sk->stamp=skb->stamp; /* Copy the address. */ @@ -921,9 +919,12 @@ } if (sk->ip_cmsg_flags) ip_cmsg_recv(msg, skb); + err = copied; +out_free: skb_free_datagram(sk, skb); - return(copied); +out: + return err; } int udp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len) --- linux-2.1.88/net/ipv6/udp.c.old Wed Feb 11 00:06:15 1998 +++ linux-2.1.88/net/ipv6/udp.c Fri Mar 6 18:51:56 1998 @@ -300,10 +300,8 @@ int udpv6_recvmsg(struct sock *sk, struct msghdr *msg, int len, int noblock, int flags, int *addr_len) { - int copied = 0; - int truesize; struct sk_buff *skb; - int err; + int copied, err; /* * Check any passed addresses @@ -318,16 +316,13 @@ */ skb = skb_recv_datagram(sk, flags, noblock, &err); - if(skb==NULL) - return err; + if (!skb) + goto out; - truesize=ntohs(((struct udphdr *)skb->h.raw)->len) - sizeof(struct udphdr); - - copied=truesize; - - if(copied>len) { - copied=len; - msg->msg_flags|=MSG_TRUNC; + copied = ntohs(((struct udphdr *)skb->h.raw)->len) - sizeof(struct udphdr); + if (copied > len) { + copied = len; + msg->msg_flags |= MSG_TRUNC; } /* @@ -337,7 +332,7 @@ err = skb_copy_datagram_iovec(skb, sizeof(struct udphdr), msg->msg_iov, copied); if (err) - return err; + goto out_free; sk->stamp=skb->stamp; @@ -346,7 +341,6 @@ struct sockaddr_in6 *sin6; sin6 = (struct sockaddr_in6 *) msg->msg_name; - sin6->sin6_family = AF_INET6; sin6->sin6_port = skb->h.uh->source; @@ -361,9 +355,12 @@ datagram_recv_ctl(sk, msg, skb); } } - + err = copied; + +out_free: skb_free_datagram(sk, skb); - return(copied); +out: + return err; } void udpv6_err(int type, int code, unsigned char *buff, __u32 info, @@ -457,6 +454,7 @@ { struct sock *sk, *sk2; + SOCKHASH_LOCK(); sk = udp_hash[ntohs(uh->dest) & (UDP_HTABLE_SIZE - 1)]; sk = udp_v6_mcast_next(sk, uh->dest, daddr, uh->source, saddr); if(sk) { @@ -465,7 +463,7 @@ uh->dest, saddr, uh->source, daddr))) { struct sk_buff *buff = skb_clone(skb, GFP_ATOMIC); - if(sock_queue_rcv_skb(sk, buff) < 0) { + if (buff && sock_queue_rcv_skb(sk2, buff) < 0) { buff->sk = NULL; kfree_skb(buff); } @@ -475,6 +473,7 @@ skb->sk = NULL; kfree_skb(skb); } + SOCKHASH_UNLOCK(); } int udpv6_rcv(struct sk_buff *skb, struct device *dev, --- linux-2.1.88/net/ipv6/raw.c.old Wed Feb 11 00:06:14 1998 +++ linux-2.1.88/net/ipv6/raw.c Fri Mar 6 18:54:45 1998 @@ -236,13 +236,11 @@ */ int rawv6_recvmsg(struct sock *sk, struct msghdr *msg, int len, - int noblock, int flags,int *addr_len) + int noblock, int flags, int *addr_len) { - struct sockaddr_in6 *sin6=(struct sockaddr_in6 *)msg->msg_name; + struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)msg->msg_name; struct sk_buff *skb; - int copied=0; - int err; - + int copied, err; if (flags & MSG_OOB) return -EOPNOTSUPP; @@ -253,32 +251,32 @@ if (addr_len) *addr_len=sizeof(*sin6); - skb=skb_recv_datagram(sk, flags, noblock, &err); - if(skb==NULL) - return err; + skb = skb_recv_datagram(sk, flags, noblock, &err); + if (!skb) + goto out; copied = min(len, skb->tail - skb->h.raw); err = skb_copy_datagram_iovec(skb, 0, msg->msg_iov, copied); sk->stamp=skb->stamp; - if (err) - return err; + goto out_free; /* Copy the address. */ if (sin6) { sin6->sin6_family = AF_INET6; memcpy(&sin6->sin6_addr, &skb->nh.ipv6h->saddr, sizeof(struct in6_addr)); - - *addr_len = sizeof(struct sockaddr_in6); } if (msg->msg_controllen) datagram_recv_ctl(sk, msg, skb); + err = copied; +out_free: skb_free_datagram(sk, skb); - return (copied); +out: + return err; } /* --- linux-2.1.88/net/unix/af_unix.c.old Wed Feb 11 00:06:15 1998 +++ linux-2.1.88/net/unix/af_unix.c Fri Mar 6 16:04:08 1998 @@ -1033,8 +1033,9 @@ size=len-sent; - if (size>(sk->sndbuf-sizeof(struct sk_buff))/2) /* Keep two messages in the pipe so it schedules better */ - size=(sk->sndbuf-sizeof(struct sk_buff))/2; + /* Keep two messages in the pipe so it schedules better */ + if (size > (sk->sndbuf - sizeof(struct sk_buff)) / 2) + size = (sk->sndbuf - sizeof(struct sk_buff)) / 2; /* * Keep to page sized kmalloc()'s as various people @@ -1056,7 +1057,7 @@ if (skb==NULL) { if (sent) - return sent; + goto out; return err; } @@ -1074,6 +1075,7 @@ if (scm->fp) unix_attach_fds(scm, skb); + /* N.B. this could fail with -EFAULT */ memcpy_fromiovec(skb_put(skb,size), msg->msg_iov, size); other=unix_peer(sk); @@ -1082,7 +1084,7 @@ { kfree_skb(skb); if(sent) - return sent; + goto out; send_sig(SIGPIPE,current,0); return -EPIPE; } @@ -1091,6 +1093,7 @@ other->data_ready(other,size); sent+=size; } +out: return sent; } @@ -1121,20 +1124,20 @@ msg->msg_namelen = 0; - skb=skb_recv_datagram(sk, flags, noblock, &err); - if(skb==NULL) - return err; + skb = skb_recv_datagram(sk, flags, noblock, &err); + if (!skb) + goto out; if (msg->msg_name) { + msg->msg_namelen = sizeof(short); if (skb->sk->protinfo.af_unix.addr) { - memcpy(msg->msg_name, skb->sk->protinfo.af_unix.addr->name, - skb->sk->protinfo.af_unix.addr->len); msg->msg_namelen=skb->sk->protinfo.af_unix.addr->len; + memcpy(msg->msg_name, + skb->sk->protinfo.af_unix.addr->name, + skb->sk->protinfo.af_unix.addr->len); } - else - msg->msg_namelen=sizeof(short); } if (size > skb->len) @@ -1142,8 +1145,9 @@ else if (size < skb->len) msg->msg_flags |= MSG_TRUNC; - if (skb_copy_datagram_iovec(skb, 0, msg->msg_iov, size)) - return -EFAULT; + err = skb_copy_datagram_iovec(skb, 0, msg->msg_iov, size); + if (err) + goto out_free; scm->creds = *UNIXCREDS(skb); @@ -1169,8 +1173,12 @@ if (UNIXCB(skb).fp) scm->fp = scm_fp_dup(UNIXCB(skb).fp); } + err = size; + +out_free: skb_free_datagram(sk,skb); - return size; +out: + return err; } @@ -1189,7 +1197,7 @@ if (flags&MSG_OOB) return -EOPNOTSUPP; - if(flags&MSG_WAITALL) + if (flags&MSG_WAITALL) target = size; @@ -1245,18 +1253,19 @@ /* Copy address just once */ if (sunaddr) { + msg->msg_namelen = sizeof(short); if (skb->sk->protinfo.af_unix.addr) { - memcpy(sunaddr, skb->sk->protinfo.af_unix.addr->name, - skb->sk->protinfo.af_unix.addr->len); msg->msg_namelen=skb->sk->protinfo.af_unix.addr->len; + memcpy(sunaddr, + skb->sk->protinfo.af_unix.addr->name, + skb->sk->protinfo.af_unix.addr->len); } - else - msg->msg_namelen=sizeof(short); sunaddr = NULL; } chunk = min(skb->len, size); + /* N.B. This could fail with -EFAULT */ memcpy_toiovec(msg->msg_iov, skb->data, chunk); copied += chunk; size -= chunk; --- linux-2.1.88/net/packet/af_packet.c.old Wed Feb 11 00:06:15 1998 +++ linux-2.1.88/net/packet/af_packet.c Fri Mar 6 18:56:56 1998 @@ -778,9 +778,8 @@ int flags, struct scm_cookie *scm) { struct sock *sk = sock->sk; - int copied=0; struct sk_buff *skb; - int err; + int copied, err; #if 0 /* What error should we return now? EUNATTACH? */ @@ -816,7 +815,7 @@ */ if(skb==NULL) - return err; + goto out; /* * You lose any data beyond the buffer you gave. If it worries a @@ -824,7 +823,7 @@ */ copied = skb->len; - if(copied>len) + if (copied > len) { copied=len; msg->msg_flags|=MSG_TRUNC; @@ -833,9 +832,7 @@ /* We can't use skb_copy_datagram here */ err = memcpy_toiovec(msg->msg_iov, skb->data, copied); if (err) - { - return -EFAULT; - } + goto out_free; sk->stamp=skb->stamp; @@ -843,13 +840,15 @@ memcpy(msg->msg_name, skb->cb, msg->msg_namelen); /* - * Free or return the buffer as appropriate. Again this hides all the - * races and re-entrancy issues from us. + * Free or return the buffer as appropriate. Again this + * hides all the races and re-entrancy issues from us. */ + err = copied; +out_free: skb_free_datagram(sk, skb); - - return(copied); +out: + return err; } #ifdef CONFIG_SOCK_PACKET --- linux-2.1.88/net/ipx/af_ipx.c.old Sat Feb 21 09:25:04 1998 +++ linux-2.1.88/net/ipx/af_ipx.c Fri Mar 6 19:06:15 1998 @@ -998,7 +998,8 @@ return -EPROTONOSUPPORT; dev=dev_get(idef->ipx_device); - if(dev==NULL) return -ENODEV; + if (dev==NULL) + return -ENODEV; intrfc = ipxitf_find_using_phys(dev, dlink_type); if (intrfc != NULL) { @@ -1107,9 +1108,9 @@ sipx->sipx_family=AF_IPX; sipx->sipx_network=ipxif->if_netnum; memcpy(sipx->sipx_node, ipxif->if_node, sizeof(sipx->sipx_node)); - err = copy_to_user(arg,&ifr,sizeof(ifr)); - if (err) - return -EFAULT; + err = -EFAULT; + if (!copy_to_user(arg, &ifr, sizeof(ifr))) + err = 0; return err; } case SIOCAIPXITFCRT: @@ -2134,32 +2135,28 @@ struct sock *sk=sock->sk; struct sockaddr_ipx *sipx=(struct sockaddr_ipx *)msg->msg_name; struct ipxhdr *ipx = NULL; - int copied = 0; - int truesize; struct sk_buff *skb; - int err; + int copied, err; if (sk->zapped) return -ENOTCONN; skb=skb_recv_datagram(sk,flags&~MSG_DONTWAIT,flags&MSG_DONTWAIT,&err); - if(skb==NULL) - return err; + if (!skb) + goto out; ipx = skb->nh.ipxh; - truesize=ntohs(ipx->ipx_pktsize) - sizeof(struct ipxhdr); - - copied = truesize; + copied = ntohs(ipx->ipx_pktsize) - sizeof(struct ipxhdr); if(copied > size) { copied=size; msg->msg_flags|=MSG_TRUNC; } - err = skb_copy_datagram_iovec(skb,sizeof(struct ipxhdr),msg->msg_iov,copied); - + err = skb_copy_datagram_iovec(skb, sizeof(struct ipxhdr), msg->msg_iov, + copied); if (err) - return err; + goto out_free; msg->msg_namelen = sizeof(*sipx); @@ -2171,9 +2168,12 @@ sipx->sipx_network=ipx->ipx_source.net; sipx->sipx_type = ipx->ipx_type; } - skb_free_datagram(sk, skb); + err = copied; - return(copied); +out_free: + skb_free_datagram(sk, skb); +out: + return err; } /* @@ -2228,11 +2228,12 @@ { if(sk->stamp.tv_sec==0) return -ENOENT; - ret = copy_to_user((void *)arg,&sk->stamp,sizeof(struct timeval)); - if (ret) - ret = -EFAULT; + ret = -EFAULT; + if (!copy_to_user((void *)arg, &sk->stamp, + sizeof(struct timeval))) + ret = 0; } - return 0; + return ret; } case SIOCGIFDSTADDR: case SIOCSIFDSTADDR: | |