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

Message-ID: <CAKH8qBsB+DGMUBRCa4j+cWuGVg2_GLTZU4G_iun7wJ1GddaNHw@mail.gmail.com>
Date:   Thu, 16 Jun 2022 10:39:49 -0700
From:   Stanislav Fomichev <sdf@...gle.com>
To:     Maciej Żenczykowski <maze@...gle.com>
Cc:     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>,
        YiFei Zhu <zhuyifei@...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'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.

I'd like to see less of the applications poking into errno and making
some decisions based on that. IMO, the only time where it makes sense
is EINTR/EAGAIN vs the rest; the rest should be logged. Having errnos
hard-coded in the tests is fine to make sure that the condition you're
testing against has been triggered; but still treating them as UAPI
might be too much, idk.

We had to add bpf_set_retval() because some of our and third party
libraries would upgrade to v6/v4 sockets only when they receive some
specific errno, which doesn't make a lot of sense to me.