netdev - Re: Curious bpf regression in 5.18 already fixed in stable 5.18.3

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] [day] [month] [year] [list]

Date:   Thu, 16 Jun 2022 14:21:56 -0700
From:   YiFei Zhu <zhuyifei@...gle.com>
To:     Maciej Żenczykowski <maze@...gle.com>
Cc:     Stanislav Fomichev <sdf@...gle.com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Linux NetDev <netdev@...r.kernel.org>,
        BPF Mailing List <bpf@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Sasha Levin <sashal@...nel.org>,
        Carlos Llamas <cmllamas@...gle.com>
Subject: Re: Curious bpf regression in 5.18 already fixed in stable 5.18.3

On Thu, Jun 16, 2022 at 9:41 AM Maciej Żenczykowski <maze@...gle.com> wrote:
>
> On Thu, Jun 16, 2022 at 8:57 AM Stanislav Fomichev <sdf@...gle.com> wrote:
> > On Wed, Jun 15, 2022 at 6:36 PM Maciej Żenczykowski <maze@...gle.com> wrote:
> > > I'm guessing this means the regression only affects 64-bit archs,
> > > where long = void* is 8 bytes > u32 of 4 bytes, but not 32-bit ones,
> > > where long = u32 = 4 bytes
> > >
> > > Unfortunately my dev machine's 32-bit build capability has somehow
> > > regressed again and I can't check this.
> >
> > Seems so, yes. But I'm actually not sure whether we should at all
> > treat it as a regression. There is a question of whether that EPERM is
> > UAPI or not. That's why we most likely haven't caught it in the
> > selftests; most of the time we only check that syscall has returned -1
> > and don't pay attention to the particular errno.
>
> EFAULT seems like a terrible error to return no matter what, it has a very clear
> 'memory read/write access violation' semantic (ie. if you'd done from
> userspace you'd get a SIGSEGV)

I chose EFAULT because the original code of getsockopt hook returns
-EFAULT if the retval is set to a number that isn't zero or the
original value. i.e., in c4dcfdd406aa^, there was:

  /* BPF programs only allowed to set retval to 0, not some
   * arbitrary value.
   */
  if (ctx.retval != 0 && ctx.retval != retval) {
          ret = -EFAULT;
          goto out;
  }

I understood that as the convention that if a BPF program does
something illegal at runtime, return -EFAULT.

> I'm actually surprised to learn you return EFAULT on positive number...
> It should rather be some unique error code or EINVAL or something.
>
> I know someone will argue that (most/all) system calls can return EFAULT...
> But that's not actually true.  From a userspace developer the expectation is
> they will not return EFAULT if you pass in memory you know is good.
>
> #include <sys/utsname.h>
> int main() {
>   struct utsname uts;
>   uname(&uts);
> }
>
> The above cannot EFAULT in spite of it being documented as the only
> error uname can report,
> because obviously the uts structure on the stack is valid memory.
>
> Maybe ENOSYS would at least make it obvious something is very weird.