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

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

[Stanislav Fomichev]

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@fb.com>
Fixes: d8fe449a9c51 ("bpf: Don't return EINVAL from {get,set}sockopt when optlen > PAGE_SIZE")
Signed-off-by: Stanislav Fomichev <sdf@google.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)
 		sockopt_free_buf(&ctx);
 	return ret;
 }
-- 
2.30.0.284.gd98b1dd5eaa7-goog

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

[Martin KaFai Lau]

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@fb.com>
> Fixes: d8fe449a9c51 ("bpf: Don't return EINVAL from {get,set}sockopt when optlen > PAGE_SIZE")
> Signed-off-by: Stanislav Fomichev <sdf@google.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;
 }

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

[Stanislav Fomichev]

On Mon, Jan 11, 2021 at 2:32 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> 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@fb.com>
> > Fixes: d8fe449a9c51 ("bpf: Don't return EINVAL from {get,set}sockopt when optlen > PAGE_SIZE")
> > Signed-off-by: Stanislav Fomichev <sdf@google.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.
We can manually reset it to NULL when we enter
__cgroup_bpf_run_filter_setsockopt,
I didn't bother since there is only one existing caller.

But you patch also LGTM, I don't really have a preference.

> 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;
>  }

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

[Martin KaFai Lau]

On Mon, Jan 11, 2021 at 02:38:02PM -0800, Stanislav Fomichev wrote:
> On Mon, Jan 11, 2021 at 2:32 PM Martin KaFai Lau <kafai@fb.com> wrote:
> >
> > 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@fb.com>
> > > Fixes: d8fe449a9c51 ("bpf: Don't return EINVAL from {get,set}sockopt when optlen > PAGE_SIZE")
> > > Signed-off-by: Stanislav Fomichev <sdf@google.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.
> We can manually reset it to NULL when we enter
> __cgroup_bpf_run_filter_setsockopt,
It feels weird to reset the caller value at the beginning while this is not
intended to be an _init() like function, so I avoided it.

but yeah, I am fine on this way also and won't oppose it strongly ;)

> I didn't bother since there is only one existing caller.
> 
> But you patch also LGTM, I don't really have a preference.
> 
> > 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;
> >  }