diff mbox series

[5.4,29/33] net, sctp, filter: remap copy_from_user failure error

Message ID 20210122135734.750091426@linuxfoundation.org
State New
Headers show
Series None | expand

Commit Message

Greg KH Jan. 22, 2021, 2:12 p.m. UTC 
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>
Cc: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 net/core/filter.c |    2 +-
 net/sctp/socket.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Marcelo Ricardo Leitner Jan. 22, 2021, 4:55 p.m. UTC | #1 
On Fri, Jan 22, 2021 at 03:12:45PM +0100, Greg Kroah-Hartman 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>
> Cc: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

For sctp bits,
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

...
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -1319,7 +1319,7 @@ static int __sctp_setsockopt_connectx(st
>  
>  	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,
> 
>
Greg KH Jan. 23, 2021, 2:57 p.m. UTC | #2 
On Fri, Jan 22, 2021 at 01:55:45PM -0300, Marcelo Ricardo Leitner wrote:
> On Fri, Jan 22, 2021 at 03:12:45PM +0100, Greg Kroah-Hartman 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>

> > Cc: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

> 

> For sctp bits,

> Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>


Thanks for the review!
diff mbox series

Patch

--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1475,7 +1475,7 @@  struct bpf_prog *__get_filter(struct soc
 
 	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;
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -1319,7 +1319,7 @@  static int __sctp_setsockopt_connectx(st
 
 	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,