[<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