[PATCH] net: check the return value of the copy_from_sockptr

[PATCH] net: check the return value of the copy_from_sockptr

[Qianqiang Liu]

We must check the return value of the copy_from_sockptr. Otherwise, it
may cause some weird issues.

Signed-off-by: Qianqiang Liu <qianqiang.liu@163.com>
---
 net/socket.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index 0a2bd22ec105..6b9a414d01d5 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -2370,8 +2370,11 @@ int do_sock_getsockopt(struct socket *sock, bool compat, int level,
 	if (err)
 		return err;
 
-	if (!compat)
-		copy_from_sockptr(&max_optlen, optlen, sizeof(int));
+	if (!compat) {
+		err = copy_from_sockptr(&max_optlen, optlen, sizeof(int));
+		if (err)
+			return -EFAULT;
+	}
 
 	ops = READ_ONCE(sock->ops);
 	if (level == SOL_SOCKET) {
-- 
2.39.2

Re: [PATCH] net: check the return value of the copy_from_sockptr

[D. Wythe]

On 9/11/24 1:04 PM, Qianqiang Liu wrote:
> We must check the return value of the copy_from_sockptr. Otherwise, it
> may cause some weird issues.
>
> Signed-off-by: Qianqiang Liu <qianqiang.liu@163.com>
> ---
>   net/socket.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)

Looks like a fix patch, maybe you could add a fix tag.

> diff --git a/net/socket.c b/net/socket.c
> index 0a2bd22ec105..6b9a414d01d5 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -2370,8 +2370,11 @@ int do_sock_getsockopt(struct socket *sock, bool compat, int level,
>   	if (err)
>   		return err;
>   
> -	if (!compat)
> -		copy_from_sockptr(&max_optlen, optlen, sizeof(int));
> +	if (!compat) {
> +		err = copy_from_sockptr(&max_optlen, optlen, sizeof(int));
> +		if (err)
> +			return -EFAULT;
> +	}
>   
>   	ops = READ_ONCE(sock->ops);
>   	if (level == SOL_SOCKET) {

Re: [PATCH] net: check the return value of the copy_from_sockptr

[Eric Dumazet]

On Wed, Sep 11, 2024 at 7:06 AM Qianqiang Liu <qianqiang.liu@163.com> wrote:
>
> We must check the return value of the copy_from_sockptr. Otherwise, it
> may cause some weird issues.

What issues precisely ?

I do not think it matters, because the copy is performed later, with
all the needed checks.

eg :

if (copy_from_sockptr(&len, optlen, sizeof(int)))
    return -EFAULT;
...
len = min_t(unsigned int, sizeof(int), len);
if (copy_to_sockptr(optlen, &len, sizeof(int)))
    return -EFAULT;
if (copy_to_sockptr(optval, &val, len))
    return -EFAULT;
return 0;


>
> Signed-off-by: Qianqiang Liu <qianqiang.liu@163.com>
> ---
>  net/socket.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/net/socket.c b/net/socket.c
> index 0a2bd22ec105..6b9a414d01d5 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -2370,8 +2370,11 @@ int do_sock_getsockopt(struct socket *sock, bool compat, int level,
>         if (err)
>                 return err;
>
> -       if (!compat)
> -               copy_from_sockptr(&max_optlen, optlen, sizeof(int));
> +       if (!compat) {
> +               err = copy_from_sockptr(&max_optlen, optlen, sizeof(int));
> +               if (err)
> +                       return -EFAULT;
> +       }
>
>         ops = READ_ONCE(sock->ops);
>         if (level == SOL_SOCKET) {
> --
> 2.39.2
>

Re: [PATCH] net: check the return value of the copy_from_sockptr

[Qianqiang Liu]

> I do not think it matters, because the copy is performed later, with
> all the needed checks.

No, there is no checks at all.

-- 
Best,
Qianqiang Liu

Re: [PATCH] net: check the return value of the copy_from_sockptr

[Eric Dumazet]

On Wed, Sep 11, 2024 at 10:23 AM Qianqiang Liu <qianqiang.liu@163.com> wrote:
>
> > I do not think it matters, because the copy is performed later, with
> > all the needed checks.
>
> No, there is no checks at all.
>

Please elaborate ?
Why should maintainers have to spend time to provide evidence to
support your claims ?
Have you thought about the (compat) case ?

There are plenty of checks. They were there before Stanislav commit.

Each getsockopt() handler must perform the same actions.

For instance, look at do_ipv6_getsockopt()

If you find one getsockopt() method failing to perform the checks,
please report to us.

Re: [PATCH] net: check the return value of the copy_from_sockptr

[Qianqiang Liu]

On Wed, Sep 11, 2024 at 11:12:24AM +0200, Eric Dumazet wrote:
> On Wed, Sep 11, 2024 at 10:23 AM Qianqiang Liu <qianqiang.liu@163.com> wrote:
> >
> > > I do not think it matters, because the copy is performed later, with
> > > all the needed checks.
> >
> > No, there is no checks at all.
> >
> 
> Please elaborate ?
> Why should maintainers have to spend time to provide evidence to
> support your claims ?
> Have you thought about the (compat) case ?
> 
> There are plenty of checks. They were there before Stanislav commit.
> 
> Each getsockopt() handler must perform the same actions.
> 
> For instance, look at do_ipv6_getsockopt()
> 
> If you find one getsockopt() method failing to perform the checks,
> please report to us.

Sorry, let me explain a little bit.

The issue was introduced in this commit: 33f339a1ba54e ("bpf, net: Fix a
potential race in do_sock_getsockopt()")

diff --git a/net/socket.c b/net/socket.c
index fcbdd5bc47ac..0a2bd22ec105 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -2362,7 +2362,7 @@ INDIRECT_CALLABLE_DECLARE(bool tcp_bpf_bypass_getsockopt(int level,
 int do_sock_getsockopt(struct socket *sock, bool compat, int level,
 		       int optname, sockptr_t optval, sockptr_t optlen)
 {
-	int max_optlen __maybe_unused;
+	int max_optlen __maybe_unused = 0;
 	const struct proto_ops *ops;
 	int err;
 
@@ -2371,7 +2371,7 @@ int do_sock_getsockopt(struct socket *sock, bool compat, int level,
 		return err;
 
 	if (!compat)
-		max_optlen = BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen);
+		copy_from_sockptr(&max_optlen, optlen, sizeof(int));
 
 	ops = READ_ONCE(sock->ops);
 	if (level == SOL_SOCKET) {

The return value of "copy_from_sockptr()" in "do_sock_getsockopt()" was
not checked. So, I added the following patch:

diff --git a/net/socket.c b/net/socket.c
index 0a2bd22ec105..6b9a414d01d5 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -2370,8 +2370,11 @@ int do_sock_getsockopt(struct socket *sock, bool compat, int level,
 	if (err)
 		return err;
 
-	if (!compat)
-		copy_from_sockptr(&max_optlen, optlen, sizeof(int));
+	if (!compat) {
+		err = copy_from_sockptr(&max_optlen, optlen, sizeof(int));
+		if (err)
+			return -EFAULT;
+	}
 
 	ops = READ_ONCE(sock->ops);
 	if (level == SOL_SOCKET) {

Maybe I missed something?

If you think it's not an issue, then I'm OK with it.

-- 
Best,
Qianqiang Liu

Re: [PATCH] net: check the return value of the copy_from_sockptr

[Eric Dumazet]

On Wed, Sep 11, 2024 at 1:14 PM Qianqiang Liu <qianqiang.liu@163.com> wrote:
>
> On Wed, Sep 11, 2024 at 11:12:24AM +0200, Eric Dumazet wrote:
> > On Wed, Sep 11, 2024 at 10:23 AM Qianqiang Liu <qianqiang.liu@163.com> wrote:
> > >
> > > > I do not think it matters, because the copy is performed later, with
> > > > all the needed checks.
> > >
> > > No, there is no checks at all.
> > >
> >
> > Please elaborate ?
> > Why should maintainers have to spend time to provide evidence to
> > support your claims ?
> > Have you thought about the (compat) case ?
> >
> > There are plenty of checks. They were there before Stanislav commit.
> >
> > Each getsockopt() handler must perform the same actions.
> >
> > For instance, look at do_ipv6_getsockopt()
> >
> > If you find one getsockopt() method failing to perform the checks,
> > please report to us.
>
> Sorry, let me explain a little bit.
>
> The issue was introduced in this commit: 33f339a1ba54e ("bpf, net: Fix a
> potential race in do_sock_getsockopt()")

Not really.

Code before this commit also ignored copy_from_sockptr() return code.

>
> diff --git a/net/socket.c b/net/socket.c
> index fcbdd5bc47ac..0a2bd22ec105 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -2362,7 +2362,7 @@ INDIRECT_CALLABLE_DECLARE(bool tcp_bpf_bypass_getsockopt(int level,
>  int do_sock_getsockopt(struct socket *sock, bool compat, int level,
>                        int optname, sockptr_t optval, sockptr_t optlen)
>  {
> -       int max_optlen __maybe_unused;
> +       int max_optlen __maybe_unused = 0;
>         const struct proto_ops *ops;
>         int err;
>
> @@ -2371,7 +2371,7 @@ int do_sock_getsockopt(struct socket *sock, bool compat, int level,
>                 return err;
>
>         if (!compat)
> -               max_optlen = BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen);
> +               copy_from_sockptr(&max_optlen, optlen, sizeof(int));
>
>         ops = READ_ONCE(sock->ops);
>         if (level == SOL_SOCKET) {
>
> The return value of "copy_from_sockptr()" in "do_sock_getsockopt()" was
> not checked. So, I added the following patch:
>
> diff --git a/net/socket.c b/net/socket.c
> index 0a2bd22ec105..6b9a414d01d5 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -2370,8 +2370,11 @@ int do_sock_getsockopt(struct socket *sock, bool compat, int level,
>         if (err)
>                 return err;
>
> -       if (!compat)
> -               copy_from_sockptr(&max_optlen, optlen, sizeof(int));
> +       if (!compat) {
> +               err = copy_from_sockptr(&max_optlen, optlen, sizeof(int));
> +               if (err)
> +                       return -EFAULT;
> +       }
>
>         ops = READ_ONCE(sock->ops);
>         if (level == SOL_SOCKET) {
>
> Maybe I missed something?
>
> If you think it's not an issue, then I'm OK with it.

It is not an issue, just adding extra code for an unlikely condition
that must be tested later anyway.

Re: [PATCH] net: check the return value of the copy_from_sockptr

[Cong Wang]

On Wed, Sep 11, 2024 at 11:12:24AM +0200, Eric Dumazet wrote:
> On Wed, Sep 11, 2024 at 10:23 AM Qianqiang Liu <qianqiang.liu@163.com> wrote:
> >
> > > I do not think it matters, because the copy is performed later, with
> > > all the needed checks.
> >
> > No, there is no checks at all.
> >
> 
> Please elaborate ?
> Why should maintainers have to spend time to provide evidence to
> support your claims ?
> Have you thought about the (compat) case ?
> 
> There are plenty of checks. They were there before Stanislav commit.
> 
> Each getsockopt() handler must perform the same actions.


But in line 2379 we have ops->getsockopt==NULL case:

2373         if (!compat)
2374                 copy_from_sockptr(&max_optlen, optlen, sizeof(int));
2375
2376         ops = READ_ONCE(sock->ops);
2377         if (level == SOL_SOCKET) {
2378                 err = sk_getsockopt(sock->sk, level, optname, optval, optlen);
2379         } else if (unlikely(!ops->getsockopt)) {
2380                 err = -EOPNOTSUPP; 	// <--- HERE
2381         } else {
2382                 if (WARN_ONCE(optval.is_kernel || optlen.is_kernel,
2383                               "Invalid argument type"))
2384                         return -EOPNOTSUPP;
2385
2386                 err = ops->getsockopt(sock, level, optname, optval.user,
2387                                       optlen.user);
2388         }

where we simply continue with calling BPF_CGROUP_RUN_PROG_GETSOCKOPT()
which actually needs the 'max_optlen' we copied via copy_from_sockptr().

Do I miss anything here?

Thanks.

Re: [PATCH] net: check the return value of the copy_from_sockptr

[Eric Dumazet]

On Wed, Sep 11, 2024 at 6:58 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Wed, Sep 11, 2024 at 11:12:24AM +0200, Eric Dumazet wrote:
> > On Wed, Sep 11, 2024 at 10:23 AM Qianqiang Liu <qianqiang.liu@163.com> wrote:
> > >
> > > > I do not think it matters, because the copy is performed later, with
> > > > all the needed checks.
> > >
> > > No, there is no checks at all.
> > >
> >
> > Please elaborate ?
> > Why should maintainers have to spend time to provide evidence to
> > support your claims ?
> > Have you thought about the (compat) case ?
> >
> > There are plenty of checks. They were there before Stanislav commit.
> >
> > Each getsockopt() handler must perform the same actions.
>
>
> But in line 2379 we have ops->getsockopt==NULL case:
>
> 2373         if (!compat)
> 2374                 copy_from_sockptr(&max_optlen, optlen, sizeof(int));
> 2375
> 2376         ops = READ_ONCE(sock->ops);
> 2377         if (level == SOL_SOCKET) {
> 2378                 err = sk_getsockopt(sock->sk, level, optname, optval, optlen);
> 2379         } else if (unlikely(!ops->getsockopt)) {
> 2380                 err = -EOPNOTSUPP;         // <--- HERE
> 2381         } else {
> 2382                 if (WARN_ONCE(optval.is_kernel || optlen.is_kernel,
> 2383                               "Invalid argument type"))
> 2384                         return -EOPNOTSUPP;
> 2385
> 2386                 err = ops->getsockopt(sock, level, optname, optval.user,
> 2387                                       optlen.user);
> 2388         }
>
> where we simply continue with calling BPF_CGROUP_RUN_PROG_GETSOCKOPT()
> which actually needs the 'max_optlen' we copied via copy_from_sockptr().
>
> Do I miss anything here?

This is another great reason why we should not change current behavior.

err will be -EOPNOTSUPP, which was the original error code before
Stanislav patch.

Surely the eBPF program will use this value first, and not even look
at max_optlen

Returning -EFAULT might break some user programs, I don't know.

I feel we are making the kernel slower just because we can.

Re: [PATCH] net: check the return value of the copy_from_sockptr

[Cong Wang]

On Wed, Sep 11, 2024 at 07:15:27PM +0200, Eric Dumazet wrote:
> On Wed, Sep 11, 2024 at 6:58 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > On Wed, Sep 11, 2024 at 11:12:24AM +0200, Eric Dumazet wrote:
> > > On Wed, Sep 11, 2024 at 10:23 AM Qianqiang Liu <qianqiang.liu@163.com> wrote:
> > > >
> > > > > I do not think it matters, because the copy is performed later, with
> > > > > all the needed checks.
> > > >
> > > > No, there is no checks at all.
> > > >
> > >
> > > Please elaborate ?
> > > Why should maintainers have to spend time to provide evidence to
> > > support your claims ?
> > > Have you thought about the (compat) case ?
> > >
> > > There are plenty of checks. They were there before Stanislav commit.
> > >
> > > Each getsockopt() handler must perform the same actions.
> >
> >
> > But in line 2379 we have ops->getsockopt==NULL case:
> >
> > 2373         if (!compat)
> > 2374                 copy_from_sockptr(&max_optlen, optlen, sizeof(int));
> > 2375
> > 2376         ops = READ_ONCE(sock->ops);
> > 2377         if (level == SOL_SOCKET) {
> > 2378                 err = sk_getsockopt(sock->sk, level, optname, optval, optlen);
> > 2379         } else if (unlikely(!ops->getsockopt)) {
> > 2380                 err = -EOPNOTSUPP;         // <--- HERE
> > 2381         } else {
> > 2382                 if (WARN_ONCE(optval.is_kernel || optlen.is_kernel,
> > 2383                               "Invalid argument type"))
> > 2384                         return -EOPNOTSUPP;
> > 2385
> > 2386                 err = ops->getsockopt(sock, level, optname, optval.user,
> > 2387                                       optlen.user);
> > 2388         }
> >
> > where we simply continue with calling BPF_CGROUP_RUN_PROG_GETSOCKOPT()
> > which actually needs the 'max_optlen' we copied via copy_from_sockptr().
> >
> > Do I miss anything here?
> 
> This is another great reason why we should not change current behavior.

Hm? But the current behavior is buggy?

> 
> err will be -EOPNOTSUPP, which was the original error code before
> Stanislav patch.

You mean we should continue calling BPF_CGROUP_RUN_PROG_GETSOCKOPT()
despite -EFAULT?

> 
> Surely the eBPF program will use this value first, and not even look
> at max_optlen
> 
> Returning -EFAULT might break some user programs, I don't know.

As you mentioned above, other ->getsockopt() already returns -EFAULT, so
what is breaking? :)

> 
> I feel we are making the kernel slower just because we can.

Safety and correctness also matter.

Thanks.

Re: [PATCH] net: check the return value of the copy_from_sockptr

[Stanislav Fomichev]

On 09/11, Cong Wang wrote:
> On Wed, Sep 11, 2024 at 07:15:27PM +0200, Eric Dumazet wrote:
> > On Wed, Sep 11, 2024 at 6:58 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > >
> > > On Wed, Sep 11, 2024 at 11:12:24AM +0200, Eric Dumazet wrote:
> > > > On Wed, Sep 11, 2024 at 10:23 AM Qianqiang Liu <qianqiang.liu@163.com> wrote:
> > > > >
> > > > > > I do not think it matters, because the copy is performed later, with
> > > > > > all the needed checks.
> > > > >
> > > > > No, there is no checks at all.
> > > > >
> > > >
> > > > Please elaborate ?
> > > > Why should maintainers have to spend time to provide evidence to
> > > > support your claims ?
> > > > Have you thought about the (compat) case ?
> > > >
> > > > There are plenty of checks. They were there before Stanislav commit.
> > > >
> > > > Each getsockopt() handler must perform the same actions.
> > >
> > >
> > > But in line 2379 we have ops->getsockopt==NULL case:
> > >
> > > 2373         if (!compat)
> > > 2374                 copy_from_sockptr(&max_optlen, optlen, sizeof(int));
> > > 2375
> > > 2376         ops = READ_ONCE(sock->ops);
> > > 2377         if (level == SOL_SOCKET) {
> > > 2378                 err = sk_getsockopt(sock->sk, level, optname, optval, optlen);
> > > 2379         } else if (unlikely(!ops->getsockopt)) {
> > > 2380                 err = -EOPNOTSUPP;         // <--- HERE
> > > 2381         } else {
> > > 2382                 if (WARN_ONCE(optval.is_kernel || optlen.is_kernel,
> > > 2383                               "Invalid argument type"))
> > > 2384                         return -EOPNOTSUPP;
> > > 2385
> > > 2386                 err = ops->getsockopt(sock, level, optname, optval.user,
> > > 2387                                       optlen.user);
> > > 2388         }
> > >
> > > where we simply continue with calling BPF_CGROUP_RUN_PROG_GETSOCKOPT()
> > > which actually needs the 'max_optlen' we copied via copy_from_sockptr().
> > >
> > > Do I miss anything here?
> > 
> > This is another great reason why we should not change current behavior.
> 
> Hm? But the current behavior is buggy?
> 
> > 
> > err will be -EOPNOTSUPP, which was the original error code before
> > Stanislav patch.
> 
> You mean we should continue calling BPF_CGROUP_RUN_PROG_GETSOCKOPT()
> despite -EFAULT?
> 
> > 
> > Surely the eBPF program will use this value first, and not even look
> > at max_optlen
> > 
> > Returning -EFAULT might break some user programs, I don't know.
> 
> As you mentioned above, other ->getsockopt() already returns -EFAULT, so
> what is breaking? :)
> 
> > 
> > I feel we are making the kernel slower just because we can.
> 
> Safety and correctness also matter.

Can you explain what is not correct?

Calling BPF_CGROUP_RUN_PROG_GETSOCKOPT with max_optlen=0 should not be
a problem I think? (the buffer simply won't be accessible to the bpf prog)

Re: [PATCH] net: check the return value of the copy_from_sockptr

[Cong Wang]

On Wed, Sep 11, 2024 at 11:49:32AM -0700, Stanislav Fomichev wrote:
> Can you explain what is not correct?
> 
> Calling BPF_CGROUP_RUN_PROG_GETSOCKOPT with max_optlen=0 should not be
> a problem I think? (the buffer simply won't be accessible to the bpf prog)

Sure. Sorry for not providing all the details.

If I understand the behavior of copy_from_user() correctly, it may
return partially copied data in case of error, which then leads to a
partially-copied 'max_optlen'.

So, do you expect a partially-copied max_optlen to be passed to the
eBPF program meanwhile the user still expects a complete one (since no
-EFAULT)?

Thanks.

Re: [PATCH] net: check the return value of the copy_from_sockptr

[Stanislav Fomichev]

On 09/11, Cong Wang wrote:
> On Wed, Sep 11, 2024 at 11:49:32AM -0700, Stanislav Fomichev wrote:
> > Can you explain what is not correct?
> > 
> > Calling BPF_CGROUP_RUN_PROG_GETSOCKOPT with max_optlen=0 should not be
> > a problem I think? (the buffer simply won't be accessible to the bpf prog)
> 
> Sure. Sorry for not providing all the details.
> 
> If I understand the behavior of copy_from_user() correctly, it may
> return partially copied data in case of error, which then leads to a
> partially-copied 'max_optlen'.
> 
> So, do you expect a partially-copied max_optlen to be passed to the
> eBPF program meanwhile the user still expects a complete one (since no
> -EFAULT)?
> 
> Thanks.

Partial copy is basically the same as user giving us garbage input, right?
That should still be handled correctly I think.

__cgroup_bpf_run_filter_getsockopt (via sockopt_alloc_buf) should handle both cases correctly:
- garbage input / partial copy resulting in negative number -> EINVAL
- garbage input / partial copy resulting in too large number -> potentially
  EFAULT when trying to copy PAGE_SIZE-worth of data

Also, for the EOPNOTSUPP case, we shouldn't even bother copying any data.

Am I missing something?

Re: [PATCH] net: check the return value of the copy_from_sockptr

[Cong Wang]

On Wed, Sep 11, 2024 at 01:05:27PM -0700, Stanislav Fomichev wrote:
> On 09/11, Cong Wang wrote:
> > On Wed, Sep 11, 2024 at 11:49:32AM -0700, Stanislav Fomichev wrote:
> > > Can you explain what is not correct?
> > > 
> > > Calling BPF_CGROUP_RUN_PROG_GETSOCKOPT with max_optlen=0 should not be
> > > a problem I think? (the buffer simply won't be accessible to the bpf prog)
> > 
> > Sure. Sorry for not providing all the details.
> > 
> > If I understand the behavior of copy_from_user() correctly, it may
> > return partially copied data in case of error, which then leads to a
> > partially-copied 'max_optlen'.
> > 
> > So, do you expect a partially-copied max_optlen to be passed to the
> > eBPF program meanwhile the user still expects a complete one (since no
> > -EFAULT)?
> > 
> > Thanks.
> 
> Partial copy is basically the same as user giving us garbage input, right?
> That should still be handled correctly I think.

Not to me.

For explict garbage input, users (mostly syzbot) already expect it is a
garbage.

For partial copy, users expect either an error (like EFAULT) or a success
with the _original_ value.

It is all about expectation of the API.

Thanks.

Re: [PATCH] net: check the return value of the copy_from_sockptr

[Stanislav Fomichev]

On 09/13, Cong Wang wrote:
> On Wed, Sep 11, 2024 at 01:05:27PM -0700, Stanislav Fomichev wrote:
> > On 09/11, Cong Wang wrote:
> > > On Wed, Sep 11, 2024 at 11:49:32AM -0700, Stanislav Fomichev wrote:
> > > > Can you explain what is not correct?
> > > > 
> > > > Calling BPF_CGROUP_RUN_PROG_GETSOCKOPT with max_optlen=0 should not be
> > > > a problem I think? (the buffer simply won't be accessible to the bpf prog)
> > > 
> > > Sure. Sorry for not providing all the details.
> > > 
> > > If I understand the behavior of copy_from_user() correctly, it may
> > > return partially copied data in case of error, which then leads to a
> > > partially-copied 'max_optlen'.
> > > 
> > > So, do you expect a partially-copied max_optlen to be passed to the
> > > eBPF program meanwhile the user still expects a complete one (since no
> > > -EFAULT)?
> > > 
> > > Thanks.
> > 
> > Partial copy is basically the same as user giving us garbage input, right?
> > That should still be handled correctly I think.
> 
> Not to me.
> 
> For explict garbage input, users (mostly syzbot) already expect it is a
> garbage.
> 
> For partial copy, users expect either an error (like EFAULT) or a success
> with the _original_ value.
> 
> It is all about expectation of the API.
> 
> Thanks.

The best way to move this forward is for you to showcase what is exactly
broken by adding a test case to one of the tools/testing/selftests/bpf/*sockopt*
files.

We can then discuss whether it warrants the copy_from_sockptr check or
some other remediation.

RE: [PATCH] net: check the return value of the copy_from_sockptr

[David Laight]

From: Qianqiang Liu
> Sent: 11 September 2024 06:05
> 
> We must check the return value of the copy_from_sockptr. Otherwise, it
> may cause some weird issues.

Actually there is no point doing the copy for optlen in absolutely every function.
'optlen' can be passed as a kernel address and any user copy done by the caller.
Someone should have spotted that before sockptr_t was used for optlen.

The wrapper code can then also do a correct check for (optlen >= 0) which has
been pretty much broken in most protocols for ~ever.
(I wonder if any (important) userspace relies on a negative optlen
being treated as 4.)

It might mean that the final 'copy_to_user' are done in the opposite
order so that an kernel side side effects aren't reversed if the length
can't be copied out - but I suspect that doesn't matter and the paths
are likely to be untested and buggy.

I have toyed with making the getsockopt() functions return a +ve length
or -ve error - but there are a few strange places that need to update
the length and return an error.

	David

> 
> Signed-off-by: Qianqiang Liu <qianqiang.liu@163.com>
> ---
>  net/socket.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/net/socket.c b/net/socket.c
> index 0a2bd22ec105..6b9a414d01d5 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -2370,8 +2370,11 @@ int do_sock_getsockopt(struct socket *sock, bool compat, int level,
>  	if (err)
>  		return err;
> 
> -	if (!compat)
> -		copy_from_sockptr(&max_optlen, optlen, sizeof(int));
> +	if (!compat) {
> +		err = copy_from_sockptr(&max_optlen, optlen, sizeof(int));
> +		if (err)
> +			return -EFAULT;
> +	}
> 
>  	ops = READ_ONCE(sock->ops);
>  	if (level == SOL_SOCKET) {
> --
> 2.39.2
> 

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)