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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <60a3525d188d9_18a5f208f5@john-XPS-13-9370.notmuch>
Date:   Mon, 17 May 2021 22:36:29 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Cong Wang <xiyou.wangcong@...il.com>, netdev@...r.kernel.org
Cc:     bpf@...r.kernel.org, Cong Wang <cong.wang@...edance.com>,
        John Fastabend <john.fastabend@...il.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jakub Sitnicki <jakub@...udflare.com>,
        Lorenz Bauer <lmb@...udflare.com>
Subject: RE: [Patch bpf] udp: fix a memory leak in udp_read_sock()

Cong Wang wrote:
> From: Cong Wang <cong.wang@...edance.com>
> 
> sk_psock_verdict_recv() clones the skb and uses the clone
> afterward, so udp_read_sock() should free the original skb after
> done using it.

The clone only happens if sk_psock_verdict_recv() returns >0.

> 
> Fixes: d7f571188ecf ("udp: Implement ->read_sock() for sockmap")
> Cc: John Fastabend <john.fastabend@...il.com>
> Cc: Daniel Borkmann <daniel@...earbox.net>
> Cc: Jakub Sitnicki <jakub@...udflare.com>
> Cc: Lorenz Bauer <lmb@...udflare.com>
> Signed-off-by: Cong Wang <cong.wang@...edance.com>
> ---
>  net/ipv4/udp.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 15f5504adf5b..e31d67fd5183 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -1798,11 +1798,13 @@ int udp_read_sock(struct sock *sk, read_descriptor_t *desc,
>  		if (used <= 0) {
>  			if (!copied)
>  				copied = used;
> +			kfree_skb(skb);

This case is different from the TCP side, if there is an error
the sockmap side will also call kfree_skb(). In TCP side we peek
the skb because we don't want to drop it. On UDP side this will
just drop data on the floor. Its not super friendly, but its
UDP so we are making the assumption this is ok? We've tried
to remove all the drop data cases from TCP it would be nice
to not drop data on UDP side if we can help it. Could we
requeue or peek the UDP skb to avoid this?

>  			break;
>  		} else if (used <= skb->len) {
>  			copied += used;
>  		}
>  
> +		kfree_skb(skb);
>  		if (!desc->count)
>  			break;
>  	}
> -- 
> 2.25.1
> 


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ