Message ID | 20210219182939.64638-2-cascardo@canonical.com |
---|---|
State | Accepted |
Headers | show |
Series | CVE-2021-20239 | expand |
On 2/19/21 11:29 AM, Thadeu Lima de Souza Cascardo wrote: > From: Daniel Borkmann <daniel@iogearbox.net> > > [ no upstream commit ] > > Fix a potential kernel address leakage for the prerequisite where there is > a BPF program attached to the cgroup/setsockopt hook. The latter can only > be attached under root, however, if the attached program returns 1 to then > run the related kernel handler, an unprivileged program could probe for > kernel addresses that way. The reason this is possible is that we're under > set_fs(KERNEL_DS) when running the kernel setsockopt handler. Aside from > old cBPF there is also SCTP's struct sctp_getaddrs_old which contains > pointers in the uapi struct that further need copy_from_user() inside the > handler. In the normal case this would just return -EFAULT, but under a > temporary KERNEL_DS setting the memory would be copied and we'd end up at > a different error code, that is, -EINVAL, for both cases given subsequent > validations fail, which then allows the app to distinguish and make use of > this fact for probing the address space. In case of later kernel versions > this issue won't work anymore thanks to Christoph Hellwig's work that got > rid of the various temporary set_fs() address space overrides altogether. > One potential option for 5.4 as the only affected stable kernel with the > least complexity would be to remap those affected -EFAULT copy_from_user() > error codes with -EINVAL such that they cannot be probed anymore. Risk of > breakage should be rather low for this particular error case. > > Fixes: 0d01da6afc54 ("bpf: implement getsockopt and setsockopt hooks") > Reported-by: Ryota Shiga (Flatt Security) > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> > Cc: Stanislav Fomichev <sdf@google.com> > Cc: Eric Dumazet <edumazet@google.com> > Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > (cherry picked from commit 55bac51762c39ef033b488dd09b60d48908d317f linux-5.4.y) > CVE-2021-20239 > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > --- > net/core/filter.c | 2 +- > net/sctp/socket.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/core/filter.c b/net/core/filter.c > index 3e4de9e461bd..853c0d5b9fb9 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -1464,7 +1464,7 @@ struct bpf_prog *__get_filter(struct sock_fprog *fprog, struct sock *sk) > > if (copy_from_user(prog->insns, fprog->filter, fsize)) { > __bpf_prog_free(prog); > - return ERR_PTR(-EFAULT); > + return ERR_PTR(-EINVAL); > } > > prog->len = fprog->len; > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > index 102aee4f7dfd..bde36e0a7308 100644 > --- a/net/sctp/socket.c > +++ b/net/sctp/socket.c > @@ -1325,7 +1325,7 @@ static int __sctp_setsockopt_connectx(struct sock *sk, > > kaddrs = memdup_user(addrs, addrs_size); > if (IS_ERR(kaddrs)) > - return PTR_ERR(kaddrs); > + return PTR_ERR(kaddrs) == -EFAULT ? -EINVAL : PTR_ERR(kaddrs); > > /* Allow security module to validate connectx addresses. */ > err = security_sctp_bind_connect(sk, SCTP_SOCKOPT_CONNECTX, > Acked-by: Tim Gardner <tim.gardner@canonical.com> ----------- Tim Gardner Canonical, Inc
On 19/02/2021 18:29, Thadeu Lima de Souza Cascardo wrote: > From: Daniel Borkmann <daniel@iogearbox.net> > > [ no upstream commit ] > > Fix a potential kernel address leakage for the prerequisite where there is > a BPF program attached to the cgroup/setsockopt hook. The latter can only > be attached under root, however, if the attached program returns 1 to then > run the related kernel handler, an unprivileged program could probe for > kernel addresses that way. The reason this is possible is that we're under > set_fs(KERNEL_DS) when running the kernel setsockopt handler. Aside from > old cBPF there is also SCTP's struct sctp_getaddrs_old which contains > pointers in the uapi struct that further need copy_from_user() inside the > handler. In the normal case this would just return -EFAULT, but under a > temporary KERNEL_DS setting the memory would be copied and we'd end up at > a different error code, that is, -EINVAL, for both cases given subsequent > validations fail, which then allows the app to distinguish and make use of > this fact for probing the address space. In case of later kernel versions > this issue won't work anymore thanks to Christoph Hellwig's work that got > rid of the various temporary set_fs() address space overrides altogether. > One potential option for 5.4 as the only affected stable kernel with the > least complexity would be to remap those affected -EFAULT copy_from_user() > error codes with -EINVAL such that they cannot be probed anymore. Risk of > breakage should be rather low for this particular error case. > > Fixes: 0d01da6afc54 ("bpf: implement getsockopt and setsockopt hooks") > Reported-by: Ryota Shiga (Flatt Security) > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> > Cc: Stanislav Fomichev <sdf@google.com> > Cc: Eric Dumazet <edumazet@google.com> > Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > (cherry picked from commit 55bac51762c39ef033b488dd09b60d48908d317f linux-5.4.y) > CVE-2021-20239 > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > --- > net/core/filter.c | 2 +- > net/sctp/socket.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/core/filter.c b/net/core/filter.c > index 3e4de9e461bd..853c0d5b9fb9 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -1464,7 +1464,7 @@ struct bpf_prog *__get_filter(struct sock_fprog *fprog, struct sock *sk) > > if (copy_from_user(prog->insns, fprog->filter, fsize)) { > __bpf_prog_free(prog); > - return ERR_PTR(-EFAULT); > + return ERR_PTR(-EINVAL); > } > > prog->len = fprog->len; > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > index 102aee4f7dfd..bde36e0a7308 100644 > --- a/net/sctp/socket.c > +++ b/net/sctp/socket.c > @@ -1325,7 +1325,7 @@ static int __sctp_setsockopt_connectx(struct sock *sk, > > kaddrs = memdup_user(addrs, addrs_size); > if (IS_ERR(kaddrs)) > - return PTR_ERR(kaddrs); > + return PTR_ERR(kaddrs) == -EFAULT ? -EINVAL : PTR_ERR(kaddrs); > > /* Allow security module to validate connectx addresses. */ > err = security_sctp_bind_connect(sk, SCTP_SOCKOPT_CONNECTX, > LGTM. Thanks Thadeu Acked-by: Colin Ian King <colin.king@canonical.com>
diff --git a/net/core/filter.c b/net/core/filter.c index 3e4de9e461bd..853c0d5b9fb9 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -1464,7 +1464,7 @@ struct bpf_prog *__get_filter(struct sock_fprog *fprog, struct sock *sk) if (copy_from_user(prog->insns, fprog->filter, fsize)) { __bpf_prog_free(prog); - return ERR_PTR(-EFAULT); + return ERR_PTR(-EINVAL); } prog->len = fprog->len; diff --git a/net/sctp/socket.c b/net/sctp/socket.c index 102aee4f7dfd..bde36e0a7308 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c @@ -1325,7 +1325,7 @@ static int __sctp_setsockopt_connectx(struct sock *sk, kaddrs = memdup_user(addrs, addrs_size); if (IS_ERR(kaddrs)) - return PTR_ERR(kaddrs); + return PTR_ERR(kaddrs) == -EFAULT ? -EINVAL : PTR_ERR(kaddrs); /* Allow security module to validate connectx addresses. */ err = security_sctp_bind_connect(sk, SCTP_SOCKOPT_CONNECTX,