netdev - Re: [PATCH net] nfp: bpf: Add an MTU check before offloading BPF

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 30 Sep 2021 16:46:34 +0200
From:   Niklas Söderlund 
        <niklas.soderlund@...igine.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Simon Horman <simon.horman@...igine.com>,
        David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
        oss-drivers@...igine.com, Yu Xiao <yu.xiao@...igine.com>,
        Yinjun Zhang <yinjun.zhang@...igine.com>,
        Louis Peens <louis.peens@...igine.com>
Subject: Re: [PATCH net] nfp: bpf: Add an MTU check before offloading BPF

Hello Jackub,

Thanks for your feedback.

On 2021-09-29 11:47:48 -0700, Jakub Kicinski wrote:
> On Wed, 29 Sep 2021 17:24:21 +0200 Simon Horman wrote:
> > From: Yu Xiao <yu.xiao@...igine.com>
> > 
> > There is a bug during xdpoffloading. When MTU is bigger than the
> > max MTU of BFP (1888), it can still be added xdpoffloading.
> > 
> > Therefore, add an MTU check to ensure that xdpoffloading cannot be
> > loaded when MTU is larger than a max MTU of 1888.
> 
> There is a check in nfp_net_bpf_load(). TC or XDP, doesn't matter,
> we can't offload either with large MTU since the FW helper (used to be) 
> able to only access CTM. So the check is on the generic path, adding
> an XDP-specific check seems wrong.

I understand your point and it make sens. The check in 
nfp_net_bpf_load() in the generic path do indeed check for this, but in 
a slightly different way. It verifies that the BPF program don't access 
any data that is not in CMT.

The original problem this patch tried to address was to align the 
behavior that the MTU is verified differently when the BPF program is 
loaded and when the MTU is changed once the program is loaded.

Without this patch we had the following behavior,

    # ip link set ens5np0 mtu 9000
    # ip link set dev ens5np0 xdpoffload obj bpf_prog.o sec testcase
    # ip link show dev ens5np0
    11: ens5np0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 9000 xdpoffload qdisc mq state UP mode DEFAULT group default qlen 1000
	link/ether 00:15:4d:13:61:91 brd ff:ff:ff:ff:ff:ff
	prog/xdp id 48 tag 57cd311f2e27366b jited
    # ip link set ens5np0 mtu 1500
    # ip link set ens5np0 mtu 9000
    RTNETLINK answers: Device or resource busy
    # ip link set ens5np0 mtu 1888
    # ip link show dev ens5np0
    11: ens5np0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1888 xdpoffload qdisc mq state UP mode DEFAULT group default qlen 1000
        link/ether 00:15:4d:13:61:91 brd ff:ff:ff:ff:ff:ff
        prog/xdp id 48 tag 57cd311f2e27366b jited 

When the MTU is changed after the program is offloaded the check in 
nfp_bpf_check_mtu() is consulted and as it checks the MTU differently 
and fails the change. Maybe we should align this the other way around 
and update the check in nfp_bpf_check_mtu() to match the one in 
nfp_net_bpf_load()?

On a side note the check in nfp_net_bpf_load() allows for BPF programs 
to be offloaded that do access data beyond the CMT size limit provided 
the MTU is set below the CMT threshold value. There should be no real 
harm in this as the verifier forces bounds check so with a MTU small 
enough it should never happen. But maybe we should add a check for this 
too to prevent such a program to be loaded in the first place.

Thanks again for your input.

-- 
Regards,
Niklas Söderlund