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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CANn89i+zW+t1OEs2ut9Zm6FweY869Yrn-fnjq38tcmqqHLaTvA@mail.gmail.com>
Date: Wed, 11 Sep 2024 14:40:47 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Qianqiang Liu <qianqiang.liu@....com>
Cc: Tze-nan.Wu@...iatek.com, xiyou.wangcong@...il.com, davem@...emloft.net, 
	kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: check the return value of the copy_from_sockptr

On Wed, Sep 11, 2024 at 1:14 PM Qianqiang Liu <qianqiang.liu@....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@....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.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ