netdev - Re: [PATCH bpf 1/2] xsk: fix to reject invalid flags in xsk

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]

Message-ID: <91b16f06-9a3d-603c-37f1-9f617fac5793@iogearbox.net>
Date:   Fri, 8 Mar 2019 14:26:04 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Björn Töpel <bjorn.topel@...il.com>,
        Maciej Fijalkowski <maciejromanfijalkowski@...il.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Netdev <netdev@...r.kernel.org>,
        Björn Töpel <bjorn.topel@...el.com>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        Magnus Karlsson <magnus.karlsson@...il.com>,
        bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH bpf 1/2] xsk: fix to reject invalid flags in xsk_bind

On 03/08/2019 12:06 PM, Björn Töpel wrote:
> On Fri, 8 Mar 2019 at 11:46, Maciej Fijalkowski
> <maciejromanfijalkowski@...il.com> wrote:
[...]
>>>> So maybe check here also that only one particular flag is set by doing:
>>>>
>>>> if (hweight32(flags & (XDP_SHARED_UMEM | XDP_COPY | XDP_ZEROCOPY)) > 1)
>>>>         return -EINVAL;
>>>>
>>>> just like we do it for IFLA_XDP_FLAGS in net/core/rtnetlink.c?
>>>
>>> We have flag semantic checks further down, and my rational was to
>>> *only* check unknown flags first. IMO the current patch is easier to
>>> understand, than your suggested one.
>>
>> Hmm thought that bailing out earlier would be better and we could drop the
>> actual copy flags checks for shared umem. For xdp_umem_assign_dev() instead of
>> passing flags you could just pass a boolean whether you're doing zero copy or
>> not. And that brings up the question whether we really need a XDP_COPY flag?
> 
> I'd prefer doing that as a follow-up patch.

Ok, follow-up for Maciej's suggestion is fine, imho, it would definitely make
it more obvious resp. future proof such that rejecting some invalid combination
doesn't get missed under some branch. Given it's currently not the case aside
from the fix, rolling this into your next patches for bpf-next might be best.
In any case, fixes look good, so applied, thanks!

> XDP_COPY is needed to explicitly enable copy-mode. No flags is "select
> the best option", and COPY/ZEROCOPY is to explicitly select a mode.