netdev - Re: [PATCH bpf] bpf: don't leak memory in bpf getsockopt when optlen == 0

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: <20210111223147.crc56dz52j342wlb@kafai-mbp.dhcp.thefacebook.com>
Date:   Mon, 11 Jan 2021 14:31:47 -0800
From:   Martin KaFai Lau <kafai@...com>
To:     Stanislav Fomichev <sdf@...gle.com>
CC:     <netdev@...r.kernel.org>, <bpf@...r.kernel.org>, <ast@...nel.org>,
        <daniel@...earbox.net>
Subject: Re: [PATCH bpf] bpf: don't leak memory in bpf getsockopt when optlen
 == 0

On Mon, Jan 11, 2021 at 11:47:38AM -0800, Stanislav Fomichev wrote:
> optlen == 0 indicates that the kernel should ignore BPF buffer
> and use the original one from the user. We, however, forget
> to free the temporary buffer that we've allocated for BPF.
> 
> Reported-by: Martin KaFai Lau <kafai@...com>
> Fixes: d8fe449a9c51 ("bpf: Don't return EINVAL from {get,set}sockopt when optlen > PAGE_SIZE")
> Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> ---
>  kernel/bpf/cgroup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 6ec088a96302..09179ab72c03 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -1395,7 +1395,7 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level,
>  	}
>  
>  out:
> -	if (ret)
> +	if (*kernel_optval == NULL)
It seems fragile to depend on the caller to init *kernel_optval to NULL.

How about something like:

diff --git i/kernel/bpf/cgroup.c w/kernel/bpf/cgroup.c
index 6ec088a96302..8d94c004e781 100644
--- i/kernel/bpf/cgroup.c
+++ w/kernel/bpf/cgroup.c
@@ -1358,7 +1358,7 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level,
 
 	if (copy_from_user(ctx.optval, optval, min(*optlen, max_optlen)) != 0) {
 		ret = -EFAULT;
-		goto out;
+		goto err_out;
 	}
 
 	lock_sock(sk);
@@ -1368,7 +1368,7 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level,
 
 	if (!ret) {
 		ret = -EPERM;
-		goto out;
+		goto err_out;
 	}
 
 	if (ctx.optlen == -1) {
@@ -1379,7 +1379,6 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level,
 		ret = -EFAULT;
 	} else {
 		/* optlen within bounds, run kernel handler */
-		ret = 0;
 
 		/* export any potential modifications */
 		*level = ctx.level;
@@ -1391,12 +1390,15 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level,
 		if (ctx.optlen != 0) {
 			*optlen = ctx.optlen;
 			*kernel_optval = ctx.optval;
+		} else {
+			sockopt_free_buf(&ctx);
 		}
+
+		return 0;
 	}
 
-out:
-	if (ret)
-		sockopt_free_buf(&ctx);
+err_out:
+	sockopt_free_buf(&ctx);
 	return ret;
 }